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 incorrect URL parameter in update notification #21447

Merged
merged 9 commits into from
Oct 30, 2023

Conversation

bx80
Copy link
Contributor

@bx80 bx80 commented Oct 26, 2023

Description:

Fixes #21446

Corrects an invalid URL causing a JS crash when an update is detected.

Review

@bx80 bx80 added Bug For errors / faults / flaws / inconsistencies etc. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Regression Indicates a feature used to work in a certain way but it no longer does even though it should. labels Oct 26, 2023
@bx80 bx80 added this to the 5.0.0 milestone Oct 26, 2023
@bx80 bx80 self-assigned this Oct 26, 2023
@michalkleiner
Copy link
Contributor

Should we perhaps wrap https://github.com/matomo-org/matomo/blob/5.x-dev/plugins/Morpheus/javascripts/piwikHelper.js#L36-L58 in try {} catch(err) {} block and return empty string if there's an error thrown? It would prevent the hard fail, but at the same time we would not have caught the incorrect input string 🤷.

@bx80
Copy link
Contributor Author

bx80 commented Oct 26, 2023

I think that would be a better outcome than a hard fail, it's possible that future usage of the external link methods, either in core or plugins, could result in invalid URLs causing a similar crash, so minimizing the potential impact makes sense 👍

@bx80 bx80 merged commit c5bce87 into 5.x-dev Oct 30, 2023
17 of 20 checks passed
@bx80 bx80 deleted the m21446-fix-url-error-on-update branch October 30, 2023 02:09
caddoo pushed a commit that referenced this pull request Nov 5, 2023
* Fix incorrect URL parameter

* Fix incorrect URL parameter in JS tracking code generator component

* built vue files

* Improve error handling of invalid URLs in _pk_externalRawLink

* Update plugins/Morpheus/javascripts/piwikHelper.js

Co-authored-by: Michal Kleiner <[email protected]>

* built vue files

* Bump stalled tests

---------

Co-authored-by: Michal Kleiner <[email protected]>
Co-authored-by: michalkleiner <[email protected]>
Co-authored-by: bx80 <[email protected]>
caddoo pushed a commit that referenced this pull request Nov 5, 2023
* Fix incorrect URL parameter

* Fix incorrect URL parameter in JS tracking code generator component

* built vue files

* Improve error handling of invalid URLs in _pk_externalRawLink

* Update plugins/Morpheus/javascripts/piwikHelper.js

Co-authored-by: Michal Kleiner <[email protected]>

* built vue files

* Bump stalled tests

---------

Co-authored-by: Michal Kleiner <[email protected]>
Co-authored-by: michalkleiner <[email protected]>
Co-authored-by: bx80 <[email protected]>
caddoo pushed a commit that referenced this pull request Nov 5, 2023
* Fix incorrect URL parameter

* Fix incorrect URL parameter in JS tracking code generator component

* built vue files

* Improve error handling of invalid URLs in _pk_externalRawLink

* Update plugins/Morpheus/javascripts/piwikHelper.js

Co-authored-by: Michal Kleiner <[email protected]>

* built vue files

* Bump stalled tests

---------

Co-authored-by: Michal Kleiner <[email protected]>
Co-authored-by: michalkleiner <[email protected]>
Co-authored-by: bx80 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Blank screen crash when showing update notification
2 participants