-
Notifications
You must be signed in to change notification settings - Fork 798
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
Refactor ImageCDN parsing to rely on HTML API instead of RegExps #32700
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped. |
ba4718b
to
564599c
Compare
b318fbb
to
dc3a6b1
Compare
6b724e7
to
583e9ca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for proposing this! This is a nice way to use WP_HTML_Tag_Processor
, and something we can do now that we'll be requiring WordPress 6.2 (#31638).
I have not tested your PR, but left a couple of comments below about how things work in the monorepo, if that can help you discover how we usually do things :)
@haqadn @Automattic/heart-of-gold can you make sure you keep an eye on this one? |
484deb0
to
ffab63f
Compare
e51843b
to
5e23682
Compare
One note I should leave here is to ask about pulling in Core's The issue here is that the test output changes but in a semantically neutral way. I could rewrite the test so that it hardcodes the newer ordering of output, but that would leave this fragility in place for the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should I try and find someone to review this? are either of you happy with the proposed change?
Maybe @Automattic/heart-of-gold could take a look?
/** | ||
* Allow specific images to be skipped by Photon. | ||
* | ||
* @TODO: Does this need to pass the full HTML of the image tag? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at usage of this filter in other plugins in the WordPress.org plugin directory. While most plugins do not use this third parameter, there are a few that do, sometimes to look for a specific class name in the HTML. So I think we should keep this available if possible.
* | ||
* @see https://developer.wordpress.com/docs/photon/api/ | ||
* @TODO: What is the point of passing $images and $index. Are they required? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked around but couldn't figure out where that filter is being used today. It was used in the past, but may not be used anymore today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code changes look good to me and no problems found after testing. 👍🏼
P.S.: I only worked on migrating from a Jetpack module to image-cdn package. I don't have prior knowledge of the implementation details on the core functionality.
I will reconstruct the existing filter value before merge. |
@@ -178,6 +178,51 @@ protected function helper_remove_image_sizes() { | |||
remove_image_size( 'jetpack_soft_oversized_after_upload' ); | |||
} | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these functions are okay here.
Personally, I would put them in the parent class at class-image-cdn-attachment-test-case.php
but, it looks like this is the only (current) test that extends this particular class.
Since we're late in the beta testing week, I'd say let's merge this next week after the branch cut so we get the longest amount of time to test it in the wild before the next dotorg release |
This PR has been marked as stale. This happened because:
If this PR is still useful, please do a [trunk merge or rebase](https://github.com/Automattic/jetpack/blob/trunk/docs/git-workflow.md#keeping-your-branch-up-to-date) and otherwise make sure it's up to date and has clear testing instructions. You may also want to ping possible reviewers in case they've forgotten about it. Please close this PR if you think it's not valid anymore — if you do, please add a brief explanation. If the PR is not updated (or at least commented on) in another month, it will be automatically closed. |
whoops, was I supposed to merge this? I'll try and rebase the branch and get it ready again without changing the code, then maybe we can merge next week or so. |
5e23682
to
40cd179
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just went through filter_the_content
line-by-line, comparing its functionality to the original. Well done, I think you have nailed it!
I do have a couple of minor questions and suggestions - but absolutely none of them are blockers in my opinion so I'm giving this a ✅ on behalf of Heart of Gold.
Should we also remove parse_images_from_html
which now only appears in its test suite?
return null; | ||
} | ||
|
||
$search_pattern = sprintf( '#(?:^|[ \t\f\r\n])%s(?P<value>[^ \t\f\r\n]+)#', preg_quote( $prefix, null ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style question: Is there a difference between [\s]
and [ \t\f\r\n]
? If not, would it be more readable using \s
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good question! I'm so used to the HTML API where we've been very explicit about these that I didn't think about it, but the PHP docs suggest that \s
may be affected by locale options. I think it would be just as good to leave this as-is, though \s
is unlikely to be a big problem in practice so if you prefer it I'm happy to make that change.
apparently it may include additional unicode characters representing whitespace if those locale rules are active, which would diverge from how the CSS class names are parsed in the browser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unicode can't let anything be simple, not even whitespace.
What you've written would be closer to the HTML spec, so let's stick with how you've done it.
* @param string $tag Image Tag (Image HTML output). | ||
*/ | ||
if ( apply_filters( 'jetpack_photon_skip_image', false, $src, $tag ) ) { | ||
// @TODO: Do "PICTURE" and "SOURCE" belong here as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I believe we should target these. However, I would do it in a separate PR to keep the reviewing/auditing easier, and less likely to produce surprises.
$size = self::parse_class_name_prefixed_by( $class, 'size-' ); | ||
if ( null !== $size && false === $width && false === $height && 'full' !== $size && array_key_exists( $size, $image_sizes ) ) { | ||
$width = (int) $image_sizes[ $size ]['width']; | ||
$height = (int) $image_sizes[ $size ]['height']; | ||
$transform = $image_sizes[ $size ]['crop'] ? 'resize' : 'fit'; | ||
} else { | ||
unset( $size ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor point, but can we skip calling parse_class_name_prefixed_by
entirely if width
or height
are not false
, skipping some regexes per-image where sizes are explicitly set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call, why do more work than we need to?
04841f7
to
d01dfa5
Compare
) The introduction of the HTML API into WordPress 6.2 offers a new method of matching and modifying HTML. In this patch we're replacing code that attempts to parse the input HTML and visit all images, then read and modify them based on the values of their attributes and computed Photon properties. In the previous code the `Image_CDN` class scanned the entire HTML document to generate a list of PREG image match objects, then iterated over those matches and performed string-replace operations on them. Now the class does a pass from start to finish, visting each image tag along the way, and making the appropriate modifications. Co-authored-by: Adnan Haque <3737780+haqadn@users.noreply.github.com> Co-authored-by: Brandon Kraft <public@brandonkraft.com> Co-authored-by: Jeremy Herve <jeremy@jeremy.hu> Co-authored-by: Mark George <thingalon@gmail.com> Co-authored-by: Osk <oskosk@users.noreply.github.com>
All, thanks for your feedback; I've finally updated this PR and I think I have addressed everything. It now respects the old value for the |
d01dfa5
to
272668d
Compare
) The introduction of the HTML API into WordPress 6.2 offers a new method of matching and modifying HTML. In this patch we're replacing code that attempts to parse the input HTML and visit all images, then read and modify them based on the values of their attributes and computed Photon properties. In the previous code the `Image_CDN` class scanned the entire HTML document to generate a list of PREG image match objects, then iterated over those matches and performed string-replace operations on them. Now the class does a pass from start to finish, visting each image tag along the way, and making the appropriate modifications. Co-authored-by: Adnan Haque <3737780+haqadn@users.noreply.github.com> Co-authored-by: Brandon Kraft <public@brandonkraft.com> Co-authored-by: Jeremy Herve <jeremy@jeremy.hu> Co-authored-by: Mark George <thingalon@gmail.com> Co-authored-by: Osk <oskosk@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed call to WP_HTML_Tag_Processor
by using the correct case. Looks like the PR is ready to get merged.
) The introduction of the HTML API into WordPress 6.2 offers a new method of matching and modifying HTML. In this patch we're replacing code that attempts to parse the input HTML and visit all images, then read and modify them based on the values of their attributes and computed Photon properties. In the previous code the `Image_CDN` class scanned the entire HTML document to generate a list of PREG image match objects, then iterated over those matches and performed string-replace operations on them. Now the class does a pass from start to finish, visting each image tag along the way, and making the appropriate modifications. Co-authored-by: Adnan Haque <3737780+haqadn@users.noreply.github.com> Co-authored-by: Brandon Kraft <public@brandonkraft.com> Co-authored-by: Jeremy Herve <jeremy@jeremy.hu> Co-authored-by: Mark George <thingalon@gmail.com> Co-authored-by: Osk <oskosk@users.noreply.github.com>
1842583
to
ab212e3
Compare
thanks @haqadn. sorry all I've been holding out on this so I can try and test it before merging. too much to do! |
This PR has been marked as stale. This happened because:
If this PR is still useful, please do a [trunk merge or rebase](https://github.com/Automattic/jetpack/blob/trunk/docs/git-workflow.md#keeping-your-branch-up-to-date) and otherwise make sure it's up to date and has clear testing instructions. You may also want to ping possible reviewers in case they've forgotten about it. Please close this PR if you think it's not valid anymore — if you do, please add a brief explanation. If the PR is not updated (or at least commented on) in another month, it will be automatically closed. |
This PR has been automatically closed as it has not been updated in some time. If you want to resume work on the PR, feel free to restore the branch and reopen the PR. |
Status
This is a work in progress and isn't tested or verified.This has been reviewed, but the filters aren't tested because it's unclear what code might rely on them.Due to the change in indentation the diff view is more associated with the actual changes if ignoring whitespace.
Proposed changes:
The introduction of the HTML API into WordPress 6.2 offers a new method of matching and modifying HTML. In this patch we're replacing code that attempts to parse the input HTML and extract images that are direct children of an anchor ("A" tag), then read and modify them based on the values of their attributes and computed Photon properties.
In the previous code the
Image_CDN
class scanned the entire HTML document to generate a list of PREG image match objects, then iterated over those matches and performed string-replace operations on them.Now the class does a pass from start to finish, visting each image tag along the way, and making the appropriate modifications. Extra care is taken to ensure that only images that are the single child of a link are matched.
In this change the values of the
tag
key in some of the filters has changed from the initial matched HTML snippet to the name of the image tag, which could beIMG
orAMP-IMG
orAMP-ANIM
. An update to the Tag Processor or a custom sub-class thereof could provide the original HTML snippet and match the existing behavior, but that hasn't been done in this patch yet given the author's uncertainty about the use and value of those snippets.Other information:
Jetpack product discussion
This mandates running on WordPress 6.2. If we prefer being able to run on older versions, we could also include a copy of the Tag Processor in the plugin.
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
Test suite should pass. Manual review is necessary though.
I don't even know what all the function I modified is supposed to do fully, so code auditing and review of the modifications is critical.