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 all commits
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
14 changes: 14 additions & 0 deletions Zend/zend_compile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9686,6 +9686,20 @@ static void zend_compile_class_decl(znode *result, const zend_ast *ast, bool top
}
}

/* When compiling without execution (opcache_compile_file), link simple
* classes so opcache can early-bind them from cache. Skip preloading. */
if (!ce->num_interfaces && !ce->num_traits && !ce->num_hooked_prop_variance_checks
#ifdef ZEND_OPCACHE_SHM_REATTACHMENT
&& !ce->num_hooked_props
#endif
&& !extends_ast
&& (CG(compiler_options) & ZEND_COMPILE_WITHOUT_EXECUTION)
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.

I would move the check for CG(compiler_options) & ZEND_COMPILE_WITHOUT_EXECUTION up.
I assume, this is a rare case and we may avoid all the checks above.

&& !(CG(compiler_options) & ZEND_COMPILE_PRELOAD)) {
zend_build_properties_info_table(ce);
zend_inheritance_check_override(ce);
ce->ce_flags |= ZEND_ACC_LINKED;
}

Comment on lines +9689 to +9702
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.

I didn't understand why do we need this.

opline = get_next_op();

if (ce->parent_name) {
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
13 changes: 13 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 @@ -984,6 +984,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 +1006,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)) {
zend_hash_discard(EG(class_table), orig_class_count);
zend_hash_discard(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