Post Media: Add Shortcodes class for shortcode ID extraction#47200
Post Media: Add Shortcodes class for shortcode ID extraction#47200
Conversation
|
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! |
Code Coverage Summary1 file is newly checked for coverage.
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a new Automattic\Jetpack\Shortcodes utility in the jetpack-post-media package to centralize extraction of media identifiers from shortcode attributes (e.g., YouTube, Vimeo, TED, VideoPress, HTML5 audio/video, Hulu, Archive.org), along with PHPUnit coverage and a changelog entry.
Changes:
- Added
Shortcodesclass with static helper/extractor methods (including YouTube URL normalization). - Added PHPUnit tests covering supported shortcode formats and edge cases.
- Added a post-media package changelog entry documenting the addition.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| projects/packages/post-media/src/class-shortcodes.php | Adds the new Shortcodes class with media ID extraction and YouTube URL normalization logic. |
| projects/packages/post-media/tests/php/Shortcodes_Test.php | Adds unit tests for the new extraction/normalization methods. |
| projects/packages/post-media/changelog/jeherve-shortcode-extractors | Records the new functionality in the package changelog. |
💡 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 3 out of 3 changed files in this pull request and generated 4 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 3 out of 3 changed files in this pull request and generated 3 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 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
52422fe to
131a3f8
Compare
…ode attributes Consolidate scattered shortcode ID extraction functions into a single Shortcodes class in the post-media package. Methods cover YouTube, Vimeo, TED, VideoPress (wpvideo and videopress), HTML5 video, audio, Hulu, and Archive.org (including books). Includes comprehensive tests (69 test cases) ported from existing tests and new coverage for previously untested extraction logic.
131a3f8 to
03b8dbf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
allilevine
left a comment
There was a problem hiding this comment.
CI passes and the extraction looks good ✅
Co-authored-by: Allison Levine <1689238+allilevine@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * https://vimeo.com/album/1234567/video/7654321 | ||
| * https://player.vimeo.com/video/18427511 | ||
| */ | ||
| $pattern = '/(?:https?:\/\/)?vimeo\.com\/(?:groups\/[^\/]+\/videos\/|album\/\d+\/video\/|video\/|channels\/[^\/]+\/videos\/|[^\/]+\/)?([0-9]+)(?:[^\'\"0-9<]|$)/i'; |
There was a problem hiding this comment.
The regex pattern does not match player.vimeo.com URLs, but based on the test case at line 197-200 of the test file and the documentation comment at lines 145, it should. The pattern should be updated to include an optional subdomain match before vimeo.com. Consider changing the pattern to include (?:[^.]+\.)? before vimeo\.com to match domains like player.vimeo.com.
| $pattern = '/(?:https?:\/\/)?vimeo\.com\/(?:groups\/[^\/]+\/videos\/|album\/\d+\/video\/|video\/|channels\/[^\/]+\/videos\/|[^\/]+\/)?([0-9]+)(?:[^\'\"0-9<]|$)/i'; | |
| $pattern = '/(?:https?:\/\/)?(?:[^.]+\.)?vimeo\.com\/(?:groups\/[^\/]+\/videos\/|album\/\d+\/video\/|video\/|channels\/[^\/]+\/videos\/|[^\/]+\/)?([0-9]+)(?:[^\'\"0-9<]|$)/i'; |
| } | ||
|
|
||
| $url = trim( $url, ' "' ); | ||
| $url = trim( $url ); |
There was a problem hiding this comment.
There are two consecutive trim calls where the second one is redundant. Line 55 already trims spaces (among other characters), so the second trim on line 56 has no effect. Consider combining these into a single call: trim($url, ' "') which will trim both spaces and quotes.
| $url = trim( $url ); |
Fixes CM-544
Note
This only adds the functions to the package, it doesn't replace their usage across the codebase. I'll do that in a follow-up PR.
That means we only need to check that the CI checks pass in this PR.
Proposed changes:
Shortcodesclass to thepost-mediapackage that consolidates shortcode ID extraction logic currently scattered across the Jetpack plugin codebase.sanitize_youtube_url()helper that normalizes YouTube URLs (youtu.be, /v/, /shorts/, encoded ampersands, playlists).Other information:
Jetpack product discussion
n/a
Does this pull request change what data or activity we track or use?
No.
Testing instructions: