Compatibility with 7.4 preloading · Issue #226 · nette/di · GitHub
source link: https://github.com/nette/di/issues/226
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upCompatibility with 7.4 preloading #226
Closed
brendt opened this issue on Dec 21, 2019 · 41 comments
Comments
There's an issue with this package when using PHP 7.4's preloading. More specifically with this file: https://github.com/nette/di/blob/master/src/compatibility.php Because these class aliases are defined unconditionally, they will throw "previously declared class warnings" if the file was preloaded. More info: https://mobile.twitter.com/nikita_ppv/status/1208025415253659648 |
Member
dg commented on Dec 21, 2019
I dont understand what to do. |
Contributor
mabar commented on Dec 21, 2019
Extend class to only load it if used could work, I guess
|
Contributor
JanTvrdik commented on Dec 24, 2019
Defining the aliases conditionally should work, i.e. if (!class_exists(Nette\DI\Config\IAdapter::class, false)) { class_alias(Nette\DI\Config\Adapter::class, Nette\DI\Config\IAdapter::class); } ... |
Member
dg commented on Dec 24, 2019
@JanTvrdik How is it possible that a class |
Contributor
mabar commented on Dec 24, 2019
It's a preloading feature. All preloaded classes and functions are available across requests, so you don't need to load them every single time. |
So the problem is that the code on the page https://stitcher.io/blog/preloading-in-php-74 loads all the files in repo and then one file Guys, I really don't like issues written as a puzzle that I have to solve to understand the task. |
Above all, loading all Condition is not a solution, fix a preloading script. For example let the Composer create a list of classes and preload them:
$list = require 'autoload_classmap.php'; $list = array_filter($list, fn($file) => strpos($file, '/vendor/xxx/') !== false); ... |
Hi David, thanks for looking into this. When I wrote this issue I was short on time and I assumed preloading was already a known concept for most open source maintainers. So let me clarify: preloading is a core feature as of PHP 7.4, and has nothing to do with composer. It can significantly reduce request startup time, especially for projects using frameworks. While optimised preload lists are the ideal solution, they don't offer much more performance gain compared to a "dumb" preloading list which preloads all classes of a given framework. You can read about these benchmarks on the composer repository: composer/composer#7777 (comment) My preload script will try to preload all of Laravel's classes, and it so happens that, because of dev dependencies, nette/bootstrap is also loaded (required by phpstan/phpstan). Obviously I can probably work around the issue, but I figured it would be a small effort to add a This probably won't be the last issue being submitted here by people who want to use a simple preload script to preload their framework, having phpstan installed and getting an error. Most of these people will submit an issue here, requiring some of your time. I think it's in both our interests to prevent this error from happening again — even though there might be other solutions. Being an open source maintainer myself, I prefer to make things fool proof on my end, so that I don't have to deal with the same issues over and over again. |
I think I understand how preloading works. But it is not about preloading and it is not about Composer. It is about some specific code that preloads classes. And I don't know the code. I think there is no standard preloader. The preloader mentioned by @mabar has issue - it simply loads all the files. It does not scale. Because loading all files is not a good idea at all. There will be more and more libraries with the same issues. Libraries with examples or tests or make-scripts inside its repo. Preloading script can't just load all them files. You can't even write to all library authors: fix it. Preloading script must be fixed. And I will be happy to help find a way to fix that script. But since I'm a programmer who wants to do things right, I want to fix a bug, not its consequences. |
Which files are loaded or not is not the issue though. We could write preload scripts that don't use composer's autoloader, and manually resolve all dependencies, but that's a lot of work for frameworks, and not worth the time. The fact is simply this: if someone wants to use preloading combined with composer's autoloader (which does 99% of the work for you), and has a dependency on If you're fine with that, then keep the code as is; but since it's a simple fix (semantically more correct in my opinion), I don't see any reason not to add it. |
FYI the solution shared by @JanTvrdik wasn't 100% correct, since if (!interface_exists(Nette\DI\Config\IAdapter::class)) { class_alias(Nette\DI\Config\Adapter::class, Nette\DI\Config\IAdapter::class); } if (!class_exists(Nette\DI\Statement::class)) { class_alias(Nette\DI\Definitions\Statement::class, Nette\DI\Statement::class); } if (!class_exists(Nette\DI\ServiceDefinition::class)) { class_alias(Nette\DI\Definitions\ServiceDefinition::class, Nette\DI\ServiceDefinition::class); } |
What is I will further assume that this is a problem between the class Preloader and nette/di. Caused by loading the This file contains no class so I am asking why Preloader is loading it? You said because it is "dumb" and preloads all classes of a given framework. If Preloader does not load this file, it will solve the problem. So the question is: what to fix? Preloader or nette/di. I say unambiguously: Preloader. Because there are lots of other libraries where the same problem arises. And it's better to repair the Preloader than to ask the authors of hundreds of libraries to modify their libraries for Preloader. Not for feature preloading, but for one class named Preloader. For example: inside Nette Tester https://github.com/nette/tester/tree/master/src there is file Which files are loaded or not is the issue. |
The question becomes: what the change? How preloading works in PHP's core, a core functionality of composer which, or a few simple lines in a package? Edit: my initial assumption of a "dumb" preloading script which loads all files was wrong, I wasn't aware that |
Simplest (not dumb) implementation of preloader would look for all php files matching config in packages composer.json autoload section (these which are used if I install package). No dev classes loaded at all. It's not so big performance gain as loading hotpath, but still ~10% which is a lot. But even using hotpath - if I have e.g. nextras/dbal compiler extension in a hotpath then I need to preload all it's dependencies, including nette/di compatibility.php. If compatibility.php is preloaded, then runtime is broken, because nette/di have in composer.json configuration which includes that file every single request and class is defined twice which php don't allow. |
Use OR can use? The second is a valid point and should be somehow addressed. |
Forgot about Preloader from stitcher.io please, it's naive implementation. Preloader can easily use only autoload section from composer.json which don't include tools and examples folders https://github.com/nette/tracy/blob/master/composer.json#L37-L40 |
Great. Preloader that will use autoload section from composer.json will have no issue. #226 (comment) |
Situation is as follows:
So… I think best solution is
Maybe this is the trick: <?php declare(strict_types=1); namespace Nette\DI; if (false) { class Statement { } } class_alias(Nette\DI\Definitions\Statement::class, Nette\DI\Statement::class); |
Hi David I'm sorry if the conversation got a little heated yesterday. I admit I was getting frustrated with how we didn't manage to address the actual problem. I hope we can start with a clean slate today would you mind elaborating on what you mean with this?
As mentioned before, composer will automatically load compatibility.php if it's listed as a Let me illustrate why composer's autoloader is such an asset: Say you've got two classes // src/A.php class A {} // src/B.php class B extends A {} If you want to preload Now imagine a framework with literally thousands of links between classes. Doing this manually is a very difficult task. It's also inefficient because framework updates might break your current preloading setup. That's why using composer's autoloader can be very helpful, it already has a classmap built-in, so that you don't have to build it yourself. In your opinion, do you think that |
I'm sorry if I'm repeating something that has already been said before, but there's one thing I want to clarify: The problem here is specifically that Nette performs class (alias) declarations unconditionally through the Composer autoloader. The problem is not that Nette contains a file somewhere that declares those. Of course a preloader is not going to load all random PHP files from a library! Nobody is going to preload your examples/ directory. Nobody is going to preload your test/ directory. Nobody is going to preload your bin/ directory. But at least the current standard preloading solutions in use right now (e.g. the preloader that ships with Symfony) will include the composer autoloader as part of the preloading script, and if just doing that already includes unconditional class declarations, then there's little you can do against that. The core issue here is really not so much that Nette declares these without doing a class_exists check first, but that it declares them as part of "files" at all. That means your classes are going to get loaded for everyone, even if they don't use Nette at all, or don't use it on a particular request. You can do that, but the general expectation people have from the library ecosystem is that this doesn't happen. Aliases should be declared the same way as all other classes: Through autoloading. That is, create a separate file for the alias like you would do for any other class, and declare the alias in there. |
Can I ask you for an example? I tried to use this solution a few days ago, I can't think of anything more elegant. |
@brendt I understand how preloading works and where the problem is. For me is very difficult to solve a problem with preloaders when no one wants to show me them :-) As I said in my first comment, smart should rely on composer's autoloader and not dumbly load all scripts. If it does, it cannot load the |
Contributor
ondrejmirtes commented on Jan 11
I think that the preloader should be able to safely load all classes based on PSR-4 and PSR-0 rules, classmap and files directives in composer.json. AFAIK compatibility.php is in files directive. |
@ondrejmirtes I wouldn't be sure about the files. Because it will certainly cause conflicts. It is usually used to define functions that are also preloaded. |
Contributor
ondrejmirtes commented on Jan 11
Are functions even part of preloading? If yes, then those files must be wrapped in |
I believe @nikic is suggesting to create separate file for each alias -> so that autoloader can correctly load needed alias on demand. (For example This comes with disadvantage that you are polluting your working directory with deprecated alias files. How about to create new
|
No one assigned
None yet
No milestone
Successfully merging a pull request may close this issue.
None yet
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK