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

Limit decoding attribute to images with src starting with double quote #3586

Closed
wants to merge 8 commits into from

Conversation

adamsilverstein
Copy link
Member

Trac ticket: https://core.trac.wordpress.org/ticket/56969


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@adamsilverstein Looks solid to me, just two nit-picks here in the test HTML.

tests/phpunit/tests/media.php Outdated Show resolved Hide resolved
tests/phpunit/tests/media.php Outdated Show resolved Hide resolved
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

This seems Ok as temporary fix but as single quotes are a perfectly legitimate form of HTML, it would be good to follow up with something more elegant.

WordPress itself uses the quote types interchangeably and it seems to be fairly common in plugins too, especially when generating the markup from a set of variables.

"<img alt='%s' src='%s' srcset='%s' class='%s' height='%d' width='%d' %s/>",

tests/phpunit/tests/media.php Outdated Show resolved Hide resolved
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Thanks @adamsilverstein. It looks good to me left some feedback for space and text changes.

https://github.com/WordPress/wordpress-develop/blob/trunk/tests/phpunit/tests/media/wpImageTagAddDecodingAttr.php contains all the unit tests for wp_img_tag_add_decoding_attr() so is it better to move this new unit test there?

src/wp-includes/media.php Outdated Show resolved Hide resolved
tests/phpunit/tests/media.php Outdated Show resolved Hide resolved
tests/phpunit/tests/media.php Outdated Show resolved Hide resolved
tests/phpunit/tests/media.php Outdated Show resolved Hide resolved
desrosj and others added 4 commits November 10, 2022 11:24
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
@desrosj
Copy link
Contributor

desrosj commented Nov 10, 2022

Applied some of the more straightforward suggestions. @adamsilverstein would you be able to get this committed at some point today?

@adamsilverstein
Copy link
Member Author

Yes, will do.

adamsilverstein and others added 3 commits November 10, 2022 13:03
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
@adamsilverstein
Copy link
Member Author

This seems Ok as temporary fix but as single quotes are a perfectly legitimate form of HTML, it would be good to follow up with something more elegant.

WordPress itself uses the quote types interchangeably and it seems to be fairly common in plugins too, especially when generating the markup from a set of variables.

"<img alt='%s' src='%s' srcset='%s' class='%s' height='%d' width='%d' %s/>",

I agree it would be nice to have a more robust solution; this one seems ok for now - especially since the lazy attribute callback already has an identical check.

@adamsilverstein
Copy link
Member Author

Applied some of the more straightforward suggestions. @adamsilverstein would you be able to get this committed at some point today?

@desrosj I'll get this committed and backported first thing tomorrow if you don't beat me to it.

@desrosj desrosj changed the title Limit decoing attribute to images with src starting with double quote Limit decoding attribute to images with src starting with double quote Nov 11, 2022
@peterwilsoncc
Copy link
Contributor

Merged in https://core.trac.wordpress.org/changeset/54802 / 5103242

@adamsilverstein
Copy link
Member Author

thanks @peterwilsoncc!

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.

5 participants