Skip to content

fix(starter-content): stop generated post creation failing on PHP 8 - #1026

Open
jason10lee wants to merge 1 commit into
mainfrom
fix/starter-content-sideload-by-ref
Open

fix(starter-content): stop generated post creation failing on PHP 8#1026
jason10lee wants to merge 1 commit into
mainfrom
fix/starter-content-sideload-by-ref

Conversation

@jason10lee

Copy link
Copy Markdown
Contributor

All Submissions:

Changes proposed in this Pull Request:

Generated starter content creates its categories and then no posts at all. wp_handle_sideload() takes its first argument by reference, and the call passed an array literal, which PHP 8 rejects outright with Argument #1 ($file) could not be passed by reference. The fatal lands on the first post, so n setup finishes with categories and an empty site. The setup wizard reaches the same call through api_starter_content_post, which is registered on every site.

Two things kept this quiet. The fatal has nothing to do with the network, so it fires whether or not the image host answers, and it presents as one of the image-fetch failures being fixed elsewhere. And the E2E path branches to copy_bundled_image(), added when starter-content images moved off the public internet, so nothing running in CI reaches the call.

The array is not a constant expression, since it calls mime_content_type() and filesize(), so it is a temporary. Passing a temporary by reference was a notice on PHP 7 and the call went through; PHP 8 made it an error. That dates the break to the PHP 8 move rather than to when this code was written.

The fix assigns the array to a variable. Two regression tests come with it, stubbing the HTTP layer so the sideload runs for real against the bundled image instead of being asserted around.

We weighed one alternative: pointing the non-E2E path at the bundled image too, dropping the network dependency entirely. We left it out because it changes what publishers get, one repeated image in place of a varied one, and that is a product decision separate from the fatal.

Clearing pre_http_request in the test teardown takes the WordPress test suite's own handlers with it. The first version of these tests did exactly that and turned 142 later tests into errors and failures, while still passing on its own under --filter. The teardown therefore removes its own filters by name, and the comment in the file says why.

Found while building an environment for unrelated work; no Linear issue.

How to test the changes in this Pull Request:

  1. On a site where NEWSPACK_IS_E2E is not defined, run n setup --yes. Ten starter posts are created, each with a featured image.
  2. Check the PHP error log for that run. It carries no wp_handle_sideload() fatal.
  3. Run the same setup with NEWSPACK_IS_E2E defined. Posts still take the bundled image, unchanged from before.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?

Full newspack-plugin suite green (3899 tests, 11026 assertions), phpcs clean. Reverting the fix turns the new tests into the fatal above, so they fail without it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The change is minimal, directly addresses the PHP 8 fatal, and includes targeted regression tests that exercise the corrected sideload behavior.

Pull request overview

Fixes a PHP 8 fatal in Starter_Content_Generated::download_random_image() that prevented generated starter content posts from being created (and could also affect the setup wizard path), by ensuring the wp_handle_sideload() $file argument is passed as a variable (required because it is passed by reference). Adds focused unit tests that stub the HTTP layer so sideloading executes against a real local image file and verifies both the success and failure behaviors.

Changes:

  • Avoid PHP 8 “cannot be passed by reference” fatal by assigning the sideload $file array to a variable before calling wp_handle_sideload().
  • Add unit coverage for the image download/sideload success case (file ends up in uploads) and the failure case (returns null on request error).
  • Ensure test cleanup removes only this test’s pre_http_request filters (without disrupting the WP test suite’s own handlers).
File summaries
File Description
plugins/newspack-plugin/includes/starter_content/class-starter-content-generated.php Fixes the PHP 8 by-reference argument fatal by passing a $file variable into wp_handle_sideload().
plugins/newspack-plugin/tests/unit-tests/starter-content-generated.php Adds regression tests covering the sideload success path and the “request fails” null-return path, with safe per-test HTTP filter teardown.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@jason10lee
jason10lee marked this pull request as ready for review September 3, 2026 11:32
@jason10lee
jason10lee requested a review from a team as a code owner September 3, 2026 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants