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

Compatibility with 7.4 preloading #226

Closed
brendt opened this issue Dec 21, 2019 · 41 comments
Closed

Compatibility with 7.4 preloading #226

brendt opened this issue Dec 21, 2019 · 41 comments

Comments

@brendt
Copy link

@brendt brendt commented Dec 21, 2019

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

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Dec 21, 2019

I dont understand what to do.

@mabar

This comment has been minimized.

Copy link
Contributor

@mabar mabar commented Dec 21, 2019

Extend class to only load it if used could work, I guess

namespace Nette\DI;
/**
 * @deprecated
 */
class ServiceDefinition extends \Nette\DI\Definitions\ServiceDefinition {}
@JanTvrdik

This comment has been minimized.

Copy link
Contributor

@JanTvrdik JanTvrdik commented Dec 23, 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);
}

...
@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Dec 24, 2019

@JanTvrdik How is it possible that a class IAdapter exists?

@mabar

This comment has been minimized.

Copy link
Contributor

@mabar mabar commented 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.
https://stitcher.io/blog/preloading-in-php-74

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Dec 25, 2019

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 compatiblity.php is loaded by Composer?

Guys, I really don't like issues written as a puzzle that I have to solve to understand the task.

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Dec 25, 2019

Above all, loading all *.php files from the vendor/xxx directory is a short-sighted idea, because you are not sure that scripts do not have side effects (like script in nette/di has). Or this one https://github.com/nette/tracy/blob/master/tools/create-phar/create-phar.php.

Condition is not a solution, fix a preloading script. For example let the Composer create a list of classes and preload them:

composer dumpautoload --classmap-authoritative
$list = require 'autoload_classmap.php';
$list = array_filter($list, fn($file) => strpos($file, '/vendor/xxx/') !== false);
...
@brendt

This comment has been minimized.

Copy link
Author

@brendt brendt commented Jan 7, 2020

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 class_exists call. I'm perfectly fine with sending a PR myself.

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.

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jan 7, 2020

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.

@brendt

This comment has been minimized.

Copy link
Author

@brendt brendt commented Jan 7, 2020

We're not in the same page, and it's clear that you don't want to change your mind. I'll close this issue and work around it. Thanks for your time.

@brendt brendt closed this Jan 7, 2020
@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jan 7, 2020

I probably don't change the idea that loading all the *.php in a repository is a bad idea (do you really think it's good?), but I'm trying to be helpful in finding another general solution.

@brendt

This comment has been minimized.

Copy link
Author

@brendt brendt commented Jan 7, 2020

Which files are loaded or not is not the issue though. compatibility.php is configured to be autoloaded automatically: https://github.com/nette/di/blob/master/composer.json#L36

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 nette/di, they won't be able to do any preloading at all.

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.

@brendt

This comment has been minimized.

Copy link
Author

@brendt brendt commented Jan 7, 2020

FYI the solution shared by @JanTvrdik wasn't 100% correct, since Nette\DI\Config\IAdapter is an interface. This is a working solution:

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);
}
@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jan 7, 2020

The fact is simply this: if someone wants to use preloading combined with composer's autoloader

What is to use preloading? Do you mean to use preloading with Preloaded from stitcher.io/blog? If so, there is a conflict between the script Preloader and nette/di. But not between PHP 7.4 preloading feature and nette/di. (If not, please explain me how nette/di disallows preloading generally, I do not understand that).

I will further assume that this is a problem between the class Preloader and nette/di. Caused by loading the compatiblity.php file that has a side effect.

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 tester.php with die(). If someone wants to use Preloader (not preloading) and has a dependency on nette/tester, they won't be able to do it at all. Whose fault is it?

Which files are loaded or not is the issue.

@brendt

This comment has been minimized.

Copy link
Author

@brendt brendt commented Jan 7, 2020

This file contains no class so I am asking why Preloader is loading it?

If Preloader does not load this file, it will solve the problem.

  • PHP will preload every file that's loaded within the preload script, not just classes
  • Composer will autoload every file that's listed within the autoload.files entry, no way around that
  • Hence: combining preloading, composer and nette/di will not work

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 compatibility.php was listed as a file in composer.json, meaning you cannot prevent it from being autoloaded

@mabar

This comment has been minimized.

Copy link
Contributor

@mabar mabar commented Jan 7, 2020

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.

@mabar

This comment has been minimized.

Copy link
Contributor

@mabar mabar commented Jan 7, 2020

nette/tester tester.php would be never preloaded by that not dumb preloader as it's not a script used in production server runtime

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jan 7, 2020

Hence: combining preloading, composer and nette/di will not work

No! Combining of preloading via Preloader, composer and nette/di will not work.

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jan 7, 2020

@mabar 🤦‍♂️ I think you have a file E_COMPILE_ERROR.php on your production server which triggers E_COMPILE_ERROR. Preload it!

@mabar

This comment has been minimized.

Copy link
Contributor

@mabar mabar commented Jan 7, 2020

Combining of any preloader with composer and nette/di will not work as long as these compatibility classes need to be preloaded, because another preloaded file use one of these compatibility classes.

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jan 7, 2020

Combining of any preloader with composer and nette/di will not work as long as these compatibility classes need to be preloaded, because another preloaded file use one of these compatibility classes.

Use OR can use?

The second is a valid point and should be somehow addressed.

@mabar

This comment has been minimized.

Copy link
Contributor

@mabar mabar commented Jan 7, 2020

Can use. It's currently not possible to preload a class which uses one of these always required, non-conditionally defined classes.

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jan 7, 2020

Sure. But the issue is loading everything with a .php extension. I can add class_exist to nette/di, but try to preload Tracy for example. And there are a lot of Tracy extensions.

@mabar

This comment has been minimized.

Copy link
Contributor

@mabar mabar commented Jan 7, 2020

I can add class_exist to nette/di

Please do

but try to preload Tracy for example

Only always loaded file is shortcuts.php and all functions in it are already wrapped in function_exists(), so there should not be problems with preloading or did I miss something?

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jan 7, 2020

or did I miss something?

Yes, folders tools and examples.

@mabar

This comment has been minimized.

Copy link
Contributor

@mabar mabar commented Jan 7, 2020

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

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jan 7, 2020

Great. Preloader that will use autoload section from composer.json will have no issue. #226 (comment)

@mabar

This comment has been minimized.

Copy link
Contributor

@mabar mabar commented Jan 7, 2020

compatibility.php would still need define aliases conditionally, to be allowed preload these aliases (or classes which uses these aliases).

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jan 7, 2020

Yes, that's right.

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jan 7, 2020

Situation is as follows:

  • dump/naive preloaders need class_exists in compatibility.php, but dumb preloades means big troubles (Tester and die, etc…) (Yes, I am using sometimes Tester on prod)
  • really smart preloaders will ignore compatibility.php

So… I think best solution is

  • use smart preloaders
  • somehow fool composer to load compatibility.php on demand

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);
dg added a commit that referenced this issue Jan 7, 2020
dg added a commit that referenced this issue Jan 7, 2020
@brendt

This comment has been minimized.

Copy link
Author

@brendt brendt commented Jan 8, 2020

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?

So… I think best solution is
use smart preloaders

As mentioned before, composer will automatically load compatibility.php if it's listed as a autoload.files entry. So do you mean with smart preloaders that they shouldn't use composer's autoloader? Even when we're only preloading specific files?

Let me illustrate why composer's autoloader is such an asset:

Say you've got two classes A and B, and B extends from A:

// src/A.php
class A {}

// src/B.php
class B extends A {}

If you want to preload B, you also need to preload A, because B has a so called "link" to A. If you only preload src/B.php, you'll get a notice on server startup that PHP wasn't able to preload it, because it has unlinked dependencies.

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 smart preloaders shouldn't rely on composer's autoloader?

@nikic

This comment has been minimized.

Copy link

@nikic nikic commented Jan 8, 2020

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.

@nikic

This comment has been minimized.

Copy link

@nikic nikic commented Jan 8, 2020

TL;DR please only use "files" if you want to declare functions or constants, because those cannot be autoloaded. For class declarations, including class alias declarations, please prefer ordinary autoloading.

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jan 10, 2020

@nikic: 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.

dg added a commit that referenced this issue Jan 10, 2020
@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jan 10, 2020

@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 compatibility.php file, because it is not in Composer's metadata. On the contrary, only dump preloading script, that loads everything with .php extension loads this file.

@ondrejmirtes

This comment has been minimized.

Copy link
Contributor

@ondrejmirtes ondrejmirtes commented Jan 10, 2020

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.

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jan 10, 2020

@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.

@ondrejmirtes

This comment has been minimized.

Copy link
Contributor

@ondrejmirtes ondrejmirtes commented Jan 10, 2020

Are functions even part of preloading? If yes, then those files must be wrapped in if (!function_exists(...)).

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jan 10, 2020

This is a workaround, not a system solution. And unless there is a system solution and until the libraries are ready, the general preloader should not load 'files'.

@peldax

This comment has been minimized.

Copy link
Contributor

@peldax peldax commented Jan 12, 2020

@nikic: 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.

I believe @nikic is suggesting to create separate file for each alias -> so that autoloader can correctly load needed alias on demand. (For example IAdapter.php file, with class_alias call to create correct alias from IAdapater to Adapter.) Not one big compatibility.php file with definition of all aliases.

This comes with disadvantage that you are polluting your working directory with deprecated alias files. How about to create new deprecated directory on the same level as src, where those files can sit quietly and cause no trouble? This directory could be listed in composers autoload to ensure autoloading of those deprecated class names.

  • the problem above is solved, because there is no compatibility.php in files
  • compatibility is fully guaranteed because alias files are known by composer
  • users don't load old class names at all if not used in code, or load only the specific aliases they need
  • your working directory is not polluted with deprecated files
@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jan 12, 2020

@peldax Can you show me a functional example?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.