zend_ast: Add parentheses around IIFE in zend_ast_export() #18688
Conversation
TimWolla
changed the title
There was a problem hiding this comment.
Changes make sense to me. I would consider this a bugfix and apply to PHP 8.3?
@xepozz While there are no official commit message guidelines, using the conventional commit style is super unusual in php-src. Personally I just prefix the Subject with a short "module indicator" and thus retitled the PR.
|
I would personally not consider this a bug fix. The dumper has known rough edges. |
Okay, then master it is. Unless someone beats me to it, I'll then add NEWS and merge in the next days. |
should I rename the commits now or we'll squash them with PR title when merge it? |
We're squash merging in php-src, so I'll make sure to adjust the commit message then. One thing I noticed while adding NEWS was that your base branch was greatly out of date. Please make sure to keep your fork up to date. Easiest done with the "Update branch" button here:
|
Co-authored-by: Dmitrii Derepko <[email protected]>
@TimWolla I'd suggest to enable this setting to be able update the branch in the PR |
This would unnecessarily run CI twice. The base branch being a little out of date is generally fine with the activity happening in php-src, but in this case it was 3 months behind. Was easy enough to merge this locally while adding the NEWS file. Just waiting for another CI pass now, though 😃 |
| case ZEND_AST_CALL: { | ||
| zend_ast *left = ast->child[0]; | ||
| if (left->kind == ZEND_AST_ARROW_FUNC || left->kind == ZEND_AST_CLOSURE) { | ||
| smart_str_appends(str, "("); |
There was a problem hiding this comment.
Should have used smart_str_appendc() here and below, fixing in #18703 along with a few others
No description provided.