some cleanups and a simpler image handling - #46
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved PHP 7.4 compatibility, image collection, test setup/CI, and release metadata issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This pull request refactors Open Graph image handling, raises the minimum WordPress version to 6.2, and adds PHPUnit tooling and CI coverage.
Changes:
- Consolidates image discovery and updates Twitter card behavior.
- Replaces legacy image helpers with the new image-source filter.
- Adds tests, local tooling, CI configuration, and compatibility metadata.
File summaries
| File | Summary |
|---|---|
tests/phpunit/tests/class-test-opengraph.php |
Metadata regression tests |
tests/phpunit/tests/class-test-opengraph-images.php |
Image discovery and limit tests |
tests/phpunit/includes/class-opengraph-testcase.php |
Shared PHPUnit helpers |
tests/phpunit/bootstrap.php |
PHPUnit bootstrap |
readme.md |
WordPress requirement metadata |
phpunit.xml.dist |
PHPUnit configuration |
phpcs.xml |
Coding-standard configuration |
package.json |
Local wp-env scripts |
opengraph.php |
Core metadata and image-handling refactor |
composer.json |
PHPUnit dependencies and test scripts |
bin/install-wp-tests.sh |
WordPress test-suite installer |
.wp-env.json |
Local test environment configuration |
.gitignore |
Development artifact exclusions |
.github/workflows/phpunit.yml |
PHPUnit CI workflow |
.github/workflows/phpcs.yml |
PHPCS CI workflow |
.gitattributes |
Distribution archive exclusions |
Review details
Suppressed comments (5)
.github/workflows/phpunit.yml:15
- The test setup now depends on
bin/install-wp-tests.sh, but neither path list triggers this workflow when that script changes. A later fix to the installer could therefore merge without running the tests; include the installer in both the push and pull-request path filters.
- '**/*.php'
composer.json:33
- The new
composer testcommand installs the test suite with database hosttest-dbandSKIP_DB_CREATE=true, but notest-dbservice or pre-created database is defined anywhere in this repository; the wp-env configuration also does not provide that host. A clean local checkout therefore cannot runcomposer testas advertised—either provision that service/database or use the same connection/setup as the CI command.
"bin/install-wp-tests.sh opengraph-test root opengraph-test test-db latest true",
opengraph.php:759
- This second
str_starts_with()call is also unavailable on the declared PHP 7.4 floor. Sites that enableOPENGRAPH_STRICT_MODEwill hit an undefined-function fatal while rendering every metadata request; replace these checks with a PHP 7.4-compatible implementation.
} elseif ( str_starts_with( $key, 'twitter:' ) || str_starts_with( $key, 'fediverse:' ) ) {
opengraph.php:397
- This matches the substring anywhere in the class attribute, so a class such as
not-wp-image-123is treated as attachment 123 even though it is not awp-image-{id}class. Use class-token boundaries so unrelated class names cannot select the wrong attachment.
if ( is_string( $class ) && preg_match( '/wp-image-([0-9]+)/i', $class, $matches ) ) {
opengraph.php:371
get_post_field()defaults to thedisplaycontext, so this appliesthe_contentfilters instead of scanning the raw stored markup. That can reintroduce content-filter side effects (and undermines the fix for the #39 failure); request therawcontext explicitly here.
$tags = new WP_HTML_Tag_Processor( get_post_field( 'post_content', $post_id ) );
- Files reviewed: 14/17 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Critical CI and password-protection issues remain, along with metadata and release-version concerns.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
Previously missed (1) — in code that hasn't changed since the last review.
opengraph.php:822
$metadatais the current metadata, and this callback is an extension point. The previous implementation appended term names toarticle:tag, preserving values supplied by earlieropengraph_metadatacallbacks; replacing the key here now silently drops those custom tags. Merge the plucked terms into the existing value (with an empty-array fallback) instead.
.github/workflows/phpunit.yml:68
- This repeats the invalid dot access to the hyphenated
php-versionsmatrix key, so the environment passed to PHPUnit is invalid. Use bracket notation or rename the matrix key.
PHP_VERSION: ${{ matrix.php-versions }}
opengraph.php:169
- Removing the final max-images filter means a normal
opengraph_imagecallback that runs after the priority-5 default (for example, at priority 10) can append more thanopengraph_max_images(). The default callback only caps the list at its own execution point, so the documented limit is no longer enforced; retain a final cap after all image filters or enforce it when metadata is finalized.
add_filter( 'opengraph_image', 'opengraph_fallback_image', 35 );
opengraph.php:12
- The new compatibility metadata leaves the plugin
Versionand readmeStable tagat 2.0.2, even though the PR removes public functions, raises the minimum WordPress version, and describes a minor version bump. A release built from this change would still identify itself as the old 2.0.2 release; bump the plugin header and readme version together before publishing.
* Requires at least: 6.2
* Requires PHP: 7.4
readme.md:5
- This PR raises the minimum WordPress version and removes public functions, but the plugin header and readme stable tag still identify the release as 2.0.2. That would ship the breaking change under the old patch version and does not match the requested minor bump; update the release version consistently in the header, stable tag, and changelog.
- Requires at least: 6.2
- Files reviewed: 14/17 changed files
- Comments generated: 3
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved moderate findings affect image limits, PHP 7.4 compatibility, and release version metadata.
Review details
Suppressed comments (4)
opengraph.php:169
- The final max-image callback was removed, but
opengraph_default_image()only slices the values present at priority 5. A plugin that appends images at a later priority (for example 10) can now makeog:imageexceedOPENGRAPH_MAX_IMAGES;opengraph_fallback_image()returns early and does not cap it. Keep a final cap callback after third-party filters and add a regression test for a later-priority filter.
add_filter( 'opengraph_image', 'opengraph_fallback_image', 35 );
opengraph.php:406
- PHP 7.4 is still supported, but the removed polyfill leaves
str_starts_with()undefined on this URL-fallback path. The newtest_content_image_by_url()exercises this line, so the PHP 7.4 CI job will fatal; use a PHP 7.4-compatible prefix check here (or restore the polyfill), and likewise fix the strict-mode call below.
if ( ! is_string( $src ) || ! str_starts_with( $src, wp_get_upload_dir()['baseurl'] ) ) {
opengraph.php:11
- The plugin header and
readme.mdstill identify this as version 2.0.2, although this change removes public functions, adds a replacement filter, and raises the minimum WordPress version; the PR description calls for a minor bump. Update the plugin version and Stable tag so this release is distinguishable from 2.0.2.
* Requires at least: 6.2
readme.md:5
- The PR description calls for a minor version bump, but the plugin header and the readme Stable tag still identify this code as 2.0.2. Shipping the removed public callbacks and new minimum WordPress requirement under the existing release metadata would leave WordPress treating it as the already-published 2.0.2 release; please bump the header, Stable tag, and changelog together, or clarify that versioning is intentionally deferred.
- Requires at least: 6.2
- Files reviewed: 14/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
I went through the whole plugin and cleaned up a lot of duplicated code. The biggest change is the image handling.
Images: Instead of four filter callbacks that each scan the post from a different angle, there is now one default callback that walks a list of sources (thumbnail, content, attached images) and stops as soon as the list is full. The content source does one pass over the raw HTML and reads the
wp-image-{id}class, the same way core does. That also catches gallery, media & text and nested images, which the old block parser missed. Only images without the class go through the (slow) URL lookup.Twitter card: The "more than one image" rule was a leftover from the old
gallerycard type that Twitter removed in 2015. The card is nowsummary_large_imagewhenever a singular post has anog:image,summaryotherwise. The property filters get the metadata collected so far as a second argument, so the Twitter card does not run the whole image chain a second time.Tests: phpunit setup based on the ActivityPub plugin, with tests for all image variants (blocks, classic content, URL fallback, limits, fallback images) and the rest of the metadata. One of the tests found an old bug: merging into an existing
prefixattribute dropped the space.Minimum WordPress version is now 6.2 (the readme still said 2.3, which was not true for years). That is the version with
WP_HTML_Tag_Processor, so the class check and thestr_starts_withpolyfill are gone. PHP stays at 7.4.Some public functions are gone:
opengraph_block_image,opengraph_parsed_image,opengraph_attached_image,opengraph_ensure_max_imageandopengraph_site_supports_blocks(and its filter). The newopengraph_image_sourcesfilter is the way to add or remove a source now. I think this should be a minor version bump, not a patch release.Two things I am not sure about: posts without own images now get a large Twitter card with the site icon, and the
og:imagelist is no longer capped for images that third party filters add.This also supersedes #39: the fatal error with The Events Calendar came from the
get_the_content()call in the old block collector, which is gone. There is a test for it now.