Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:126027
Precedence: bulk
MIME-Version: 1.0
References: <CAPyj-LD+X-XZoc5kZmW2qTCuu1-q5NtqEZyZhEs0rhvXa2AyJg@mail.gmail.com>
In-Reply-To: <CAPyj-LD+X-XZoc5kZmW2qTCuu1-q5NtqEZyZhEs0rhvXa2AyJg@mail.gmail.com>
Date: Thu, 21 Nov 2024 15:05:56 +0000
Message-ID: <CAMhsXHydJxS-N1gpZpWVuvzjPcRvJ1O0zQZnwa=nCqQ=WZP+Cg@mail.gmail.com>
Subject: Re: [PHP-DEV] opcache_compile_file() declares top-level functions
To: Ilija Tovilo <tovilo.ilija@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
From: sam@melroseandco.uk (Samuel Melrose)

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

On Wed, Nov 20, 2024 at 9:24=E2=80=AFPM Ilija Tovilo <tovilo.ilija@gmail.co=
m> wrote:
>
> 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 wit=
hout executing it. This can be used to prime the cache after a Web server r=
estart 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.
>
> ```php
> // 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.
>
> ```php
> // 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.
>
> ```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();
>
> // 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.
>
> ```php
> // 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