Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Malformed URL in real-time-visitors module #15233

Merged
merged 1 commit into from Dec 8, 2019
Merged

Fix Malformed URL in real-time-visitors module #15233

merged 1 commit into from Dec 8, 2019

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Dec 3, 2019

This issue was created in WP Matomo so was investigating and could reproduce this eg by tracking

_paq.push(['trackLink', 'http://mydomain.co.uk/mailto/Agent', 'download']);

It adds the http:// prefix twice causing the links to not work. Maybe this was a regression from when refactoring things into visitorDetails? It's probably more a workaround since the actual issue might be that it shouldn't set the url prefix when it keeps the http url, or maybe it should remove the http in the action when it sets a url prefix...

This might be a more proper fix but not sure if it would regress anything

--- a/app/plugins/Actions/Actions/ActionDownloadUrl.php
+++ b/app/plugins/Actions/Actions/ActionDownloadUrl.php
@@ -34,7 +34,7 @@ class ActionDownloadUrl extends Action
     {
         return array(
             // Note: we do not normalize download URL
-            'idaction_url' => array($this->getActionUrl(), $this->getActionType())
+               'idaction_url'  => $this->getUrlAndType()
         );
     }

(it would actually use the wrong type Page URL and would need to adjust this...) I reckon we better don't change this in a minor/patch release.

fix #15232

This issue was created in WP Matomo so was investigating and could reproduce this eg by tracking

```js
_paq.push(['trackLink', 'http://mydomain.co.uk/mailto/Agent', 'download']);
```

It adds the http:// prefix twice causing the links to not work. Maybe this was a regression from when refactoring things into visitorDetails? It's probably more a workaround since the actual issue might be that it shouldn't set the url prefix when it keeps the http url, or maybe it should remove the http in the action when it sets a url prefix... 

This might be a more proper fix but not sure if it would regress anything

```diff
--- a/app/plugins/Actions/Actions/ActionDownloadUrl.php
+++ b/app/plugins/Actions/Actions/ActionDownloadUrl.php
@@ -34,7 +34,7 @@ class ActionDownloadUrl extends Action
     {
         return array(
             // Note: we do not normalize download URL
-            'idaction_url' => array($this->getActionUrl(), $this->getActionType())
+               'idaction_url'  => $this->getUrlAndType()
         );
     }
 ```

(it would actually use the wrong type Page URL and would need to adjust this...) I reckon we better don't change this in a minor/patch release.
@tsteur tsteur added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Dec 3, 2019
@tsteur tsteur added this to the 3.13.1 milestone Dec 3, 2019
@diosmosis
Copy link
Member

Should we create the original issue open or create a new one for matomo 4 to fix this in a different way later?

@tsteur
Copy link
Member Author

tsteur commented Dec 8, 2019

@diosmosis created #15246 for now but not sure we actually need to schedule it just yet. Not sure if there's any issue with setting url_prefix and protocol. So far other things seem to work nicely.

@diosmosis diosmosis merged commit af6a530 into 3.x-dev Dec 8, 2019
@diosmosis diosmosis deleted the 15232 branch December 8, 2019 19:56
jonasgrilleres pushed a commit to 1024pix/pix-analytics that referenced this pull request Sep 22, 2020
This issue was created in WP Matomo so was investigating and could reproduce this eg by tracking

```js
_paq.push(['trackLink', 'http://mydomain.co.uk/mailto/Agent', 'download']);
```

It adds the http:// prefix twice causing the links to not work. Maybe this was a regression from when refactoring things into visitorDetails? It's probably more a workaround since the actual issue might be that it shouldn't set the url prefix when it keeps the http url, or maybe it should remove the http in the action when it sets a url prefix... 

This might be a more proper fix but not sure if it would regress anything

```diff
--- a/app/plugins/Actions/Actions/ActionDownloadUrl.php
+++ b/app/plugins/Actions/Actions/ActionDownloadUrl.php
@@ -34,7 +34,7 @@ class ActionDownloadUrl extends Action
     {
         return array(
             // Note: we do not normalize download URL
-            'idaction_url' => array($this->getActionUrl(), $this->getActionType())
+               'idaction_url'  => $this->getUrlAndType()
         );
     }
 ```

(it would actually use the wrong type Page URL and would need to adjust this...) I reckon we better don't change this in a minor/patch release.
jbuget pushed a commit to 1024pix/pix-analytics that referenced this pull request Sep 26, 2020
This issue was created in WP Matomo so was investigating and could reproduce this eg by tracking

```js
_paq.push(['trackLink', 'http://mydomain.co.uk/mailto/Agent', 'download']);
```

It adds the http:// prefix twice causing the links to not work. Maybe this was a regression from when refactoring things into visitorDetails? It's probably more a workaround since the actual issue might be that it shouldn't set the url prefix when it keeps the http url, or maybe it should remove the http in the action when it sets a url prefix... 

This might be a more proper fix but not sure if it would regress anything

```diff
--- a/app/plugins/Actions/Actions/ActionDownloadUrl.php
+++ b/app/plugins/Actions/Actions/ActionDownloadUrl.php
@@ -34,7 +34,7 @@ class ActionDownloadUrl extends Action
     {
         return array(
             // Note: we do not normalize download URL
-            'idaction_url' => array($this->getActionUrl(), $this->getActionType())
+               'idaction_url'  => $this->getUrlAndType()
         );
     }
 ```

(it would actually use the wrong type Page URL and would need to adjust this...) I reckon we better don't change this in a minor/patch release.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Malformed URL in real-time-visitors module
2 participants