Skip to content
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.

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

Usage of global constants breaks apps using PHP 7.4s class preloading #20

Open
spackmat opened this issue Dec 8, 2020 · 4 comments
Open

Comments

@spackmat
Copy link

spackmat commented Dec 8, 2020

Hi, I enabled PHP class preloading for my Symfony based app. But when it comes to creating files with TBS/OpenTBS, I get an error "Undefined constant 'TBS_INSTALL'". After digging in, I realized that preloading and global constants defined at runtime with define() won't work together: they are simply not defined when their class is preloaded.

Can anyone confirm that? Any ideas to solve that (besides refactoring the constants-system of TBS/OpenTBS creating a BC break)?

I tried to exclude the (service) class using the TBS_INSTALL constant from preloading, but that doesn't solve the problem (as it and the TBS classes are still preloaded as a dependency).

How to reproduce

Just try to use TBS/OpenTBS in an application with PHP preloading. In opcache_get_status()['preload_statistics']['classes'] you get a list of preloaded classes where clsTinyButStrong, clsTbsDataSource and clsTbsLocator are included, provoking the error.

@roxblnfk
Copy link
Contributor

roxblnfk commented Dec 8, 2020

@Skrol29 what do you think about releasing a new major version of the TBS package, which will support minimum PHP 7.4?

This can be done with preservation of API and compatibility (using a decorators). This will have modern code that adheres to PSR, with phpunit 9 for testing, etc.

@Skrol29
Copy link
Owner

Skrol29 commented Dec 15, 2020

Hi,

Sorry I need more time to reproduce this issue since Preloading is not available on PHP for Windows for now. (shame on me, I use Windows). My Linux environment is far from here.

It’s surprising that constants are ignored with preloading. The documentation is unclear on this point :

Any symbols (functions, classes, etc.) in those files will then become globally available

I wonder if PHP 8 has the same behavior.

Nevertheless I agree the next major TBS version should avoid named constants (define) and use Class constants instead.

A waiting solution could be to have TBS and OpenTBS constants defined only once dynamically at the first instantation of the class.
Does such a solution sounds ok for you ?

@Skrol29
Copy link
Owner

Skrol29 commented Dec 15, 2020

@Skrol29 what do you think about releasing a new major version of the TBS package, which will support minimum PHP 7.4?
This can be done with preservation of API and compatibility (using a decorators). This will have modern code that adheres to PSR, with phpunit 9 for testing, etc.

Having a major version suitable for PHP 7 and modern coding is a very nice idea which I often think of.
I also think about leaving unused features (such as Assigned merging for which I have zero feedback) ; add new a barrel feature for the Engine in order to have it processing sub-templates in the same way as OpenTBS ; and some few renaming.
So it is not clear for me to have all this in one major version or several.

@spackmat
Copy link
Author

@Skrol29 I was also surprised, but it is consequent as define() is a function called at runtime and preloading doesn't run any code, but only compiles the symbols and keeps them in memory. So code in preloaded files outside those symbols will never run, because the actual files are not required/included anymore.

A waiting solution could be to have TBS and OpenTBS constants defined only once dynamically at the first instantation of the class. Does such a solution sounds ok for you ?

You mean inside the constructor of the main classes with a check like if (!defined('MY_GLOBAL_CONSTANT')){define('MY_GLOBAL_CONSTANT', 'MY_VALUE');}? Could work.

My devsystem is also on windows, so I see that problem only in staging/production. Nice surprise. ;)

Besides that: A new major version with contemporary coding is always a good idea.

@roxblnfk roxblnfk mentioned this issue Dec 16, 2020
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants