Make WordPress Core

Opened 8 weeks ago

Closed 7 weeks ago

Last modified 7 weeks ago

#63212 closed enhancement (fixed)

Enhancement: Do not throw PHP notice when file is not a pattern registration file

Reported by: webmandesign's profile webmandesign Owned by: joemcgill's profile joemcgill
Milestone: 6.8 Priority: normal
Severity: normal Version: trunk
Component: Themes Keywords: has-patch changes-requested fixed-major dev-reviewed
Focuses: Cc:

Description

Hi,

Since testing WP6.8 RC1 I get this PHP notice:

Notice:
Function get_block_patterns was called incorrectly.
Could not register file "./themes/theme-slug/patterns/affected/file.php" as a block pattern ("Slug" field missing)
Please see Debugging in WordPress for more information.
(This message was added in version 6.0.0.) in ./wp-includes/functions.php on line 6121

Could you please modify the code to skip all PHP files without "Slug" defined in theme-slug/patterns folder?

As maybe such files are not meant for a pattern registration purpose. They may be helper files for patterns, for example.
That way the notice is not triggered and file is skipped during automatic WordPress pattern registration.


My usecase for explanation:

In my theme I register most of the patterns with proper file metadata.

However, one of the patterns runs dynamic PHP code that has to be registered later on front-end (during wp action hook) to receive proper data from WordPress. I need to register this pattern using PHP `register_block_pattern()` function instead of setting the pattern file metadata.

To fix the new WP6.8 PHP notice I would need to move the affected pattern from patterns/ folder to other folder, such as patterns-other/, which creates inconsistency in my theme code organization.

Another alternative solution for me would be to register all patterns with register_block_pattern() function and not use patterns/ folder at all...

Pre-WP6.8 solution was to have pattern files with metadata as direct children of patterns/ folder and use subfolders for the patterns registered via register_block_pattern() function.


Proposed solution:

The solution is as simple as removing _doing_it_wrong() block from if ( empty( $pattern['slug'] ) ) {...} statement in get_block_patterns() function (check the code at GitHub).

Change History (25)

#1 @audrasjb
8 weeks ago

  • Version trunk deleted

I'm removing trunk version since this wasn't introduced in 6.8 and it is not related to any change that happened in 6.8.

#2 @audrasjb
8 weeks ago

  • Summary changed from WP6.8-RC1: Enhancement: Do not throw PHP notice when file is not a pattern registration file to Enhancement: Do not throw PHP notice when file is not a pattern registration file

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


8 weeks ago
#3

  • Keywords has-patch added

Skipping non-pattern-definition PHP files within theme-slug/patterns folder during block pattern registration.
This allows using helper PHP files within theme-slug/patterns folder, keeping pattern related code in the theme together.

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

#4 @webmandesign
8 weeks ago

@audrasjb Please note that this issue occurs for WP6.8 only.

This is due to the change in WP6.8 when all files including files in subfolders are being used to register patterns within theme-slug/patterns folder.

Previously only direct children of theme-slug/patterns folder were used, not the files in subfolders.

The change is related to WP6.8 development, to ticket #62378

#5 @audrasjb
8 weeks ago

  • Milestone changed from Awaiting Review to 6.8
  • Version set to trunk

AH. Sorry I misinterpreted the issue. I'm moving it back to 6.8 so we can try to sort it out for RC3. Otherwise it will be moved to 6.8.1. Thank you for clarifying.

#6 @webmandesign
8 weeks ago

Thank you @audrasjb

I'm hoping the issue is going to be fixed for 6.8 release. The fix is very easy if you consider my proposition.

If the fix is postponed to 6.8.1 or later, I would either need to advice my theme users to wait for such late WordPress update or change the name of the patterns folder to something else temporarily and then change it back to patterns for WP6.8.1+ for all of my block themes. That seems to me as an unnecessary walkaround affecting my theme users with crucial, yet somehow unnecessary theme updates.

Thanks for understanding.

#7 @joemcgill
8 weeks ago

  • Owner set to joemcgill
  • Status changed from new to reviewing

#8 follow-up: @joemcgill
8 weeks ago

Hi @webmandesign. Could you give me more info about how you previously had set up your file structure for patterns? It sounds like you previously had sub-folders in the /patterns directory that would not get autoloaded, but now they are being autoloaded since [59872] and this causing the warning you describe. Is that correct?

I don't think we will want to remove the _doing_it_wrong() that you are suggesting, but perhaps we can develop a workaround to support organizing pattern files in your /patterns directory that should not be autoloaded.

#9 @webmandesign
8 weeks ago

Hi @joemcgill

Your presumption is correct.

As an example I will use my publicly available theme Zooey that has slightly different usecase than the one I originally described in this ticket:

I have subfolders in patterns/ directory to organize my block patterns and I register these patterns with `register_block_pattern()` function providing user a way to filter the patterns list before registration (this will become a theme option in future update so user can disable patterns they don't use).

So, even though I use patterns/ directory to organize my block patterns, I don't provide file metadata for their registration - I use the register_block_pattern() function. All of the files in patterns/ directory are throwing the PHP notice in WP6.8.

It almost looks like I can not use register_block_pattern() function together with patterns/ directory to organize my patterns.

Solution for a theme like Zooey would be as simple as renaming patterns/ directory to pattern/. But it kind of looks wrong as I have plural directory names in the theme folder structure, such as parts, templates and using singular pattern looks weird to me.
But maybe renaming to block-patterns/ would do the trick, though it feels really weird to me that I have to do this hack.

You can test this theme example in WP playground - check the logs (FYI: there is also _load_textdomain_just_in_time PHP notice for this particular theme I will fix in the theme update).


I can come up with additional usage examples, though:

Let's say I have this block pattern files structure in my theme:

patterns/
	helpers/
		icon.php
	columns/
		icons-in-2-columns.php
		icons-in-3-columns.php 

The patterns/helpers/icon.php is not a pattern file. It is a helper file I use not to repeat the same code across multiple pattern files. This is the content of the file:

<!-- wp:image {"sizeSlug":"full"} -->
<figure class="wp-block-image size-full">
<img src="<?php echo esc_url( get_theme_file_uri( 'assets/images/icon-demo.webp' ) ); ?>" alt="Demo icon"/>
</figure>
<!-- /wp:image -->

Then I include the patterns/helpers/icon.php in the actual pattern files within patterns/columns/ folder.

With this example I'd get PHP notice for the patterns/helpers/icon.php file.

I'd like to keep the patterns/helpers/icon.php in the patterns/ folder because I use it only for patterns, so I like to keep patterns code organized together.

#10 in reply to: ↑ 8 @webmandesign
8 weeks ago

Replying to joemcgill:

I don't think we will want to remove the _doing_it_wrong() that you are suggesting, but perhaps we can develop a workaround to support organizing pattern files in your /patterns directory that should not be autoloaded.

Well, that's pity :)

In that case I would take some filter hook too. Such as providing a filter for `$files` variable.

(Or possibly even filtering for related `scandir()` method output, or its $path variable value, so I can short circuit it.)

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


8 weeks ago

#12 @poena
7 weeks ago

Hi

I agree that the notice needs to be kept to help with pattern registration.
I understand its an inconvenience, but as I see it there are two options, disable notices or move the files.

#13 follow-up: @poena
7 weeks ago

Even if the code was updated to exclude one named folder inside patterns (example: helpers), theme developers that have used subfolders this way would still need to take action, so it would not really solve anything.

#14 in reply to: ↑ 13 @webmandesign
7 weeks ago

Replying to poena:

Even if the code was updated to exclude one named folder inside patterns (example: helpers), theme developers that have used subfolders this way would still need to take action, so it would not really solve anything.

I agree. Having a special subfolder name to skip is not a valid option even for me. I'd much prefer something more flexible.

I've updated my GitHub PR code and proposing:

  1. Adding get_block_patterns_files (name is subject to change) filter to allow theme authors filter out the array of files within patterns directory before they are processed. This way a theme author doing something non-standard with patterns can manage, while still keeping the default patterns directory functionality intact.
  1. I've also added a check whether the pattern file has both slug and title set. If both are empty, then maybe we can assume this is not a pattern file? Surely, this can be removed if this is "too much".

#15 @tremidkhar
7 weeks ago

I agree that pattern registration should be flexible, and the filter-out approach seems logical. This allows developers to exclude non-pattern files from the patterns directory as needed.

However, I believe the core function should throw an error if a pattern file is missing the required slug metadata. If a developer wants to suppress the error, they can simply filter out the file using the available filter.

#16 @webmandesign
7 weeks ago

I've forgotten to mention that I returned back the _doing_it_wrong() PHP notice for slug in my GitHub PR.

So, really the question now is whether we get the filter hook for the pattern files in WP6.8.

#17 @joemcgill
7 weeks ago

  • Keywords changes-requested added
  • Status changed from reviewing to accepted

Thanks @webmandesign. I've left a few small bits of feedback on the PR, @poena do you have any concerns with the latest proposal?

#18 @webmandesign
7 weeks ago

Thanks for the feedback @joemcgill I've updated the PR code.

#19 @joemcgill
7 weeks ago

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

In 60142:

Themes: Allow files in a block theme's 'patterns' directory to be filterable.

Since [59872] all PHP files in the 'patterns' directory or subdirectories are auto registered. Adding a filter prior to autoregistration allows theme developers to modify the list of files before the auto registration.

Props webmandesign, joemcgill, jorbin, poena.
Fixes #63212.

#20 @joemcgill
7 weeks ago

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

Reopening for consideration in the 6.8 branch

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


7 weeks ago

#22 @audrasjb
7 weeks ago

  • Keywords dev-reviewed added; dev-feedback removed

The new filter looks good for a 6.8 backport @joemcgill

#23 @joemcgill
7 weeks ago

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

In 60144:

Themes: Allow files in a block theme's 'patterns' directory to be filterable.

Since [59872] all PHP files in the 'patterns' directory or subdirectories are auto registered. Adding a filter prior to autoregistration allows theme developers to modify the list of files before the auto registration.

Reviewed by audrasjb.
Merges [60142] to the 6.8 branch

Props webmandesign, joemcgill, jorbin, poena.
Fixes #63212.

#24 @webmandesign
7 weeks ago

Thank you @joemcgill, @audrasjb, @poena for implementing this enhancement for WP6.8!

#25 @joemcgill
7 weeks ago

@webmandesign thanks for the thoughtful feedback about how the change to the /patterns directory affected you and for collaborating on a solution!

Note: See TracTickets for help on using tickets.