Skip to content

Fix GH-18714: opcache_compile_file() breaks class hoisting #21470

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
iliaal wants to merge 2 commits into
base: master
Choose a base branch
from
+55 −0
Open

Fix GH-18714: opcache_compile_file() breaks class hoisting #21470

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e974407
Fix GH-18714: opcache_compile_file() breaks class hoisting
iliaal Mar 18, 2026
fbc29d8
Address review feedback: minimize diff, use zend_hash_discard
iliaal Mar 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 32 additions & 23 deletions Zend/zend_compile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9654,35 +9654,44 @@ static void zend_compile_class_decl(znode *result, const zend_ast *ast, bool top
/* See zend_link_hooked_object_iter(). */
&& !ce->num_hooked_props
#endif
&& !(CG(compiler_options) & ZEND_COMPILE_WITHOUT_EXECUTION)) {
if (toplevel) {
if (extends_ast) {
zend_class_entry *parent_ce = zend_lookup_class_ex(
ce->parent_name, NULL, ZEND_FETCH_CLASS_NO_AUTOLOAD);

if (parent_ce
&& !zend_compile_ignore_class(parent_ce, ce->info.user.filename)) {
if (zend_try_early_bind(ce, parent_ce, lcname, NULL)) {
zend_string_release(lcname);
return;
) {
if (!(CG(compiler_options) & ZEND_COMPILE_WITHOUT_EXECUTION)) {
if (toplevel) {
if (extends_ast) {
zend_class_entry *parent_ce = zend_lookup_class_ex(
ce->parent_name, NULL, ZEND_FETCH_CLASS_NO_AUTOLOAD);

if (parent_ce
&& !zend_compile_ignore_class(parent_ce, ce->info.user.filename)) {
if (zend_try_early_bind(ce, parent_ce, lcname, NULL)) {
zend_string_release(lcname);
return;
}
}
} else if (EXPECTED(zend_hash_add_ptr(CG(class_table), lcname, ce) != NULL)) {
zend_string_release(lcname);
zend_build_properties_info_table(ce);
zend_inheritance_check_override(ce);
ce->ce_flags |= ZEND_ACC_LINKED;
zend_observer_class_linked_notify(ce, lcname);
return;
} else {
goto link_unbound;
}
} else if (EXPECTED(zend_hash_add_ptr(CG(class_table), lcname, ce) != NULL)) {
zend_string_release(lcname);
} else if (!extends_ast) {
link_unbound:
/* Link unbound simple class */
zend_build_properties_info_table(ce);
zend_inheritance_check_override(ce);
ce->ce_flags |= ZEND_ACC_LINKED;
zend_observer_class_linked_notify(ce, lcname);
return;
} else {
goto link_unbound;
}
} else if (!extends_ast) {
link_unbound:
/* Link unbound simple class */
zend_build_properties_info_table(ce);
zend_inheritance_check_override(ce);
ce->ce_flags |= ZEND_ACC_LINKED;
} else if (!extends_ast
&& !(CG(compiler_options) & ZEND_COMPILE_PRELOAD)) {
/* When compiling without execution (opcache_compile_file),
* link simple classes without parents so opcache can
* early-bind them when loaded from cache. Skip during
* preloading which has its own linking pipeline. */
goto link_unbound;
}
}

Expand Down
28 changes: 28 additions & 0 deletions ext/opcache/tests/gh18714.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
--TEST--
GH-18714 (opcache_compile_file() breaks class hoisting)
--EXTENSIONS--
opcache
--INI--
opcache.enable=1
opcache.enable_cli=1
--FILE--
<?php

// Class used before declaration relies on hoisting
file_put_contents(__DIR__ . '/gh18714_test.php', <<<'PHP'
<?php
$x = new HelloWorld();
echo get_class($x) . "\n";
class HelloWorld {}
PHP);

opcache_compile_file(__DIR__ . '/gh18714_test.php');
require_once __DIR__ . '/gh18714_test.php';

?>
--CLEAN--
<?php
@unlink(__DIR__ . '/gh18714_test.php');
?>
--EXPECT--
HelloWorld
30 changes: 30 additions & 0 deletions ext/opcache/zend_accelerator_module.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -961,6 +961,23 @@ ZEND_FUNCTION(opcache_jit_blacklist)
#endif
}

/* Remove hash table entries appended after orig_count without calling the
* destructor, since these point to SHM-backed data we don't own. */
static void accel_rollback_hash(HashTable *ht, uint32_t orig_count)
{
dtor_func_t orig_dtor = ht->pDestructor;
ht->pDestructor = NULL;
while (ht->nNumUsed > orig_count) {
Bucket *p = &ht->arData[ht->nNumUsed - 1];
if (EXPECTED(Z_TYPE(p->val) != IS_UNDEF)) {
zend_hash_del_bucket(ht, p);
} else {
ht->nNumUsed--;
}
}
ht->pDestructor = orig_dtor;
}

Copy link
Copy Markdown
Member

@dstogov dstogov Mar 23, 2026

Choose a reason for hiding this comment

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

You can use ZEND_HASH_MAP_FOREACH_END_DEL. See examples in Zend/zend_execute_API.c and ext/opcache/ZendAccelerator.c.

Copy link
Copy Markdown
Contributor Author

@iliaal iliaal Mar 23, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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.

ZEND_FUNCTION(opcache_compile_file)
{
zend_string *script_name;
Expand All @@ -984,6 +1001,11 @@ ZEND_FUNCTION(opcache_compile_file)
orig_compiler_options = CG(compiler_options);
CG(compiler_options) |= ZEND_COMPILE_WITHOUT_EXECUTION;

/* Save class/function table state so we can undo the side effects
* of zend_accel_load_script() called by persistent_compile_file(). */
uint32_t orig_class_count = EG(class_table)->nNumUsed;
uint32_t orig_function_count = EG(function_table)->nNumUsed;

if (CG(compiler_options) & ZEND_COMPILE_PRELOAD) {
/* During preloading, a failure in opcache_compile_file() should result in an overall
* preloading failure. Otherwise we may include partially compiled files in the preload
Expand All @@ -1001,6 +1023,14 @@ ZEND_FUNCTION(opcache_compile_file)
CG(compiler_options) = orig_compiler_options;

if(op_array != NULL) {
/* 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);
}
Comment on lines +1009 to +1015
Copy link
Copy Markdown
Member

@dstogov dstogov Mar 23, 2026

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

@iliaal iliaal Mar 23, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@dstogov dstogov Apr 13, 2026

Choose a reason for hiding this comment

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

In case we change the existing behavior, we almost necessary break some code, that relays on it.
I would prefer keeping old behavior and add a changed one.


destroy_op_array(op_array);
efree(op_array);
RETVAL_TRUE;
Expand Down
Loading