Skip to content

Fix: notification links with ampersands in page titles work correctly#6348

Open
hpr wants to merge 3 commits intowikimedia:mainfrom
hpr:fix-notification-link-ampersand
Open

Fix: notification links with ampersands in page titles work correctly#6348
hpr wants to merge 3 commits intowikimedia:mainfrom
hpr:fix-notification-link-ampersand

Conversation

@hpr
Copy link
Copy Markdown

@hpr hpr commented Feb 26, 2026

What does this do?

Fixes notification links where page titles have ampersands. These shouldn't be decoded because it breaks parsing by the notification link handler.

Example of bug in Wikipedia Beta:
EmptyListError

Example of fixed behavior:
T418432

Why is this needed?

To prevent "Empty list doesn't contain element at index 0." bugs on notifications

Phabricator:
https://phabricator.wikimedia.org/T418432

Copy link
Copy Markdown
Collaborator

@cooltey cooltey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for submitting the PR to fix it! Removing the decodeURL() here may solve the issue in English articles that contain & in the title, but for the article in other wikis, the URL will be encoded, and if we remove this here, the links will become unavailable and cause more issues.

@hpr
Copy link
Copy Markdown
Author

hpr commented Feb 27, 2026

@cooltey, thanks, I tried another approach by storing the encoded URL separately. Compiled this and observed that the fix still works with the above article, if there's another test case I should try let me know.

@hpr hpr requested a review from cooltey March 2, 2026 00:33
@dbrant
Copy link
Copy Markdown
Member

dbrant commented Mar 4, 2026

Thanks! Let's standby on this -- I actually think your original fix is the right idea; we should remove instances of decoding/encoding URLs in places where it's not clear why it's done. And we should definitely not keep both encoded and decoded url fields in a model class like Notification, again without a super obvious reason.

I have a feeling that the proper fix involves just a little more digging into where all these URLs eventually end up, and performing the URL decoding in that single place, instead of multiple places that shouldn't need to be "aware" of url encoding.

@hpr hpr force-pushed the fix-notification-link-ampersand branch from f801603 to 7cc6cc7 Compare March 12, 2026 00:17
mohammadmanzu1-creator

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants