Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:126015
Content-Type: text/plain;
	charset=us-ascii
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.200.121\))
Subject: Re: [PHP-DEV] opcache_compile_file() declares top-level functions
Date: Thu, 21 Nov 2024 09:15:12 +0100
References: <CAPyj-LD+X-XZoc5kZmW2qTCuu1-q5NtqEZyZhEs0rhvXa2AyJg@mail.gmail.com>
To: internals@lists.php.net
In-Reply-To: <CAPyj-LD+X-XZoc5kZmW2qTCuu1-q5NtqEZyZhEs0rhvXa2AyJg@mail.gmail.com>
Message-ID: <A4F812D1-2C6F-4621-A663-3B98A5F6C8E1@gmail.com>
From: daniil.gentili@gmail.com (Daniil Gentili)

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

> On 20 Nov 2024, at 22:22, Ilija Tovilo <tovilo.ilija@gmail.com> wrote:
>=20
> Hi everyone
>=20
> We recently received a bug report regarding the behavior of
> opcache_compile_file() [1]. The documentation specifies:
>=20
> https://www.php.net/manual/en/function.opcache-compile-file.php
>=20
>> 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.
>=20
> 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:
>=20
> * 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.
>=20
> Confusingly, top-level functions _will_ be added to the function
> table. This has some weird consequences:
>=20
> * 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.
>=20
> ```php
> // index.php
> opcache_compile_file(__DIR__ . '/test.php');
> opcache_compile_file(__DIR__ . '/test.php');
>=20
> // test.php
> class Foo {} // No problem
> function foo() {} // Fatal error: Cannot redeclare function foo()
> ```
>=20
> * 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.
>=20
> ```php
> // index.php
> opcache_compile_file(__DIR__ . '/test.php');
> require __DIR__ . '/test.php';
>=20
> // test.php
> class Foo {} // No problem
> function foo() {} // Fatal error: Cannot redeclare function foo()
> ```
>=20
> * 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.
>=20
> ```php
> // index.php
> spl_autoload_register(function ($name) {
>    if ($name =3D=3D=3D 'Foo') {
>        require __DIR__ . '/b.php';
>    }
> });
> opcache_compile_file(__DIR__ . '/test.php');
> foo();
>=20
> // test.php
> class Foo {}
> function foo() {
>    // Triggers the autoloader, the autoloader fails due to:
>    // Fatal error: Cannot redeclare function foo()
>    var_dump(new Foo());
> }
> ```
>=20
> * 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.
>=20
> ```php
> // index.php
> opcache_compile_file(__DIR__ . '/test.php');
> foo(); // Uncaught Error: Call to undefined function foo()
>=20
> // test.php
> if (true) {
>    function foo() {}
> }
> ```
>=20
> 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.
>=20
> Are there any concerns with making this change for 8.5? Are there any
> use-cases this would break?
>=20
> 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].
>=20
> Ilija
>=20
> [1] https://github.com/php/php-src/issues/16668
> [2] https://github.com/php/php-src/pull/16862
> [3] https://github.com/Roave/BetterReflection