Skip to content

Conversation

@arnaud-lb
Copy link
Member

@arnaud-lb arnaud-lb commented Jan 7, 2026

Changes introduced in #20717 do not handle ZEND_AST_CALLABLE_CONVERT properly in file cache: nodes in zend_ast_fcc->args are serialized, but not zend_ast_fcc->args itself. This breaks the build: https://github.com/php/php-src/actions/runs/20767748729/job/59637430230.

cc @iluuu1994 @TimWolla

Comment on lines +388 to +391
SERIALIZE_PTR(fcc->args);
tmp = fcc->args;
UNSERIALIZE_PTR(tmp);
zend_file_cache_serialize_ast(tmp, script, info, buf);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same logic as the generic loop bellow

@arnaud-lb arnaud-lb marked this pull request as ready for review January 7, 2026 16:14
@arnaud-lb arnaud-lb requested a review from dstogov as a code owner January 7, 2026 16:14
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question. LGTM otherwise.

zend_ast_fcc *fcc = (zend_ast_fcc*)ast;
ZEND_MAP_PTR_INIT(fcc->fptr, NULL);
zend_file_cache_serialize_ast(fcc->args, script, info, buf);
if (!IS_SERIALIZED(fcc->args)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be reached multiple times per AST, or does this just mirror the other branches?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mirrors the other branches, but I assume that this can be reached multiple times when the AST is referenced from multiple places. In the other branches this happens at least when serializing ast class consts, for inherited consts.

@arnaud-lb arnaud-lb merged commit 92b2887 into php:master Jan 7, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants