opcache_compile_file() declares top-level functions

1 year ago by Ilija Tovilo — view source — reply

unread

Hi everyone

We recently received a bug report regarding the behavior of
opcache_compile_file() [1]. The documentation specifies:

https://www.php.net/manual/en/function.opcache-compile-file.php

This function compiles a PHP script and adds it to the opcode cache without executing it. This can be used to prime the cache after a Web server restart by pre-caching files that will be included in later requests.

Arguably, "without executing it" implies that, aside from putting the
script into opcache, there are no other observable side-effects. This
assumption is currently incorrect. To be more specific,
opcache_compile_file() differs from require in two ways:

The "main" function of the script (containing all the code at the
top-level) is not executed.
Classes will not be added to the class table.

Confusingly, top-level functions will be added to the function
table. This has some weird consequences:

opcache_compile_file() can be called multiple times on files
containing classes. However, the same is not true for files containing
functions. The second call will lead to a function redeclaration
error.

// index.php
opcache_compile_file(__DIR__ . '/test.php');
opcache_compile_file(__DIR__ . '/test.php');

// test.php
class Foo {} // No problem
function foo() {} // Fatal error: Cannot redeclare function foo()

Similarly, after calling opcache_compile_file() on files containing
classes, the same file may later be required without issues. This does
not work for files containing functions for the same reason.

// index.php
opcache_compile_file(__DIR__ . '/test.php');
require __DIR__ . '/test.php';

// test.php
class Foo {} // No problem
function foo() {} // Fatal error: Cannot redeclare function foo()

Mixing functions with classes is incompatible. An attempt to use one
of the classes from one of the functions will either error because of
an undeclared class, or trigger the autoloader and include the file
again, leading to a function redeclaration error.

// index.php
spl_autoload_register(function ($name) {
    if ($name === 'Foo') {
        require __DIR__ . '/b.php';
    }
});
opcache_compile_file(__DIR__ . '/test.php');
foo();

// test.php
class Foo {}
function foo() {
    // Triggers the autoloader, the autoloader fails due to:
    // Fatal error: Cannot redeclare function foo()
    var_dump(new Foo());
}

Functions that are conditionally declared will not be added to the
function table, since they are not top-level functions, even if the
condition always evaluates to true.

// index.php
opcache_compile_file(__DIR__ . '/test.php');
foo(); // Uncaught Error: Call to undefined function foo()

// test.php
if (true) {
    function foo() {}
}

This behavior is inconsistent and confusing. Arguably, the correct
behavior is to never register functions in opcache_compile_file() to
begin with, since opcache_compile_file() exists to prime the cache,
rather than execute code. I created a PR with this change [2]. This is
breaking, since code using opcache_compile_file() might currently
depend on functions being declared and then calling them directly.
Such code would have to be adjusted to use require instead.

Are there any concerns with making this change for 8.5? Are there any
use-cases this would break?

Of note is that it may be beneficial to provide a related function
that allows compiling files and declaring symbols without executing
the "main" function. This could be useful static analysis tools that
want to reflect on files without executing them. That said, there are
existing solutions in userland that solve this problem in a better
way, like BetterReflection [3].

Ilija

[1] https://github.com/php/php-src/issues/16668
[2] https://github.com/php/php-src/pull/16862
[3] https://github.com/Roave/BetterReflection

1 year ago by Daniil Gentili — view source — reply

unread

Hi Ilija,
I believe the removal of function caching is a bit of an overreaction: it was always known, to those who used it, that preloading will cache functions, and requires an include guard to avoid function redeclaration errors, and in fact it is a very useful feature.

There is nothing wrong with this behaviour, and it does what you expect it to do.

Regards,
Daniil Gentili

Hi everyone

We recently received a bug report regarding the behavior of
opcache_compile_file() [1]. The documentation specifies:

https://www.php.net/manual/en/function.opcache-compile-file.php

This function compiles a PHP script and adds it to the opcode cache without executing it. This can be used to prime the cache after a Web server restart by pre-caching files that will be included in later requests.

Arguably, "without executing it" implies that, aside from putting the
script into opcache, there are no other observable side-effects. This
assumption is currently incorrect. To be more specific,
opcache_compile_file() differs from require in two ways:

The "main" function of the script (containing all the code at the
top-level) is not executed.

Classes will not be added to the class table.

Confusingly, top-level functions will be added to the function
table. This has some weird consequences:

opcache_compile_file() can be called multiple times on files
containing classes. However, the same is not true for files containing
functions. The second call will lead to a function redeclaration
error.
// index.php
opcache_compile_file(__DIR__ . '/test.php');
opcache_compile_file(__DIR__ . '/test.php');

// test.php
class Foo {} // No problem
function foo() {} // Fatal error: Cannot redeclare function foo()
Similarly, after calling opcache_compile_file() on files containing
classes, the same file may later be required without issues. This does
not work for files containing functions for the same reason.
// index.php
opcache_compile_file(__DIR__ . '/test.php');
require __DIR__ . '/test.php';

// test.php
class Foo {} // No problem
function foo() {} // Fatal error: Cannot redeclare function foo()
Mixing functions with classes is incompatible. An attempt to use one
of the classes from one of the functions will either error because of
an undeclared class, or trigger the autoloader and include the file
again, leading to a function redeclaration error.
// index.php
spl_autoload_register(function ($name) {
   if ($name === 'Foo') {
       require __DIR__ . '/b.php';
   }
});
opcache_compile_file(__DIR__ . '/test.php');
foo();

// test.php
class Foo {}
function foo() {
   // Triggers the autoloader, the autoloader fails due to:
   // Fatal error: Cannot redeclare function foo()
   var_dump(new Foo());
}
Functions that are conditionally declared will not be added to the
function table, since they are not top-level functions, even if the
condition always evaluates to true.
// index.php
opcache_compile_file(__DIR__ . '/test.php');
foo(); // Uncaught Error: Call to undefined function foo()

// test.php
if (true) {
   function foo() {}
}
This behavior is inconsistent and confusing. Arguably, the correct
behavior is to never register functions in opcache_compile_file() to
begin with, since opcache_compile_file() exists to prime the cache,
rather than execute code. I created a PR with this change [2]. This is
breaking, since code using opcache_compile_file() might currently
depend on functions being declared and then calling them directly.
Such code would have to be adjusted to use require instead.

Are there any concerns with making this change for 8.5? Are there any
use-cases this would break?

Of note is that it may be beneficial to provide a related function
that allows compiling files and declaring symbols without executing
the "main" function. This could be useful static analysis tools that
want to reflect on files without executing them. That said, there are
existing solutions in userland that solve this problem in a better
way, like BetterReflection [3].

Ilija

[1] https://github.com/php/php-src/issues/16668
[2] https://github.com/php/php-src/pull/16862
[3] https://github.com/Roave/BetterReflection

1 year ago by Rowan Tommins [IMSoP] — view source — reply

unread

Hi Ilija,
I believe the removal of function caching is a bit of an overreaction: it was always known, to those who used it, that preloading will cache functions, and requires an include guard to avoid function redeclaration errors, and in fact it is a very useful feature.

There is nothing wrong with this behaviour, and it does what you expect it to do.

This reads rather awkwardly, because you're claiming to speak for some unknown users ("those who used it") and even for the reader of the message ("it does what you expect it to do").

More helpful would be to say how you use it, or where you have seen it used. Evidently it wasn't obvious to Ilija, and wasn't what they expected. Since it's not mentioned in the manual, it's not what I would have expected either.

Ilija also went to some length to describe real practical problems, so just saying there's "nothing wrong with this behaviour" also seems unhelpful. Perhaps you could expand on why you think the problems illustrated aren't significant in practice?

I would also be interested to know if you think the difference in behaviour between classes and functions is explicitly useful, or just neutral to your use case.

Regards,
Rowan Tommins
[IMSoP]

1 year ago by Daniil Gentili — view source — reply

unread

This reads rather awkwardly, because you're claiming to speak for some unknown users ("those who used it") and even for the reader of the message ("it does what you expect it to do").

I speak for myself (and some others, as can be seen by pull requests on some FOSS projects, which made pull requests to account for this behaviour), as a user of preloading who has encountered this behaviour, understood the reason for it and made the required changes to keep using it.

More helpful would be to say how you use it, or where you have seen it used. Evidently it wasn't obvious to Ilija, and wasn't what they expected. Since it's not mentioned in the manual, it's not what I would have expected either.

It was indeed not mentioned in the manual, and preloading is used so rarely (as you can see, not even Ilija has been using it) that it seems no one even thought of updating it :)

Ilija also went to some length to describe real practical problems, so just saying there's "nothing wrong with this behaviour" also seems unhelpful. Perhaps you could expand on why you think the problems illustrated aren't significant in practice?
I would also be interested to know if you think the difference in behaviour between classes and functions is explicitly useful, or just neutral to your use case.

There is indeed nothing wrong with the listed behaviours IMO, as they make sense for how the current logic is working, and aren’t a deal breaker for preloading.

In fact preloading in a way behaves as if you included the preload file before including the entry point, and doing that even without preloading would issue the same behaviour: functions cannot be declared twice, and already-declared classes are not autoloaded again, so no error is emitted (when preloading no error would be emitted anyway for redeclaration of classes, but in the vast majority of cases when using composer already declared classes are not re-included by the autoloader, so the end result is the same).

If functions could be autoloaded this edge case could be avoided without having to add include guards, but alternatively, it might be a nice idea to simply ignore the redeclaration of functions (like for classes), instead of not preloading them at all.

Regards,
Daniil Gentili.

1 year ago by Rowan Tommins [IMSoP] — view source — reply

unread

I speak for myself (and some others, as can be seen by pull requests on some FOSS projects, which made pull requests to account for this behaviour), as a user of preloading who has encountered this behaviour, understood the reason for it and made the required changes to keep using it.

This is still painfully vague. What projects? What "reason for it" did you understand? When you say projects accounted for it, do you mean they got some benefit from it, or that they worked around the problems it caused them?

There is indeed nothing wrong with the listed behaviours IMO, as they make sense for how the current logic is working, and aren’t a deal breaker for preloading.

This seems to be entirely circular - "the current behaviour makes sense for the current behaviour".

In fact preloading in a way behaves as if you included the preload file before including the entry point

Note that we are not talking about preloading, as a general concept; we are talking about the specific function opcache_compile_file. That function's explicit purpose is to prime the opcache without behaving the same way as including the file.

functions cannot be declared twice, and already-declared classes are not autoloaded again

Finally, we seem to get to the use case you are actually advocating: precompiling classes but using an autoloader to actually declare them; but fully pre-declaring functions, because there's no autoloader for them.

That certainly makes sense as a distinction, but I then wonder why you would use opcache_compile_file for declaring those functions, rather than a normal include. Since you can't include the file a second time either way, is there any way to make use of the difference?

it might be a nice idea to simply ignore the redeclaration of functions (like for classes), instead of not preloading them at all.

There is nothing that ignores the redeclaration of classes; they are not declared when pre-compiling, and can be declared exactly once by passing include/require the pre-compiled file. Making functions match that behaviour is what Ilija is proposing.

Rowan Tommins
[IMSoP]

1 year ago by Rob Landers — view source — reply

unread

I speak for myself (and some others, as can be seen by pull requests on some FOSS projects, which made pull requests to account for this behaviour), as a user of preloading who has encountered this behaviour, understood the reason for it and made the required changes to keep using it.

This is still painfully vague. What projects? What "reason for it" did you understand? When you say projects accounted for it, do you mean they got some benefit from it, or that they worked around the problems it caused them?

There is indeed nothing wrong with the listed behaviours IMO, as they make sense for how the current logic is working, and aren’t a deal breaker for preloading.

This seems to be entirely circular - "the current behaviour makes sense for the current behaviour".

In fact preloading in a way behaves as if you included the preload file before including the entry point

Note that we are not talking about preloading, as a general concept; we are talking about the specific function opcache_compile_file. That function's explicit purpose is to prime the opcache without behaving the same way as including the file.

functions cannot be declared twice, and already-declared classes are not autoloaded again

Finally, we seem to get to the use case you are actually advocating: precompiling classes but using an autoloader to actually declare them; but fully pre-declaring functions, because there's no autoloader for them.

That certainly makes sense as a distinction, but I then wonder why you would use opcache_compile_file for declaring those functions, rather than a normal include. Since you can't include the file a second time either way, is there any way to make use of the difference?

it might be a nice idea to simply ignore the redeclaration of functions (like for classes), instead of not preloading them at all.

There is nothing that ignores the redeclaration of classes; they are not declared when pre-compiling, and can be declared exactly once by passing include/require the pre-compiled file. Making functions match that behaviour is what Ilija is proposing.

Rowan Tommins
[IMSoP]

Seems like function autoloading would alleviate this use case and make this behavior easier to deprecate.

— Rob

1 year ago by Daniil Gentili — view source — reply

unread

I speak for myself (and some others, as can be seen by pull requests on some FOSS projects, which made pull requests to account for this behaviour), as a user of preloading who has encountered this behaviour, understood the reason for it and made the required changes to keep using it.

This is still painfully vague. What projects? What "reason for it" did you understand? When you say projects accounted for it, do you mean they got some benefit from it, or that they worked around the problems it caused them?

I confused the linked issue with another issue related to preloading that I encountered and worked around with include guards (https://github.com/amphp/amp/issues/321).

Note that we are not talking about preloading, as a general concept; we are talking about the specific function opcache_compile_file. That function's explicit purpose is to prime the opcache without behaving the same way as including the file.

Yep, this makes sense, completely missed that when reading the initial issue :)

Seems like function autoloading would alleviate this use case and make this behavior easier to deprecate.

+++

1 year ago by Samuel Melrose — view source — reply

unread

Thanks for reporting this Ilija,

I came across the same problem when using opcache_compile_file, and
agree it's confusing.

For an example, please see the code sample in the opening comment of my PR:

https://github.com/php/php-src/pull/16551

As part of my change to allow a read-only file based opcache, for use
with Docker, I'm using opcache_compile_file on all files found with
Symphony's Finder, although I had to force composer's autoloader not
to load in the usual global scripts (i.e. that define helper
functions), as I was getting the problem you describe around double
declaration.

I'd be very happy with your change to the behaviour, and if it lands
in 8.5, it would hopefully complement my change which should have more
people using opcache_compile_file with/in Docker container builds.

Regards,
Samuel Melrose

Hi everyone

We recently received a bug report regarding the behavior of
opcache_compile_file() [1]. The documentation specifies:

https://www.php.net/manual/en/function.opcache-compile-file.php

This function compiles a PHP script and adds it to the opcode cache without executing it. This can be used to prime the cache after a Web server restart by pre-caching files that will be included in later requests.

Arguably, "without executing it" implies that, aside from putting the
script into opcache, there are no other observable side-effects. This
assumption is currently incorrect. To be more specific,
opcache_compile_file() differs from require in two ways:

The "main" function of the script (containing all the code at the
top-level) is not executed.

Classes will not be added to the class table.

Confusingly, top-level functions will be added to the function
table. This has some weird consequences:

opcache_compile_file() can be called multiple times on files
containing classes. However, the same is not true for files containing
functions. The second call will lead to a function redeclaration
error.
// index.php
opcache_compile_file(__DIR__ . '/test.php');
opcache_compile_file(__DIR__ . '/test.php');

// test.php
class Foo {} // No problem
function foo() {} // Fatal error: Cannot redeclare function foo()
Similarly, after calling opcache_compile_file() on files containing
classes, the same file may later be required without issues. This does
not work for files containing functions for the same reason.
// index.php
opcache_compile_file(__DIR__ . '/test.php');
require __DIR__ . '/test.php';

// test.php
class Foo {} // No problem
function foo() {} // Fatal error: Cannot redeclare function foo()
Mixing functions with classes is incompatible. An attempt to use one
of the classes from one of the functions will either error because of
an undeclared class, or trigger the autoloader and include the file
again, leading to a function redeclaration error.
// index.php
spl_autoload_register(function ($name) {
    if ($name === 'Foo') {
        require __DIR__ . '/b.php';
    }
});
opcache_compile_file(__DIR__ . '/test.php');
foo();

// test.php
class Foo {}
function foo() {
    // Triggers the autoloader, the autoloader fails due to:
    // Fatal error: Cannot redeclare function foo()
    var_dump(new Foo());
}
Functions that are conditionally declared will not be added to the
function table, since they are not top-level functions, even if the
condition always evaluates to true.
// index.php
opcache_compile_file(__DIR__ . '/test.php');
foo(); // Uncaught Error: Call to undefined function foo()

// test.php
if (true) {
    function foo() {}
}
This behavior is inconsistent and confusing. Arguably, the correct
behavior is to never register functions in opcache_compile_file() to
begin with, since opcache_compile_file() exists to prime the cache,
rather than execute code. I created a PR with this change [2]. This is
breaking, since code using opcache_compile_file() might currently
depend on functions being declared and then calling them directly.
Such code would have to be adjusted to use require instead.

Are there any concerns with making this change for 8.5? Are there any
use-cases this would break?

Of note is that it may be beneficial to provide a related function
that allows compiling files and declaring symbols without executing
the "main" function. This could be useful static analysis tools that
want to reflect on files without executing them. That said, there are
existing solutions in userland that solve this problem in a better
way, like BetterReflection [3].

Ilija

[1] https://github.com/php/php-src/issues/16668
[2] https://github.com/php/php-src/pull/16862
[3] https://github.com/Roave/BetterReflection