Repository navigation
Conversation
|
Size Change: 0 B Total Size: 6.84 MB ℹ️ View Unchanged
|
scruffian
changed the title
scruffian
marked this pull request as ready for review
February 20, 2026 12:22
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
scruffian
added
[Type] Code Quality
scruffian
force-pushed
the
fix/shared-navigation-files
branch
2 times, most recently
from
February 24, 2026 15:04
9ff9da6 to
458347d
Compare
|
Flaky tests detected in 458347d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/22356642175
|
jeryj
reviewed
Feb 24, 2026
jeryj
left a comment
jeryj
left a comment
Contributor
There was a problem hiding this comment.
This PR gives me pause. It feels difficult to understand the impact of changing these build processes.
Previously, cross-file function references required manually adding entries to the prefixFunctions array in package.json.
Can you give an example of how we could address the shared navigation php code using this prefixFunctions array in package.json? If we can use the existing system, I think I'd prefer doing that than touching the build.
If this PR does work to autoprefix all the functions, then shouldn't we remove the instances of prefixFunctions from the package.json s that use it?
Contributor
Author
The These are functions defined in WordPress core, so the auto-prefix first pass won’t discover them. They still need the manual list so the build knows to transform wp_apply_colors_support → gutenberg_apply_colors_support (note the wp_ stripping). The auto-prefix only handles cross-file references within the same package — functions that are defined in one PHP file and called from another PHP file in packages/block-library/src/. |
Contributor
Author
yes, we could just add |
Instead of manually adding internal cross-file function names to the prefixFunctions array, the build now collects all function definitions across PHP files in a first pass and automatically prefixes cross-file references. Comments are left untouched. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Source files are never loaded directly — only the built versions are used at runtime. All other blocks already use build-layout paths (e.g., __DIR__ . '/block-name'), so the file_exists check to handle source vs build paths was unnecessary dead code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
scruffian
force-pushed
the
fix/shared-navigation-files
branch
from
February 25, 2026 13:56
52e8cbd to
77cb33e
Compare
The simplified regex caused substring matches — e.g., `render_block_core_navigation` matched inside `gutenberg_render_block_core_navigation_link`, producing double-prefixed function names. Add lookbehind/lookahead word boundaries to prevent this. Also remove hardcoded `gutenberg_` prefix from `item_should_render` calls in source files so the build adds it consistently. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
@scruffian I'm not sure I'm following properly the discussion here, my reading is that |
Contributor
Author
That's right, the problem is that because of the way prefixing works in gutenberg, the function definitions in the block library are automattcally prefixed with It works in trunk because we have the function definitions in core as well, but if you change the functions in gutenberg, nothing happens because they aren't being called. My suggestion here is to also add the prefixing to the places where we call the functions as well as the definitions. The alternative that Jerry is proposing is to just add the functions to the prefixFunctions array, but my concern is that this array will continue to grow as we add more shared functions. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors Navigation block PHP helpers by moving submenu-icon rendering into a shared helper and updates the PHP build transform to automatically prefix cross-file function references, while simplifying require_once paths in related blocks.
Changes:
- Updates
wp-build’s PHP transform to collect package-wide function definitions and auto-prefix cross-file function references during build. - Removes
block_core_navigation_render_submenu_icon()from the Navigation block and switches Navigation Link/Submenu to the sharedblock_core_shared_navigation_render_submenu_icon(). - Removes
file_exists-based branching in Navigation Link/Submenu PHP includes, keeping only build-layout paths.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/wp-build/lib/build.mjs | Adds cross-file PHP function definition scanning and auto-prefixing logic in the build transform. |
| packages/block-library/src/navigation/index.php | Deletes the submenu icon helper from Navigation block to avoid duplication. |
| packages/block-library/src/navigation-link/index.php | Switches includes to build-layout shared helpers and updates submenu icon function usage. |
| packages/block-library/src/navigation-submenu/index.php | Switches includes to build-layout shared helpers and updates submenu icon + shared render-check usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+702
to
+722
| // Collect all function definitions across PHP files so that | ||
| // cross-file references can be auto-prefixed without needing | ||
| // a manual prefixFunctions entry. | ||
| const allFunctionNames = []; | ||
| if ( transforms.php?.functionPrefix ) { | ||
| for ( const filePattern of files ) { | ||
| const matchedFiles = await glob( | ||
| normalizePath( path.join( packageDir, filePattern ) ) | ||
| ); | ||
| for ( const sourceFile of matchedFiles ) { | ||
| if ( ! sourceFile.endsWith( '.php' ) ) { | ||
| continue; | ||
| } | ||
| const content = await readFile( sourceFile, 'utf8' ); | ||
| for ( const [ , name ] of content.matchAll( | ||
| /^\s*function ([^\(]+)/gm | ||
| ) ) { | ||
| allFunctionNames.push( name ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This adds an extra full read/scan of all PHP source files to collect allFunctionNames, and then the build loop reads each file again for transformation. For large packages this is a noticeable build-time regression. Consider collecting definitions while doing the existing per-file reads (e.g. first pass cache of file contents, or a single pass that both collects names and writes outputs) and de-duplicating the function-name list to keep the generated regex smaller.
Comment on lines
+357
to
+365
| const funcPattern = crossFileFunctions.join( '|' ); | ||
| content = content.replace( | ||
| new RegExp( | ||
| '(?<![a-zA-Z0-9_])(?:' + funcPattern + ')(?![a-zA-Z0-9_])', | ||
| 'g' | ||
| ), | ||
| ( match ) => | ||
| `${ functionPrefix }${ match.replace( /^wp_/, '' ) }` | ||
| ); |
There was a problem hiding this comment.
The cross-file prefixing content.replace(...) runs over the entire PHP file, so it will also rewrite occurrences inside PHP comments. That can unintentionally change documentation (e.g. comments that mention another file’s helper function) and contradicts the stated goal of leaving comments untouched. Consider masking/removing comment tokens during the replacement so only code and string literals are processed.
Comment on lines
+350
to
+365
| // Prefix cross-file function references (functions defined in other | ||
| // files within the same package). Skip functions that are local to | ||
| // this file since they were already handled above. | ||
| const crossFileFunctions = allFunctionNames.filter( | ||
| ( name ) => ! localFunctions.has( name ) | ||
| ); | ||
| if ( crossFileFunctions.length ) { | ||
| const funcPattern = crossFileFunctions.join( '|' ); | ||
| content = content.replace( | ||
| new RegExp( | ||
| '(?<![a-zA-Z0-9_])(?:' + funcPattern + ')(?![a-zA-Z0-9_])', | ||
| 'g' | ||
| ), | ||
| ( match ) => | ||
| `${ functionPrefix }${ match.replace( /^wp_/, '' ) }` | ||
| ); |
There was a problem hiding this comment.
allFunctionNames may include functions that already start with the functionPrefix (e.g. a source file defining gutenberg_*). In that case the cross-file prefixing step will match them and prepend functionPrefix again, producing double-prefixed calls. Filter crossFileFunctions to exclude names that already start with the prefix (and/or make the replace callback a no-op when match is already prefixed).
Comment on lines
+344
to
345
| content = content.replace( | ||
| new RegExp( functionName + '(?![a-zA-Z0-9_])', 'g' ), |
There was a problem hiding this comment.
The local function renaming regex only enforces an identifier boundary at the end of the match. Without a start boundary it can rename substrings inside longer identifiers (e.g. my_block_core_foo containing block_core_foo). Align this with the cross-file regex by enforcing a start boundary (and escaping the function name when building the RegExp).
Suggested change
| content = content.replace( | |
| new RegExp( functionName + '(?![a-zA-Z0-9_])', 'g' ), | |
| const escapedFunctionName = functionName.replace( | |
| /[.*+?^${}()|[\]\\]/g, | |
| '\\$&' | |
| ); | |
| const localFuncPattern = | |
| '(?<![a-zA-Z0-9_])' + escapedFunctionName + '(?![a-zA-Z0-9_])'; | |
| content = content.replace( | |
| new RegExp( localFuncPattern, 'g' ), |
jeryj
reviewed
Mar 2, 2026
jeryj
left a comment
jeryj
left a comment
Contributor
There was a problem hiding this comment.
I'm still in favor of using the existing system over refactoring to make it auto-magic. It feels too black-boxey to me to know which function is going to be called and how to adequately test this. Given the large impact if it does break something and the difficulty in tracing what's going on if it does break (the "error" is quite silent, it just calls the wp core function), I prefer keeping the existing system for now.
So, the existing manual way would just be adding block_core_shared_navigation_render_submenu_icon to this prefixFunctions array in block-library/package.json? We could merge that now, and then keep the auto-prefixing open for later if we do keep adding to it?
| 'g' | ||
| ), | ||
| ( match ) => | ||
| `${ functionPrefix }${ match.replace( /^wp_/, '' ) }` |
Contributor
There was a problem hiding this comment.
Should this use the same replace regex/method as one of the ones above?
3 tasks
Contributor
Author
|
Ok, I opened an alternative approach in Navigation blocks: use shared render_submenu_icon via prefixFunctions |
Contributor
Author
|
Fixed in #76372 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Moves the
block_core_navigation_render_submenu_icon()function out ofnavigation/index.phpinto the shared file atnavigation-link/shared/render-submenu-icon.php(asblock_core_shared_navigation_render_submenu_icon) and updates all callers. Also enhances the build system to automatically prefix cross-file PHP function references, and removes unnecessaryfile_existsbranching.Why?
The submenu icon rendering function was duplicated — defined in
navigation/index.phpbut also needed bynavigation-linkandnavigation-submenu. Moving it to a shared file eliminates the duplication and follows the existing pattern for shared helpers (likeitem-should-render.php).Previously, cross-file function references required manually adding entries to the
prefixFunctionsarray inpackage.json. The build now handles this automatically.The
file_existsbranching innavigation-link/index.phpandnavigation-submenu/index.phpexisted to handle path differences between source and build layouts. Since source files are never loaded directly at runtime (only the built versions are used), and all other blocks already use build-layout paths (e.g.,__DIR__ . '/block-name'), this branching was dead code.How?
PHP changes
block_core_navigation_render_submenu_icon()fromnavigation/index.phpnavigation-link/index.phpandnavigation-submenu/index.phpto use the sharedblock_core_shared_navigation_render_submenu_icon()file_existsbranching from both files, keeping only the build-layoutrequire_oncepathsBuild system changes (
packages/wp-build/lib/build.mjs)block_core_shared_navigation_render_submenu_icon→gutenberg_block_core_shared_navigation_render_submenu_icon). Comments are left untouched.prefixFunctions: Added lookbehind/lookahead assertions to prevent partial matches on function names.gutenberg_prefix, preventing double-prefixing.The manual
prefixFunctionsarray is still needed for external WordPress core functions (wp_apply_colors_support, etc.) that aren't defined within the package.Testing Instructions
npm run build— should complete without errors.grep -r "block_core_shared_navigation_render_submenu_icon" build/scripts/block-library/navigation-link/shared/render-submenu-icon.phpshould definegutenberg_block_core_shared_navigation_render_submenu_icon()navigation-link.phpandnavigation-submenu.phpshould callgutenberg_block_core_shared_navigation_render_submenu_icon()grep "block_core_gallery_data_id_backcompatibility" build/scripts/block-library/image.phpgrep -rP "^function (?!gutenberg_)[a-z]" build/scripts/block-library/require_oncepaths are correct in the build:grep -r "require_once.*shared" build/scripts/block-library/__DIR__ . '/navigation-link/shared/...'with nofile_existsbranching.