Refactor ImageCDN parsing to rely on HTML API instead of RegExps #32700

dmsnell · 2023-08-26T01:02:00Z

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 be IMG or AMP-IMG or AMP-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:

Have you written new tests for your changes, if applicable?
Have you checked the E2E test CI results, and verified that your changes do not break them?
Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

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.

github-actions · 2023-08-26T01:03:09Z

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

To test on WoA, go to the Plugins menu on a WordPress.com Simple site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin, and enable the image-cdn/rely-on-html-api branch.

To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack image-cdn/rely-on-html-api

Interested in more tips and information?

In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
Read more about our development workflow here: PCYsg-eg0-p2
Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

github-actions · 2023-08-26T01:03:18Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

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.
Then, add the "[Status] Needs Team Review" label and ask someone from your team review the code. Once reviewed, it can then be merged.
If you need an extra review from someone familiar with the codebase, you can update the labels from "[Status] Needs Team Review" to "[Status] Needs Review", and in that case Jetpack Approvers will do a final review of your PR.

jeherve

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 :)

projects/packages/image-cdn/src/class-image-cdn.php

projects/packages/image-cdn/CHANGELOG.md

oskosk · 2023-08-29T16:00:23Z

@haqadn @Automattic/heart-of-gold can you make sure you keep an eye on this one?

dmsnell · 2023-09-05T17:56:23Z

@oskosk @jeherve should I try and find someone to review this? are either of you happy with the proposed change?

dmsnell · 2023-09-05T17:58:52Z

One note I should leave here is to ask about pulling in Core's assertEqualMarkup() function. I wanted to add it to the BaseTestCase but then I saw that's an external class and wasn't sure where to add it so that it would be available all around Jetpack.

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.

jeherve

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?

jeherve · 2023-09-06T09:02:28Z

projects/packages/image-cdn/src/class-image-cdn.php

+			/**
+			 * Allow specific images to be skipped by Photon.
+			 *
+			 * @TODO: Does this need to pass the full HTML of the image tag?


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.

jeherve · 2023-09-06T09:16:38Z

projects/packages/image-cdn/src/class-image-cdn.php

 					 *
-					 * @see https://developer.wordpress.com/docs/photon/api/
+					 * @TODO: What is the point of passing $images and $index. Are they required?


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.

haqadn

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.

dmsnell · 2023-09-13T19:08:37Z

I will reconstruct the existing filter value before merge.
Would still love to have some thoughts on the assertEquivalentMarkup() method from Core, if there's a better place to put that.

kraftbj · 2023-09-14T20:40:57Z

projects/packages/image-cdn/tests/php/test_class.image_cdn.php

@@ -178,6 +178,51 @@ protected function helper_remove_image_sizes() {
 		remove_image_size( 'jetpack_soft_oversized_after_upload' );
 	}

+	/**


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.

kraftbj · 2023-09-14T20:42:41Z

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

github-actions · 2023-12-14T00:49:19Z

This PR has been marked as stale. This happened because:

It has been inactive for the past 3 months.
It hasn’t been labeled `[Pri] BLOCKER`, `[Pri] High`, etc.

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.

dmsnell · 2023-12-14T07:59:23Z

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.

thingalon

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?

thingalon · 2023-12-15T01:03:00Z

projects/packages/image-cdn/src/class-image-cdn.php

+			return null;
+		}
+
+		$search_pattern = sprintf( '#(?:^|[ \t\f\r\n])%s(?P<value>[^ \t\f\r\n]+)#', preg_quote( $prefix, null ) );


Style question: Is there a difference between [\s] and [ \t\f\r\n]? If not, would it be more readable using \s?

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.

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.

thingalon · 2023-12-15T03:28:55Z

projects/packages/image-cdn/src/class-image-cdn.php

-				 * @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?


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.

thingalon · 2023-12-15T03:52:50Z

projects/packages/image-cdn/src/class-image-cdn.php

+				$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 );
+				}


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?

good call, why do more work than we need to?

) 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>

dmsnell · 2023-12-23T02:50:15Z

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 $tag parameter in the jetpack_photon_skip_image filter.

) 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>

haqadn

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>

dmsnell · 2024-02-02T18:29:06Z

thanks @haqadn. sorry all I've been holding out on this so I can try and test it before merging. too much to do!

github-actions · 2024-05-05T00:49:30Z

This PR has been marked as stale. This happened because:

It has been inactive for the past 3 months.
It hasn’t been labeled `[Pri] BLOCKER`, `[Pri] High`, etc.

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.

github-actions · 2024-06-17T00:49:26Z

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.

github-actions bot added [Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins [Package] Image CDN [Status] In Progress labels Aug 26, 2023

dmsnell force-pushed the image-cdn/rely-on-html-api branch 6 times, most recently from ba4718b to 564599c Compare August 26, 2023 01:27

github-actions bot added the [Tests] Includes Tests label Aug 28, 2023

dmsnell force-pushed the image-cdn/rely-on-html-api branch from b318fbb to dc3a6b1 Compare August 28, 2023 22:42

github-actions bot added the Docs label Aug 29, 2023

dmsnell force-pushed the image-cdn/rely-on-html-api branch 2 times, most recently from 6b724e7 to 583e9ca Compare August 29, 2023 02:28

jeherve mentioned this pull request Aug 29, 2023

WordPress 6.2 compatibility #27795

Closed

16 tasks

jeherve reviewed Aug 29, 2023

View reviewed changes

projects/packages/image-cdn/src/class-image-cdn.php Outdated Show resolved Hide resolved

projects/packages/image-cdn/CHANGELOG.md Outdated Show resolved Hide resolved

dmsnell force-pushed the image-cdn/rely-on-html-api branch from 484deb0 to ffab63f Compare August 29, 2023 16:09

dmsnell changed the title ~~WIP: Refactor ImageCDN parsing to rely on HTML API instead of RegExps~~ Refactor ImageCDN parsing to rely on HTML API instead of RegExps Aug 29, 2023

dmsnell force-pushed the image-cdn/rely-on-html-api branch from e51843b to 5e23682 Compare September 1, 2023 20:47

dmsnell marked this pull request as ready for review September 4, 2023 20:54

jeherve reviewed Sep 6, 2023

View reviewed changes

haqadn previously approved these changes Sep 12, 2023

View reviewed changes

kraftbj reviewed Sep 14, 2023

View reviewed changes

github-actions bot added the [Status] Stale label Dec 14, 2023

dmsnell dismissed haqadn’s stale review via 40cd179 December 14, 2023 08:13

dmsnell force-pushed the image-cdn/rely-on-html-api branch from 5e23682 to 40cd179 Compare December 14, 2023 08:13

github-actions bot removed the [Status] Stale label Dec 15, 2023

thingalon previously approved these changes Dec 15, 2023

View reviewed changes

dmsnell dismissed thingalon’s stale review via 04841f7 December 23, 2023 02:45

dmsnell force-pushed the image-cdn/rely-on-html-api branch from 04841f7 to d01dfa5 Compare December 23, 2023 02:49

dmsnell force-pushed the image-cdn/rely-on-html-api branch from d01dfa5 to 272668d Compare December 23, 2023 06:07

haqadn previously approved these changes Jan 8, 2024

View reviewed changes

dmsnell dismissed haqadn’s stale review via ab212e3 February 2, 2024 18:28

dmsnell force-pushed the image-cdn/rely-on-html-api branch from 1842583 to ab212e3 Compare February 2, 2024 18:28

github-actions bot added the [Status] Stale label May 5, 2024

github-actions bot closed this Jun 17, 2024

github-actions bot deleted the image-cdn/rely-on-html-api branch June 17, 2024 00:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ImageCDN parsing to rely on HTML API instead of RegExps #32700

Refactor ImageCDN parsing to rely on HTML API instead of RegExps #32700

dmsnell commented Aug 26, 2023 •

edited

Loading

github-actions bot commented Aug 26, 2023 •

edited

Loading

github-actions bot commented Aug 26, 2023 •

edited

Loading

jeherve left a comment

oskosk commented Aug 29, 2023

dmsnell commented Sep 5, 2023

dmsnell commented Sep 5, 2023

jeherve left a comment

jeherve Sep 6, 2023

jeherve Sep 6, 2023

haqadn left a comment

dmsnell commented Sep 13, 2023

kraftbj Sep 14, 2023

kraftbj commented Sep 14, 2023

github-actions bot commented Dec 14, 2023

dmsnell commented Dec 14, 2023

thingalon left a comment

thingalon Dec 15, 2023

dmsnell Dec 23, 2023

thingalon Jan 8, 2024

thingalon Dec 15, 2023

thingalon Dec 15, 2023 •

edited

Loading

dmsnell Dec 23, 2023

dmsnell commented Dec 23, 2023

haqadn left a comment

dmsnell commented Feb 2, 2024

github-actions bot commented May 5, 2024

github-actions bot commented Jun 17, 2024

Refactor ImageCDN parsing to rely on HTML API instead of RegExps #32700

Refactor ImageCDN parsing to rely on HTML API instead of RegExps #32700

Conversation

dmsnell commented Aug 26, 2023 • edited Loading

Status

Proposed changes:

Other information:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

github-actions bot commented Aug 26, 2023 • edited Loading

github-actions bot commented Aug 26, 2023 • edited Loading

jeherve left a comment

Choose a reason for hiding this comment

oskosk commented Aug 29, 2023

dmsnell commented Sep 5, 2023

dmsnell commented Sep 5, 2023

jeherve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haqadn left a comment

Choose a reason for hiding this comment

dmsnell commented Sep 13, 2023

Choose a reason for hiding this comment

kraftbj commented Sep 14, 2023

github-actions bot commented Dec 14, 2023

dmsnell commented Dec 14, 2023

thingalon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thingalon Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell commented Dec 23, 2023

haqadn left a comment

Choose a reason for hiding this comment

dmsnell commented Feb 2, 2024

github-actions bot commented May 5, 2024

github-actions bot commented Jun 17, 2024

dmsnell commented Aug 26, 2023 •

edited

Loading

github-actions bot commented Aug 26, 2023 •

edited

Loading

github-actions bot commented Aug 26, 2023 •

edited

Loading

thingalon Dec 15, 2023 •

edited

Loading