Repository navigation
Block Parser: Restore count() stack access and type $stack as a list - #13487
mukeshpanchal27 wants to merge 5 commits into
Conversation
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
We tried to change this line and reverted it in 3dba2c6 of #12417 (comment) CC: @westonruter |
westonruter
left a comment
westonruter
left a comment
There was a problem hiding this comment.
Right, this change causes a PHPStan error to be introduced (above rule level 5):
So we reverted the change with this reasoning:
Replace the
array_last()call and its accompanyingLogicExceptionguard with$this->stack[ array_key_last( $this->stack ) ].Both internal call sites are already guarded against an empty stack, so the exception was unreachable from core. Since
add_inner_block()is a public method of a class that can be replaced via theblock_parser_classfilter, throwing a new exception type would have changed behavior for third-party callers, which is out of scope for a coding standards change.Indexing by
array_key_last()keeps the readability improvement without introducing a nullable into the expression.
This error does not occur when using $this->stack[ array_key_last( $this->stack ) ].
So yes, to prevent this issue from being raised again, it probably makes sense to add a comment explaining why array_last() is not being used.
If we did throw an exception, then we would be obliged to add a @throws tag, but then any caller of the function could be flagged for not having a try/catch block when in practice an exception will never be thrown.
Updated the method to access the last element of the stack using array_key_last() instead of array_last() for better static analysis compatibility.
mukeshpanchal27
changed the title
array_last() in WP_Block_Parser::add_inner_block()array_last() is not used in WP_Block_Parser::add_inner_block()
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
Add an inline comment to `WP_Block_Parser::add_inner_block()` recording why the last stack entry is read as `$this->stack[ array_key_last( $this->stack ) ]` rather than with `array_last()`. The latter was tried while preparing r62956 and reverted before it landed, and has since been proposed again. Because `array_last()` returns `null` for an empty array, using it here reintroduces a static analysis error on the property accesses that follow. Guarding the return value is not an option: both call sites already return early when the stack is empty, so the guard would be unreachable from core, and `add_inner_block()` is a public method on a class that can be replaced via the `block_parser_class` filter, so throwing a new exception type would change behavior for third-party callers. A `@throws` tag for an exception that can never be thrown would in turn flag every caller for not wrapping the call in `try`/`catch`. Developed in #13487. Follow-up to r62956. Props mukesh27, soean, westonruter. See #65818. git-svn-id: https://develop.svn.wordpress.org/trunk@63624 602fd350-edb4-49c9-b593-d223f7449a82
Add an inline comment to `WP_Block_Parser::add_inner_block()` recording why the last stack entry is read as `$this->stack[ array_key_last( $this->stack ) ]` rather than with `array_last()`. The latter was tried while preparing r62956 and reverted before it landed, and has since been proposed again. Because `array_last()` returns `null` for an empty array, using it here reintroduces a static analysis error on the property accesses that follow. Guarding the return value is not an option: both call sites already return early when the stack is empty, so the guard would be unreachable from core, and `add_inner_block()` is a public method on a class that can be replaced via the `block_parser_class` filter, so throwing a new exception type would change behavior for third-party callers. A `@throws` tag for an exception that can never be thrown would in turn flag every caller for not wrapping the call in `try`/`catch`. Developed in WordPress/wordpress-develop#13487. Follow-up to r62956. Props mukesh27, soean, westonruter. See #65818. Built from https://develop.svn.wordpress.org/trunk@63624 git-svn-id: http://core.svn.wordpress.org/trunk@62800 1a063a9b-81f0-0310-95a4-ce76da25c4cd
|
TODO: Revisit per WordPress/gutenberg#82695 (review) Note also WordPress/gutenberg#82695 (comment) |
mukeshpanchal27
changed the title
array_last() is not used in WP_Block_Parser::add_inner_block()count() stack access and type $stack as a list
In `WP_Block_Parser::add_inner_block()` the last stack frame is once again read as `$this->stack[ count( $this->stack ) - 1 ]`, undoing the switch to `array_key_last()` made in r62956 and removing the inline comment added in r63624 that explained why `array_last()` is not used. Indexing by `array_key_last()` gives the misleading impression that `$this->stack` could be an associative array, when it is only ever a sequentially indexed list. The original code was already correct, and both callers ensure the stack is not empty. The `$stack` property is now also documented with `@phpstan-var list<WP_Block_Parser_Frame>`, which tells static analysis that the stack is always sequentially indexed. With that in place, there is nothing prompting a switch to `array_key_last()` or `array_last()`, so the explanatory comment is no longer needed. This mirrors the change made to the parser in Gutenberg via WordPress/gutenberg#82695. Developed in #13487. Follow-up to r62956, r63624. Props mukesh27, westonruter, dmsnell, soean. See #65818, #65817. git-svn-id: https://develop.svn.wordpress.org/trunk@63937 602fd350-edb4-49c9-b593-d223f7449a82
In `WP_Block_Parser::add_inner_block()` the last stack frame is once again read as `$this->stack[ count( $this->stack ) - 1 ]`, undoing the switch to `array_key_last()` made in r62956 and removing the inline comment added in r63624 that explained why `array_last()` is not used. Indexing by `array_key_last()` gives the misleading impression that `$this->stack` could be an associative array, when it is only ever a sequentially indexed list. The original code was already correct, and both callers ensure the stack is not empty. The `$stack` property is now also documented with `@phpstan-var list<WP_Block_Parser_Frame>`, which tells static analysis that the stack is always sequentially indexed. With that in place, there is nothing prompting a switch to `array_key_last()` or `array_last()`, so the explanatory comment is no longer needed. This mirrors the change made to the parser in Gutenberg via WordPress/gutenberg#82695. Developed in WordPress/wordpress-develop#13487. Follow-up to r62956, r63624. Props mukesh27, westonruter, dmsnell, soean. See #65818, #65817. Built from https://develop.svn.wordpress.org/trunk@63937 git-svn-id: http://core.svn.wordpress.org/trunk@63103 1a063a9b-81f0-0310-95a4-ce76da25c4cd
✅ Committed in:
Trac ticket: https://core.trac.wordpress.org/ticket/65818
Follow-up to r63624. Syncs
WP_Block_Parserwith WordPress/gutenberg#82695.Changes
$this->stack[ count( $this->stack ) - 1 ]inWP_Block_Parser::add_inner_block(), replacingarray_key_last().array_last()is not used.@phpstan-var list<WP_Block_Parser_Frame>to the$stackproperty.Why
As raised by @dmsnell in WordPress/gutenberg#82695 (review), using
array_key_last()suggests that$this->stackcould be an associative array, which it never is. The originalcount()-based access was already correct. Both callers ensure the stack is not empty.Typing
$stackas alist, as suggested by @westonruter in WordPress/gutenberg#82695 (comment), tells static analysis that the stack is always sequentially indexed. That makes it clear there is no need forarray_key_last()orarray_last(), so the explanatory comment is no longer needed. PHPStan reports no errors for the file.Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5.5
Used for: Applying the review feedback and drafting this description. Reviewed and verified by me.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.