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

Remove extra HTML encoding #17932

Closed

Conversation

cedric-anne
Copy link
Member

Checklist before requesting a review

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my code.

Description

Follows #17927 and #17928

Commit 1: The /front/cron.php script is often use in CLI context. The HTML special chars should not be encoded in this case. I also encapsulate the strings in the __() function to make them translatable.

Commit 2: comments returned by Dropdown::getDropdownName() contains legitimate HTML, it should not be re-encoded.

@cedric-anne cedric-anne self-assigned this Sep 25, 2024
@trasher
Copy link
Contributor

trasher commented Sep 26, 2024

Since the cron script is not really made to be called with a specific language (and anyway it can be a different language than the one who will receive cron message) - I'm not sure making strings translatable is really a good idea.
Also, it requires extra work from translators.

@trasher
Copy link
Contributor

trasher commented Sep 26, 2024

Commit 2: comments returned by Dropdown::getDropdownName() contains legitimate HTML, it should not be re-encoded.

Well... That's a hard case to handle; because it can always contain HTML we have to escape (and a get method should not do that). Seems wrong not to escape, and seems wrong to escape.... I've thrown a LogicalException :D

@cedric-anne
Copy link
Member Author

cedric-anne commented Sep 26, 2024

Since the cron script is not really made to be called with a specific language (and anyway it can be a different language than the one who will receive cron message) - I'm not sure making strings translatable is really a good idea. Also, it requires extra work from translators.

We could remove translations, and then we would be sure that we have nothing to encode, as the strings does not contain >, < or &.

@cedric-anne
Copy link
Member Author

Commit 2: comments returned by Dropdown::getDropdownName() contains legitimate HTML, it should not be re-encoded.

Well... That's a hard case to handle; because it can always contain HTML we have to escape (and a get method should not do that). Seems wrong not to escape, and seems wrong to escape.... I've thrown a LogicalException :D

This method should be splitted, as I have done for the getUserName() function in #17872, to have a method that returns the raw (non encoded) name, and a method that returns the "tooltip HTML code" where variables are correctly encoded.

I will try to implement this solution.

@cedric-anne
Copy link
Member Author

See #17981 and #17982 .

@cedric-anne cedric-anne closed this Oct 1, 2024
@cedric-anne cedric-anne deleted the 11.0/fix-extra-escaping branch October 1, 2024 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants