Fix GH-18714: opcache_compile_file() breaks class hoisting#21470
Fix GH-18714: opcache_compile_file() breaks class hoisting#21470iliaal wants to merge 2 commits intophp:masterfrom
Conversation
opcache_compile_file() sets ZEND_COMPILE_WITHOUT_EXECUTION which prevented simple parentless classes from being linked during compilation. When the cached script was later loaded via require, opcache's delayed early binding couldn't resolve the unlinked class, causing "Class not found" errors for classes used before their declaration. Two changes: 1. zend_compile.c: Allow linking simple classes (no parent, no interfaces, no traits) even with ZEND_COMPILE_WITHOUT_EXECUTION. This is purely structural with no execution side effects. The change is scoped to non-preload compilation to avoid interfering with preloading's own class linking pipeline. 2. zend_accelerator_module.c: Clean up classes/functions registered by zend_accel_load_script() after opcache_compile_file() returns. The function should only cache, not pollute the runtime tables. Cleanup is skipped during preloading where registrations must persist. Closes phpGH-18714
dstogov
left a comment
dstogov
left a comment
There was a problem hiding this comment.
Can you please try to minimize the changes in zend_compile.c. Or separate significant changes from indentation in different commits.
Also see my thought in comments.
| } | ||
| ht->pDestructor = orig_dtor; | ||
| } | ||
|
|
There was a problem hiding this comment.
You can use ZEND_HASH_MAP_FOREACH_END_DEL. See examples in Zend/zend_execute_API.c and ext/opcache/ZendAccelerator.c.
There was a problem hiding this comment.
Switched to zend_hash_discard(), which does the same thing in one call: removes entries from nNumUsed back to a given point, updates hash chains, skips the destructor. Dropped the custom accel_rollback_hash function.
| /* Undo classes/functions registered by zend_accel_load_script(). | ||
| * opcache_compile_file() should only cache without side effects. | ||
| * Skip during preloading: preload needs the registrations to persist. */ | ||
| if (!(orig_compiler_options & ZEND_COMPILE_PRELOAD)) { | ||
| accel_rollback_hash(EG(class_table), orig_class_count); | ||
| accel_rollback_hash(EG(function_table), orig_function_count); | ||
| } |
There was a problem hiding this comment.
I think the old behavior may make sense not only for preloading.
Changing it may cause troubles for existing use cases.
I would propose to introduce the new behavior with an optional boolean argument, keeping the old behavior by default. What do you think?
There was a problem hiding this comment.
I think the registration is a side effect of the implementation path (persistent_compile_file → zend_accel_load_script), not intentional behavior. The function compiles with ZEND_COMPILE_WITHOUT_EXECUTION, and registering classes and functions in the runtime tables is an execution side effect.
Users who call opcache_compile_file() then require_once the same file hit this bug because of that side effect: duplicate class/function registration. Making the fix opt-in means anyone who hits #18714 needs to discover a new parameter to get correct behavior.
If there are known use cases that depend on the registration happening, I'm open to adding the argument. I haven't found any in the docs or issue tracker though -- the documented purpose is cache warming, not class loading.
- zend_compile.c: keep original block untouched, add separate block for ZEND_COMPILE_WITHOUT_EXECUTION class linking after it - zend_accelerator_module.c: replace custom accel_rollback_hash with built-in zend_hash_discard()
|
Restructured. The |
2 participants
Summary
opcache_compile_file()followed byrequire_onceon the same file fails with "Class not found" when the class is used before its declaration (relying on class hoisting).Root cause:
opcache_compile_file()setsZEND_COMPILE_WITHOUT_EXECUTIONwhich prevents simple parentless classes from being linked during compilation. The cached script stores the class as unlinked, and opcache's delayed early binding can't resolve it -- the class has no parent to look up but isn't marked as linked either, so the binding is silently skipped.Fix (two parts):
Zend/zend_compile.c: Add a separate block after the existing early-binding logic that links simple classes (no parent, no traits, no interfaces) duringZEND_COMPILE_WITHOUT_EXECUTION. Linking is purely structural (zend_build_properties_info_table+zend_inheritance_check_override+ZEND_ACC_LINKEDflag) with no execution side effects. Skipped during preloading which has its own class linking pipeline. The existing early-binding block is untouched.ext/opcache/zend_accelerator_module.c:opcache_compile_file()goes throughpersistent_compile_file()which callszend_accel_load_script(), registering classes/functions in the runtime tables. This causes duplicate registration whenrequire_oncelater loads the same file. Clean up viazend_hash_discard()after compilation returns. Cleanup is skipped during preloading where registrations must persist.Fixes #18714