Missing checks for pattern files trigger (invisible) PHP warnings

[afercia]

This morning I pulled latest changes onto my local WordPress trunk environment.
Suddenly, all the admin pages showed an unexpected spacing at the top of the page. That's because the CSS class php-error was added to the body element but no actual error or warning is printed out on the page. See attached screenshot.

Turned out ​this commit on Twenty Twenty-Four which has been included in core renamed the pattern file cta.php to text-centered-cta-subscribe.php

After manually dumping $error_get_last in the src/wp-admin/admin-header.php I was finally able to see what the PHP warning is about:

array(4) {
  ["type"]=>
  int(2)
  ["message"]=>
  string(144) "include(): Failed opening '/var/www/src/wp-content/themes/twentytwentyfour/patterns/cta.php' for inclusion (include_path='.:/usr/local/lib/php')"
  ["file"]=>
  string(43) "/var/www/src/wp-includes/block-patterns.php"
  ["line"]=>
  int(355)
}

Content of the debug.log file:

[11-Oct-2023 09:52:22 UTC] PHP Warning:  include(/var/www/src/wp-content/themes/twentytwentyfour/patterns/cta.php): failed to open stream: No such file or directory in /var/www/src/wp-includes/block-patterns.php on line 355
[11-Oct-2023 09:52:22 UTC] PHP Warning:  include(): Failed opening '/var/www/src/wp-content/themes/twentytwentyfour/patterns/cta.php' for inclusion (include_path='.:/usr/local/lib/php') in /var/www/src/wp-includes/block-patterns.php on line 355

I'm not familiar with this part of the code but it turns out it has to do with the pattern files caching mechanism.

From a quick look, it seems the cache gets cleared when updating a theme from the admin UI. However, it seems it does _not_ get cleared when a theme gets updated via a different flow. For example:

• Users can download a new version of the theme in use from the WordPress thems directory.
• The can just unzip the file and uploaded the new theme files via FTP.
• This is a legitimate scenario and it has been supported since ages.
• If the theme new version renamed some pattern files, the PHP warning will be triggered.
• If the theme new version _added_ new pattern files, I have no idea what would happen. I'm not sure there is something in place to actually read the patterns directory and check for new files.
• If the theme new version _deleted_ some pattern files, I have no idea what would happen.

I'm not sure these scenarios have been considered and actually _tested_ when the caching mechanism has been implemented.

There could be more scenarios where the theme pattern files get renamed / added / deleted that haven't been taken into consideration.

Since the new release is very close, I'd think it's important to double check all possible, legitimate, scenarios and prevent any PHP warning and the like

[SergeyBiryukov]

Replying to afercia:

This morning I pulled latest changes onto my local WordPress trunk environment.
Suddenly, all the admin pages showed an unexpected spacing at the top of the page. That's because the CSS class php-error was added to the body element but no actual error or warning is printed out on the page. See attached screenshot.

Related: #51383

[afercia]

As @SergeyBiryukov pointed out, the reason why the PHP warning isn't actually printed out on the page may be related to https://core.trac.wordpress.org/ticket/51383

[spacedmonkey]

@afercia This might be related to #59490. Are you running your local environment with development mode active? Caches will need to invalidated if patterns have been removed.

[afercia]

@spacedmonkey yes of course this is wordpress-develoop trunk, standard configuration:
define( 'WP_DEVELOPMENT_MODE', 'core' );

My point is more about what happens when a theme gets updated to a new version (with renamed / added / deleted pattern files) via FTP or other ways that don't trigger caches to be invalidated.

[spacedmonkey]

If you editing a theme, then it should be define( 'WP_DEVELOPMENT_MODE', 'theme' ).

We may have to update the caching to check to see if the file exists.

Pinging the performance team, as a bug related to one of our changes. @joemcgill @flixos90

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

Improve error handling of block pattern registration. Error handling is improved in the following ways.

• New doing it wrong message for empty files.
• PHP error are suppressed, so the empty file check is triggered.
• Check invalidation if file does not exist.

[spacedmonkey]

@afercia I have put together fix for users uploading block patterns. See my ​PR. This should resolve this issue.

@felixarntz

The bug comes from when a block pattern file is removed or renamed, when developer mode is not active. A valid use case for those that using FTP files to deploy changes. If a user / developers removes or renames, this generates a PHP warning, as the file does not exist. This is because the new pattern cache, uses the keys in the array to do include. What this patch does, is if the file does not exist, now we silence the error, it will see if the file is empty and then flush the pattern cache. This means the in the extremely rare cache of the file magically not existing, it will force a cache refresh. If the file has been removed, then the cache will regenerate and remove the file from. If it is a case that file was unavaliable while a file is being uploaded, the regeneration should be in time for file to be able.

This PR is being overly careful while files are being updated or if files are renamed.

[spacedmonkey]

Assigning to @afercia for review.

@spacedmonkey Thanks for clarifying. I agree with the idea behind it, however I have one concern with your reply:

A valid use case for those that using FTP files to deploy changes.

Deploying a new version of a theme via FTP or git is totally reasonable and should work given the caches only are used if the theme version matches. However, deploying changes to specific files in a theme is literally why WP_DEVELOPMENT_MODE exists. If you change a specific file in a theme, you are working on the theme, so you need to set WP_DEVELOPMENT_MODE to "theme".

Regarding the concrete fix, I think there is a better solution, or at least an additional fix to add: We should solve this also at the caching level itself: In _wp_get_block_patterns(), we should ensure that, if theme development mode is enabled, any existing caches are wiped. Otherwise, it leads to a problem like the one reported when you then turn theme development mode off later, since the caches would still exist from before. So I think we should adjust the beginning of _wp_get_block_patterns() to something like this:

$pattern_data = $theme->get_pattern_cache();
if ( is_array( $pattern_data ) {
        if ( $can_use_cached ) {
                return $pattern_data;
        }
        $theme->delete_pattern_cache();
}

This ensures that, as soon as someone enables theme development mode, existing caches are wiped.

[afercia]

I'm prett sure there are contributors more knowledgeable than me to own and review this ticket.

[afercia]

Question related to one more scenario:

• As a site owner, I create my own custom pattern file.
• I upload it via FTP to my active theme patterns directory.
• I would expect my new pattern to just work.

What happens in this case?

[flixos90]

Replying to afercia:

Question related to one more scenario:

• As a site owner, I create my own custom pattern file.
• I upload it via FTP to my active theme patterns directory.
• I would expect my new pattern to just work.

What happens in this case?

In order to make direct changes to the theme (a.k.a. develop the theme), you should define the WP_DEVELOPMENT_MODE constant and set it to "theme" in wp-config.php. This will ensure any theme specific caching is disabled while you're making changes.

Once you're done making changes, you could unset the constant again. Or, if you're regularly making changes to the theme on that site, you could keep the constant set like that permanently, which would be a tradeoff between development convenience vs the additional performance boost of caching certain theme files.

[flixos90]

In 56931:

Themes: Clear existing pattern cache when in theme development mode and prevent PHP warning due to missing file.

Follow up to [56765].

Props spacedmonkey, afercia, jrf, flixos90.
Fixes #59591.
See #59490.

Committed in https://core.trac.wordpress.org/changeset/56931

[afercia]

In order to make direct changes to the theme (a.k.a. develop the theme), you should define the WP_DEVELOPMENT_MODE constant and set it to "theme" in wp-config.php. This will ensure any theme specific caching is disabled while you're making changes.

I'm not sure this answers my question.

As a site owner, I'm not required to know what the WP_DEVELOPMENT_MODE constant is. I just want to upload a new pattern file to my directory and I'd expect it to just work.

Actually, in this scenario, as a site owner I'm not even _editing_ the theme. I'm just uploading a new pattern file. This is a pretty common scenario and I don't think it is covered by the current implementation.

I'm pretty sure there are more scenarios that I can't think of right now that may be very confusing for users. At the very least the caching mechanism should be very well explained in the UI. It should not be an invisible feature. I'd also think there should be a button in the UI to allow users to invalidate the cache.

[flixos90]

Replying to afercia:

As a site owner, I'm not required to know what the WP_DEVELOPMENT_MODE constant is.

That is correct. The WP_DEVELOPMENT_MODE constant is relevant only when developing on a WordPress site, either core, a plugin, or a theme. For theme developers though, it is crucial to be familiar with the constant.

I just want to upload a new pattern file to my directory and I'd expect it to just work.

Actually, in this scenario, as a site owner I'm not even _editing_ the theme. I'm just uploading a new pattern file.

In that case, you are developing/editing a theme. Adding a pattern to a theme, updating a pattern in a theme, editing theme.json, all of that is developing a theme.

In order for your changes to be properly reflected while developing, you need to set the WP_DEVELOPMENT_MODE constant to "theme". See ​https://make.wordpress.org/core/2023/07/14/configuring-development-mode-in-6-3/

I'm pretty sure there are more scenarios that I can't think of right now that may be very confusing for users.

Users can edit patterns in the UI, and that is not affected by the caching mechanisms. And it shouldn't, since doing so is not editing a theme.

Note that all of this is conceptually not new. The WP_DEVELOPMENT_MODE constant was introduced in 6.3, largely for the very purpose it is used for here. Certain theme files have been cached even since 6.1. This ticket is not the place to discuss the higher-level concept behind the development mode. If you disagree with the concept, please open a separate ticket.

[afercia]

@flixos90 when a ticket has the 2nd-opinion keyword, that means _another_ person who is not one of the people who implemented the feature is needed to express an opinion. See ​https://make.wordpress.org/core/handbook/contribute/trac/keywords/#action-based-keywords

I understand you are attached to this feature as you are part of the team that implemented it. However, I would like to hear thoughts from _other_ committers and tech leads. For this reason, I'm going to reopen again this ticket. Also, before closing a ticket it would be fair to check if there is consensus.

I feel this issue is being viewed a bit too much under a _technical_ perspective. I'm not interested in a "formally correct" technical approach. My concern is that this caching mechanism will break the flow of many users. Just to mention one scenario: professionals who manage several sites for their customers and upload stuff via FTP. I think the user experience here has been overlooked and I would like to hear second opinions from people outside of the team who implemented this feature. Thanks.

[flixos90]

@afercia I understand what 2nd opinion means. However, your concerns are based on the concept behind the development mode, which is not what this ticket is about. Also, please do not make this personal. I am very well aware of my involvement in this effort, and I do not gain anything from having development mode or this ticket in core.

The reason I closed the ticket is that it's not the right place for discussing the concerns you raised. This ticket caches block pattern data, which comes from theme files. Even if this was reverted, there would still be other instances of cached theme files still in core. We need to have a holistic discussion and make a holistic decision on this topic - not handle it one way in one ticket and another way in another.

I would encourage you to open a new ticket for that reason. I am not going to close this again, but please consider the above.

[afercia]

Also, please do not make this personal. I am very well aware of my involvement in this effort, and I do not gain anything from having development mode or this ticket in core.

I didn't mean anything malicious. I did mean that, from a human perspective, I do understand very well how much everybody is 'emotionally' attached to their own code, because of the huge effort and dedication we all put in contributing to this project. Based on my experience, sometimes this emotional attachment may lead to decisions that aren't ideal. We all should make an effort to be ready to accept concerns and criticism even when we don't like it. Personally, I learned it the hard way.

there would still be other instances of cached theme files still in core. We need to have a holistic discussion and make a holistic decision on this topic

I agree on this point. I also heard a similar caching mechanism is being worked on for templates, so that a holistic discussion makes totally sense. My concern is that tomorrow is WordPress RC 1 day and we're a bit too late to make sure caching won't introduce new problems for users.

We need to have a holistic discussion

Starting a discussion the day before RC 1 is maybe a bit too late but fair enough, I will create a new ticket.

Scale back on the caching of block patterns. Still do glob on every request. This way the code can easily detect the remove and adding of files in this directory.

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

[spacedmonkey]

The current solution, does not take into account the adding of new pattern files.

I have put together a ​PR that scales back the caching a little to add for the detection of new files being added.

This PR replaces 50+ file_exists and with a simple glob function call, which maybe better for performance.

Can you take a look @flixos90

[afercia]

Thanks @spacedmonkey re-reading carefully this ticket, yes I'd agree it can't be considered 'solved' until ​https://github.com/WordPress/wordpress-develop/pull/5495 gets in

Despite the above, what you're doing here partially addresses the concerns on the ticket, so I'm not strongly opposed to merging this as a small improvement. But we should have unit tests to make sure it doesn't introduce problems, especially this late in the cycle.

@felixarntz You mind doing some performance profiling on this change? I believe one glob is better than 50 files exist for performance. Sadly I will not have time to work on this, as I am busy with other things.

[nicolefurlan]

Given the outstanding discussion going on here it looks like we should bump this to 6.5. Do we agree @flixos90?

[hellofromTonya]

Hello all,

Offering a 2nd-opinion. I think this ticket has been resolved by [56931]. Thus it can be reclosed as fixed.

#59633 is opened for continued discussions and exploration of potential missing user workflows in the caching strategy.

As I mentioned in that ticket, I share in @flixos90 concern for considering PR 5459. I moved that PR to #59633.

[afercia]

Replying to hellofromTonya:

As I mentioned in that ticket, I share in @flixos90 concern for considering PR 5459. I moved that PR to #59633.

Just for a correct reference, I think it's PR 5495, not PR 5459.

[joemcgill]

In 56978:

Themes: Make caches for block patterns clearable.

In [56765], theme block pattern files were cached to a transient as a performance enhancement. However, transients are not easily clearable when caches are flushed on environments not using a persistent cache, which can lead to errors if the theme files are renamed, edited, or moved.

This changes the caching mechanism to use wp_cache_set() instead, and caches these values to the global group so they are still persistent on environments using an object cache, and will be cleared by a cache flush.

In addition, the helper _wp_get_block_patterns has been moved WP_Theme::get_block_patterns for consistency with other block related theme methods and cache helpers for these values, WP_Theme::get_pattern_cache and WP_Theme::set_pattern_cache, have been made private.

Relevant unit tests updated.

Props: afercia, flixos90, mukesh27, joemcgill.
Fixes #59633. See #59591, #59490.

[joemcgill]

In 56979:

Themes: Make caches for block patterns clearable.

In [56765], theme block pattern files were cached to a transient as a performance enhancement. However, transients are not easily clearable when caches are flushed on environments not using a persistent cache, which can lead to errors if the theme files are renamed, edited, or moved.

This changes the caching mechanism to use wp_cache_set() instead, and caches these values to the global group so they are still persistent on environments using an object cache, and will be cleared by a cache flush.

In addition, the helper _wp_get_block_patterns has been moved WP_Theme::get_block_patterns for consistency with other block related theme methods and cache helpers for these values, WP_Theme::get_pattern_cache and WP_Theme::set_pattern_cache, have been made private.

Relevant unit tests updated.

Props afercia, flixos90, mukesh27, joemcgill.
Merges [56978] to the 6.4 branch.
Fixes #59633. See #59591, #59490.