Make WordPress Core

Opened 6 months ago

Closed 6 months ago

Last modified 2 months ago

#59723 closed defect (bug) (fixed)

Pattern file containing PHP constant from theme causes fatal error on update to WordPress 6.4

Reported by: fabiankaegy's profile fabiankaegy Owned by: joemcgill's profile joemcgill
Milestone: 6.4 Priority: high
Severity: normal Version: 6.4
Component: Upgrade/Install Keywords: has-patch has-testing-info commit dev-reviewed fixed-major
Focuses: Cc:

Description

I have a theme that sets up some PHP constants in its functions.php file. These constants for example reference the dist directory of the built assets.

<?php
define( 'NAMESPACE_THEME_TEMPLATE_URL', get_template_directory_uri() );
define( 'NAMESPACE_THEME_DIST_URL', NAMESPACE_THEME_TEMPLATE_URL . '/dist/' );

This theme then also contains some patterns in the patterns directory.

Some of these patterns reference one of the globals defined in the functions.php file to load an image from the dist folder.

<?php
<?php
/**
 * Title: Hero - Default
 * Slug: namespace/hero-default
 * Categories: header
 * Viewport Width: 500
 */

?>
<!-- wp:cover {"url":"<?php echo esc_url( NAMESPACE_THEME_DIST_URL . 'images/placeholder/pattern--no-text.jpg' ); ?>","dimRatio":70,"minHeight":70,"minHeightUnit":"vh","contentPosition":"bottom left","isDark":false,"align":"full"} -->

This works great when just using the site. But when installing WordPress 6.4 RC1 or RC2 via CLI the update appears to run through but when I try to log into the Admin I get a blank white screen on the URL: /wp-admin/upgrade.php

When I look at the debug.log file I get a fatal error saying that the constant used in the pattern file isn't defined.

So it appears the patterns are getting loaded before the theme is loaded.

Change History (32)

This ticket was mentioned in Slack in #core by fabiankaegy. View the logs.


6 months ago

#2 @fabiankaegy
6 months ago

I have created a sample repository containing a minimal theme setup to reproduce the issue: https://github.com/fabiankaegy/wp-core-59723-sample-theme

#3 @joemcgill
6 months ago

  • Milestone changed from Awaiting Review to 6.4
  • Owner set to joemcgill
  • Priority changed from normal to high
  • Status changed from new to reviewing

#4 @rajinsharwar
6 months ago

  • Milestone changed from 6.4 to Awaiting Review
  • Priority changed from high to normal

I can replicate the error. If we use the theme shared, we get the below error.

PHP Fatal error: Uncaught Error: Undefined constant "NAMESPACE_IMAGE_URL" in ..\public\wp-content\themes\wp-core-59723-sample-theme-main\patterns\sample.php:9

#5 @rajinsharwar
6 months ago

Also, I am able to replicate it, when I am trying to downgrade from 6.4 to 6.3.2. It seems that this is calling the patterns before the theme itself, whenever the DB update is required after an update.

Specifically, on this page: http://my-site.localhost/wp-admin/upgrade.php?_wp_http_referer=%2Fwp-admin%2F

#6 @joemcgill
6 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 6.4
  • Priority changed from normal to high
  • Status changed from reviewing to accepted

Thanks, @fabiankaegy. It looks like what is happening here is that the first time wp_get_active_and_valid_themes() is run one the upgrade page, via this line, the function returns an empty array, which means the functions file isn't included.

Patterns are registered on init and do not include a check for whether the functions file has already been loaded, but we should probably add an extra check here, so patterns aren't loaded whenever wp_installing() returns true.

This ticket was mentioned in PR #5568 on WordPress/wordpress-develop by @joemcgill.


6 months ago
#7

  • Keywords has-patch added; needs-patch removed

This fixes a bug on upgrade screens where constants defined in a theme's functions.php file are not defined before the patterns are loaded, leading to potential exceptions.

Trac ticket: https://core.trac.wordpress.org/ticket/59723

#8 follow-up: @rajinsharwar
6 months ago

Just a side query, why are we loading the patterns on the init instead of after_theme_setup? 🧐
The patterns are a part of that theme after all.

#9 @joemcgill
6 months ago

After more investigation, it looks like this issue existed prior to the 6.4 release cycle, and was previously reported in #59020. It's possible this issue extends all the way back to WP version 6.0, when this was first introduced in [53152].

I'll close that ticket as a duplicate if we decide to move forward with this fix.

@rajinsharwar, to answer your question:

Just a side query, why are we loading the patterns on the init instead of after_theme_setup?

It's a good question...I'm not sure what the original rationale was for choosing that hook. However, it this case it wouldn't matter because the after_theme_setup hook fires before init in wp-settings.php and the functions.php file would still not be loaded in this case, leading to the same error.

#10 follow-up: @pbiron
6 months ago

Steps to reproduce

  1. start w/ a fresh site running 6.3.2
  2. install/activate the sample theme in comment:2
  3. update to 6.4-RC2 via WP_CLI, wp core update --version=6.4-RC
  4. check the PHP error logs, and notice PHP Fatal error: Uncaught Error: Undefined constant "NAMESPACE_IMAGE_URL" in path-to-site/wp-content/themes/wp-core-59723-sample-theme-main/patterns/sample.php:9

Steps to test the PR

  1. roll the site back to 6.3.2, wp core update --version=6.3.2 --force
    1. note: another error will be logged, you can ignore it the purposes of this testing
  2. update wp-includes/block-patterns.php in the WP_CLI ZIP file in ~/.wp-cli/cache/core/wordpress-6.4-RC2-en_US.zip so that the PR has been applied (i.e., add the early return from _register_theme_block_patterns() if wp_is_installing() is true)
  3. clear your PHP error log file
  4. update to 6.4-RC2, wp core update --version=6.4-RC2
  5. check the PHP error logs and notice no error was logged

This ticket was mentioned in Slack in #core by pbiron. View the logs.


6 months ago

#12 in reply to: ↑ 8 @pbiron
6 months ago

Replying to rajinsharwar:

Just a side query, why are we loading the patterns on the init instead of after_theme_setup? 🧐
The patterns are a part of that theme after all.

I have the same question. Registering a theme's pattens on after_theme_setup (w/ a priority of 20) seems to make much more sense. Maybe there's some underlying reason the editor needs them on init?

#13 @huzaifaalmesbah
6 months ago

I can reproduce error. following comment:10

PHP Fatal error:  Uncaught Error: Undefined constant "NAMESPACE_IMAGE_URL" in /Users/huzaifa/Project/wp/test/wp-content/themes/wp-core-59723-sample-theme/patterns/sample.php:9

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


6 months ago

#15 @hellofromTonya
6 months ago

  • Keywords changes-requested needs-testing has-testing-info added

Updating the keywords:

Patch: https://github.com/WordPress/wordpress-develop/pull/5568

  • needs-testing is testing the patch, once it's ready.
  • has-testing-info: found in comment:10
  • changes-requested: discussion is happening in the patch of possible changes.

Alerted contributors in the Make/Core core channel with a call for testing and review.

Cross-sharing here:

👉 Should it go into 6.4 RC3?
IMO yes, fatals should be fixed. But there must be high confidence that the patch does indeed resolve the fatal without side effects.

"high confidence" comes from code reviews and test reports. The more test reports, the higher the confidence.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


6 months ago

This ticket was mentioned in Slack in #core-upgrade-install by pbiron. View the logs.


6 months ago

This ticket was mentioned in Slack in #core by pbiron. View the logs.


6 months ago

@pbiron commented on PR #5568:


6 months ago
#19

Other than the minor changes requested in the reviews I just posted, the PR and tests LGTM.

@joemcgill commented on PR #5568:


6 months ago
#20

Thanks @pbiron. These are great suggestions, which I've applied in d8f3501.

#21 @joemcgill
6 months ago

@fabiankaegy if you have a chance, can you please test out the latest PR and see if it resolves the issue for you?

@pbiron commented on PR #5568:


6 months ago
#22

I've confirmed that this fixes the bug and that the tests are testing what they intend to test and pass w/ TT3.

For future consideration: we might consider adding a test theme whose patterns assume that functions.php has been loaded and use that theme in the tests.

@pbiron commented on PR #5568:


6 months ago
#23

The PHPUnit Tests / PHP 7.4 / PHP 7.4 / MariaDB 10.11 multisite (pull_request) test run errored out. Looks unrelated to this PR.

@joemcgill Are you able to restart the unit tests?

#24 in reply to: ↑ 10 @fabiankaegy
6 months ago

Replying to pbiron:

Steps to reproduce

  1. start w/ a fresh site running 6.3.2
  2. install/activate the sample theme in comment:2
  3. update to 6.4-RC2 via WP_CLI, wp core update --version=6.4-RC
  4. check the PHP error logs, and notice PHP Fatal error: Uncaught Error: Undefined constant "NAMESPACE_IMAGE_URL" in path-to-site/wp-content/themes/wp-core-59723-sample-theme-main/patterns/sample.php:9

Steps to test the PR

  1. roll the site back to 6.3.2, wp core update --version=6.3.2 --force
    1. note: another error will be logged, you can ignore it the purposes of this testing
  2. update wp-includes/block-patterns.php in the WP_CLI ZIP file in ~/.wp-cli/cache/core/wordpress-6.4-RC2-en_US.zip so that the PR has been applied (i.e., add the early return from _register_theme_block_patterns() if wp_is_installing() is true)
  3. clear your PHP error log file
  4. update to 6.4-RC2, wp core update --version=6.4-RC2
  5. check the PHP error logs and notice no error was logged

Following these test steps I am also no longer able to reproduce the issue :)

#25 @joemcgill
6 months ago

  • Keywords changes-requested removed

I've cleaned up the unit tests for this fix to avoid looping through all of the theme patterns. I would like to get a +1 from another committer (cc: @peterwilsoncc or @hellofromTonya) before committing this to trunk, and then will need that commit dev-reviewed in order to include this in the 6.4 branch.

In the meantime, I'd be happy for others to post the results of their testing, so we can demonstrate a high level of confidence (thanks, @pbiron and @fabiankaegy).

#26 @peterwilsoncc
6 months ago

@joemcgill I've approved the PR and am +1 to commit.

#27 @hellofromTonya
6 months ago

  • Keywords commit dev-reviewed added; needs-testing removed

Patch: https://github.com/WordPress/wordpress-develop/pull/5568

  • Multiple approvals on the PR ✅
  • Multiple test reports the PR resolves the issue ✅
  • Confidence is high ✅ (though more test reports are encouraged and welcomed)

Marking the patch for commit and, to expedite, dev-reviewed for backport to 6.4 branch.

#28 @joemcgill
6 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 57021:

Upgrade/Install: Skip registering theme block patterns during the upgrade process.

This fixes a bug during the database upgrade process where a theme's functions.php file may not be loaded, leading to potential exceptions if the theme's pattern files use symbols (classes, functions, constants, etc.) that are declared only when the functions.php file is loaded. To do so, a check for wp_get_active_and_valid_themes() is added early to _register_theme_block_patterns(), which returns early if no active or valid themes are returned.

Props fabiankaegy, rajinsharwar, pbiron, huzaifaalmesbah, hellofromTonya, peterwilsoncc, joemcgill.
Fixes #59723.

#29 @joemcgill
6 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for the back porting to the 6.4 branch once tests have passed, given @hellofromTonya has already marked this as dev-reviewed.

Last edited 6 months ago by joemcgill (previous) (diff)

#30 @joemcgill
6 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 57023:

Upgrade/Install: Skip registering theme block patterns during the upgrade process.

This fixes a bug during the database upgrade process where a theme's functions.php file may not be loaded, leading to potential exceptions if the theme's pattern files use symbols (classes, functions, constants, etc.) that are declared only when the functions.php file is loaded. To do so, a check for wp_get_active_and_valid_themes() is added early to _register_theme_block_patterns(), which returns early if no active or valid themes are returned.

Props fabiankaegy, rajinsharwar, pbiron, huzaifaalmesbah, hellofromTonya, peterwilsoncc, joemcgill.
Reviewed by hellofromTonya.
Merges [57021] to the 6.4 branch.
Fixes #59723.

This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.


2 months ago

Note: See TracTickets for help on using tickets.