Skip to content

Zend: simplify logic in zend_lookup_class_ex() - #24326

Open
Girgias wants to merge 3 commits into
php:masterfrom
Girgias:2026-10-class_lookup2
Open

Girgias wants to merge 3 commits into
php:masterfrom
Girgias:2026-10-class_lookup2

Conversation

@Girgias

@Girgias Girgias commented Oct 14, 2026

Copy link
Copy Markdown
Member

Commits should be reviewed individually

Supersedes #24277 and #24271

@Girgias

Girgias commented Oct 14, 2026

Copy link
Copy Markdown
Member Author

@devnexen can you check this no longer crashes the test case you had?

@devnexen

Copy link
Copy Markdown
Member

no crash :)

@Girgias
Girgias force-pushed the 2026-10-class_lookup2 branch from 7b8d42e to d1e51db Compare October 15, 2026 11:14
@Girgias
Girgias marked this pull request as ready for review October 16, 2026 11:02
@Girgias
Girgias requested review from bukka and dstogov as code owners October 16, 2026 11:02

@ndossche ndossche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agre with the idea, see my nit

Comment thread Zend/zend_execute_API.c
zend_string_release_ex(lc_name, 0);
return NULL;
}
ZEND_ASSERT(zend_is_valid_class_name(name) && "If lowercase name is valid the name must also be so");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure if the compiler is smart enough to avoid this call in release builds

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sorry this took a while and it's now done.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

with debug build I get the assertion fired with just this (there are probably other cases)

''::foo();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Interesting, nice find, that should be fixed first.

@Girgias
Girgias force-pushed the 2026-10-class_lookup2 branch from d1e51db to 43316ba Compare October 31, 2026 06:40
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.

3 participants