Post Media: move Meta_Extractor class to jetpack-post-media package#47205
Post Media: move Meta_Extractor class to jetpack-post-media package#47205
Conversation
github-actions
bot
added
[Package] Post Media
[Plugin] Jetpack
|
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 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Social plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Mu Wpcom plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Classic Theme helper plugin plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
There was a problem hiding this comment.
Pull request overview
This PR migrates the post media metadata extraction logic out of the Jetpack plugin into the automattic/jetpack-post-media package, while keeping backward compatibility via a deprecated wrapper class in the plugin.
Changes:
- Adds
Automattic\Jetpack\Post_Media\Meta_Extractorto thejetpack-post-mediapackage and ports the corresponding test suite to WorDBless. - Deprecates
Jetpack_Media_Meta_Extractorand forwards calls to the new namespaced implementation; updatesJetpack_Media_Summaryto use the new class directly. - Adds
automattic/jetpack-post-mediaas a Jetpack plugin dependency and updates the Phan baseline accordingly.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/plugins/jetpack/composer.json | Adds automattic/jetpack-post-media as a dependency. |
| projects/plugins/jetpack/composer.lock | Locks the newly added package dependency. |
| projects/plugins/jetpack/changelog/move-media-meta-extractor | Plugin changelog entry for the deprecation. |
| projects/plugins/jetpack/_inc/lib/class.media-summary.php | Switches production usage to the namespaced Meta_Extractor. |
| projects/plugins/jetpack/_inc/lib/class.media-extractor.php | Deprecates legacy class and forwards to the package implementation. |
| projects/plugins/jetpack/.phan/baseline.php | Removes now-unneeded baseline entries for the simplified legacy file. |
| projects/packages/post-media/src/class-meta-extractor.php | Introduces the package Meta_Extractor implementation. |
| projects/packages/post-media/tests/php/bootstrap.php | Initializes the shared WP/WorDBless test environment. |
| projects/packages/post-media/tests/php/Meta_Extractor_Test.php | Ports/exercises extractor behavior in package tests. |
| projects/packages/post-media/changelog/move-media-meta-extractor | Package changelog entry for the new class. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code Coverage SummaryCannot generate coverage summary while tests are failing. 🤐 Please fix the tests, or re-run the Code coverage job if it was something being flaky. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
projects/plugins/jetpack/_inc/lib/class.media-extractor.php:73
- The
@paramtype for$extract_alt_textis documented asstring, but the function signature and usage treat it as a boolean. Update the docblock type tobool/booleanto match actual behavior (helps static analysis and IDE hints).
* @param string $content HTML content.
* @param array $image_list Array of already found images.
* @param string $extract_alt_text Whether or not to extract the alt text.
*
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static function extract_images_from_content( $content, $image_list, $extract_alt_text = false ) { | ||
| $image_list = self::get_images_from_html( $content, $image_list, $extract_alt_text ); | ||
| return self::build_image_struct( $image_list, array() ); | ||
| _deprecated_function( __METHOD__, 'jetpack-$$next-version$$', 'Automattic\Jetpack\Post_Media\Meta_Extractor::extract_images_from_content' ); | ||
| return Meta_Extractor::extract_images_from_content( $content, $image_list, $extract_alt_text ); | ||
| } |
There was a problem hiding this comment.
The parameter type for $extract_alt_text is documented as string but should be boolean or bool to match the actual parameter type and usage. This appears to be a typo introduced during the deprecation.
| $srcs = wp_list_pluck( $from_gallery, 'src' ); | ||
| $image_list = array_merge( $image_list, $srcs ); | ||
| } | ||
| ++$image_booleans['gallery']; // @todo This count isn't correct, will only every count 1 |
There was a problem hiding this comment.
The comment contains a typo: "will only every count 1" should be "will only ever count 1". This typo was carried over from the original code and should be fixed.
| ++$image_booleans['gallery']; // @todo This count isn't correct, will only every count 1 | |
| ++$image_booleans['gallery']; // @todo This count isn't correct, will only ever count 1 |
github-actions
bot
added
the
[Plugin] Social
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 24 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
projects/packages/post-media/tests/php/Meta_Extractor_Test.php:478
- The expected image order in the test has been changed from image1-then-image2 to image2-then-image1. While the code may still function correctly (since the extracted images are deduplicated using array_unique), this change in ordering could indicate a behavioral difference between the old
Jetpack_Media_Meta_Extractorand the newMeta_Extractor.
Please verify that this order change is intentional and doesn't break any existing code that may depend on a specific image order. If the order doesn't matter semantically, consider documenting this in the PR description or adding a comment explaining why the order changed.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @deprecated $$next-version$$ | ||
| * | ||
| * @param string $content HTML content. | ||
| * @param array $image_list Array of already found images. |
There was a problem hiding this comment.
The parameter type for $extract_alt_text is documented as string but should be boolean or bool. The parameter is actually a boolean flag indicating whether to extract alt text, as can be seen from the method signature (default value is false) and the corresponding parameter in the new Meta_Extractor class.
Replace Jetpack_Media_Meta_Extractor usage with the new namespaced Automattic\Jetpack\Post_Media\Meta_Extractor class.
Required for the new Meta_Extractor class that replaces Jetpack_Media_Meta_Extractor.
- Add @Covers annotation to match CoversClass attribute - Fix equals sign alignment in create_attachment() and get_images_from_html() - Convert inline associative array to multi-line in add_test_post()
Replace the dynamic jetpack_shortcode_get_{$shortcode}_id() function
lookups and {Name}Shortcode class method calls with the new
Automattic\Jetpack\Shortcodes class from the post-media package.
- Replace \Jetpack_PostImages calls with Images:: (same namespace) - Remove @phan-suppress annotations no longer needed - Add `use` for Shortcodes class, simplify references - Remove placeholder class-post-media.php and PACKAGE_VERSION - Remove stale comment in class.media-summary.php - Remove old Jetpack_MediaExtractor_Test (now in post-media package)
Clean up class references to use imports instead of inline fully-qualified names. Update keeper_shortcodes docblock to reference the Shortcodes class.
Move the Shortcodes class from Automattic\Jetpack to Automattic\Jetpack\Post_Media for consistency with the Images and Meta_Extractor classes in the same package.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
The forwarding call to Meta_Extractor::reduce_extracted_images() would cause a fatal error since the method is protected and Jetpack_Media_Meta_Extractor does not extend Meta_Extractor. Inline the implementation instead.
Fixes CM-546, Fixes CM-543 These functions have been extracted into the automattic/jetpack-post-media package as the Shortcodes class under the Automattic\Jetpack\Post_Media namespace. The old functions (jetpack_shortcode_get_*_id, jetpack_get_youtube_id, jetpack_youtube_sanitize_url) are now deprecated shims that delegate to their package counterparts. Callers continue to work through the shims; they will emit deprecation notices when WP_DEBUG is enabled, guiding developers to the new API.
The move of media extraction code from the Jetpack plugin to the post-media package introduced several test failures: - Phan flagged a redundant assignment (`$x = $x - Y` → `$x -= Y`) and two potentially undefined array keys in get_images_from_html (src_width / src_height may not exist on the extracted image). - Meta_Extractor_Test ran under WorDBless but lacked the environment setup that the full WordPress test harness provides: no admin user (so wp_kses_post sanitized content on insert), no upload-dir normalization, no image_downsize shim for virtual files, and no dummy shortcode registrations for youtube/vimeo/ted/wpvideo. Add a set_up() following the same pattern already used in ImagesTest. - Functions_Compat_Test and Jetpack_Shortcodes_Vimeo_Test now call deprecated wrapper functions (jetpack_youtube_sanitize_url, jetpack_get_youtube_id, jetpack_shortcode_get_vimeo_id) which trigger _deprecated_function notices caught by WP_UnitTestCase. Declare the expected deprecations with setExpectedDeprecated().
jeherve
force-pushed
the
jeherve/move-media-extractor
branch
from
ce71443 to
2e69122
Compare
github-actions
bot
added
[Plugin] Classic Theme Helper Plugin
[Plugin] mu wpcom
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 32 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $ret_images = array(); | ||
| foreach ( $images as $image ) { | ||
| if ( empty( $image['src'] ) ) { | ||
| continue; | ||
| } | ||
| $ret_image = array( | ||
| 'url' => $image['src'], | ||
| ); | ||
| if ( ! empty( $image['src_height'] ) || ! empty( $image['src_width'] ) ) { | ||
| $ret_image['src_width'] = $image['src_width'] ?? ''; | ||
| $ret_image['src_height'] = $image['src_height'] ?? ''; | ||
| } | ||
| if ( ! empty( $image['alt_text'] ) ) { | ||
| $ret_image['alt_text'] = $image['alt_text']; | ||
| } else { | ||
| $ret_image = $image['src']; | ||
| } | ||
| $ret_images[] = $ret_image; | ||
| } | ||
| return $ret_images; |
There was a problem hiding this comment.
The reduce_extracted_images method contains the full implementation instead of just forwarding to the new Meta_Extractor class. For consistency with the other deprecated methods (extract, extract_from_content, etc.), this should only call _deprecated_function() and forward to Meta_Extractor::reduce_extracted_images(). The current implementation defeats the purpose of moving the logic to the package and creates code duplication.
| $ret_images = array(); | |
| foreach ( $images as $image ) { | |
| if ( empty( $image['src'] ) ) { | |
| continue; | |
| } | |
| $ret_image = array( | |
| 'url' => $image['src'], | |
| ); | |
| if ( ! empty( $image['src_height'] ) || ! empty( $image['src_width'] ) ) { | |
| $ret_image['src_width'] = $image['src_width'] ?? ''; | |
| $ret_image['src_height'] = $image['src_height'] ?? ''; | |
| } | |
| if ( ! empty( $image['alt_text'] ) ) { | |
| $ret_image['alt_text'] = $image['alt_text']; | |
| } else { | |
| $ret_image = $image['src']; | |
| } | |
| $ret_images[] = $ret_image; | |
| } | |
| return $ret_images; | |
| return Meta_Extractor::reduce_extracted_images( $images ); |
Internal callers (youtube.php, vimeo.php, archiveorg.php, archiveorg-book.php, class.media-summary.php, enhanced-open-graph.php) now call Shortcodes:: methods directly instead of going through the deprecated global function wrappers. This fixes unexpected deprecation notices in Jetpack_Shortcodes_Youtube_Test (15 failures across all PHP versions) and resolves phan PhanDeprecatedFunction errors. Also fixes a docblock type mismatch in class.media-extractor.php (string → bool) and adds phan baseline entries for functions.compat.php and Functions_Compat_Test.php which intentionally call deprecated functions.
Fixes CM-76
Fixes CM-546
Fixes CM-543
Proposed changes:
Jetpack_Media_Meta_Extractorclass logic intoAutomattic\Jetpack\Post_Media\Meta_Extractorin thejetpack-post-mediapackage.Jetpack_Media_Meta_Extractorclass, replacing method bodies with_deprecated_function()notices and forwarding calls to the new class.Jetpack_Media_Summary(the only production caller) to use the new namespaced class directly.WP_UnitTestCaseto WorDBless.Jetpack_MediaExtractor_Testfrom the Jetpack plugin (tests now live in the package).automattic/jetpack-post-mediaas a dependency of the Jetpack plugin.Imagesclass (from Post Media: Add Images class (copy of Jetpack_PostImages) #47208) instead of\Jetpack_PostImages— no more external dependency on the Jetpack plugin for image extraction.Shortcodesclass (from Post Media: Add Shortcodes class for shortcode ID extraction #47200) instead ofjetpack_shortcode_get_{$shortcode}_id()functions for shortcode ID extraction.Shortcodesclass fromAutomattic\JetpacktoAutomattic\Jetpack\Post_Medianamespace for consistency withImagesandMeta_Extractor.class-post-media.phpand unusedPACKAGE_VERSIONconstant.useimport statements (WP_Post,WP_Error).Other information:
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
Meta_Extractorclass atprojects/packages/post-media/src/class-meta-extractor.phpcontains all methods from the originalJetpack_Media_Meta_Extractor.projects/plugins/jetpack/_inc/lib/class.media-extractor.phpforwards all calls to the new class with deprecation notices.projects/plugins/jetpack/_inc/lib/class.media-summary.phpusesMeta_Extractorfrom the post-media package.Meta_ExtractorusesImages::(same namespace) instead of\Jetpack_PostImages::.Meta_ExtractorusesShortcodes::get_{shortcode}_id()instead of the oldjetpack_shortcode_get_*function lookups.Shortcodesclass is now in theAutomattic\Jetpack\Post_Medianamespace, consistent withImagesandMeta_Extractor.