Remove non-autoloadable classes from preload #44380

jderusse · 2021-12-01T00:57:08Z

Q	A
Branch?	5.3
Bug fix?	yes
New feature?	no
Deprecations?	mp
Tickets	Fix #44364
License	MIT
Doc PR

When a class is required in a preloaded file, all the type hinted properties have to be auto-loadable. (sees discussion in linked issue)

This could be an issue for classes like BodyRenderer that have a property typehinted with an optional dependency.

symfony/src/Symfony/Bridge/Twig/Mime/BodyRenderer.php

Lines 27 to 39 in 02b40c0

    
           private HtmlConverter $converter; 
        
           public function __construct(Environment $twig, array $context = []) 
        
           { 
        
               $this->twig = $twig; 
        
               $this->context = $context; 
        
               if (class_exists(HtmlConverter::class)) { 
        
                   $this->converter = new HtmlConverter([ 
        
                       'hard_break' => true, 
        
                       'strip_tags' => true, 
        
                       'remove_nodes' => 'head style', 
        
                   ]); 
        
               }

Sometime, the relation is tricky. For instance RedisAdapter uses RedisTrait that have a property $redis typehinted RedisClusterProxy that have a property typehinted Predis\ClientInterface => but Predis is an optional dependency

When dumping the Preload classMap, this PR now check if all the properties are auto-loadable.

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

nicolas-grekas

I really like the idea!

I think the current state of the implementation won't work: shouldn't a class that depends on a non-preloadable class also be flagged as non-preloadable?

I also have one fear: in practice, how many classes will this exclude from preloading (especially considering the previous point?) The more typed properties we use, the less classes will be preloaded.

Or maybe we shouldn't be concerned because PHP 8.1 doesn't have the issue, and preloading is also mostly dead since 8.1. We could even consider deprecating our dedicated logic for it.

jderusse · 2021-12-01T08:44:17Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

+        }
+        $this->preloadableCache[$class] = true; // prevent recursion
+
+        if (!class_exists($class) && !interface_exists($class, false) && !trait_exists($class, false)) {


beware, switched from class_exists($class, false) to class_exists($class)

stof · 2021-12-01T10:22:52Z

preloading is also mostly dead since 8.1

@nicolas-grekas what do you mean with this sentence ?

nicolas-grekas · 2021-12-01T10:43:08Z

I mean that preloading might not be worth the trouble it is to maintain and setup anymore...

jderusse · 2021-12-01T12:22:20Z

I think the current state of the implementation won't work: shouldn't a class that depends on a non-preloadable class also be flagged as non-preloadable?

Indeed, I missed 3 points:

The Preloader browse the classes, properties and methods parameters to discover more classes.

This is now fixed by doing the ~same thing in PHPDumper.

The preloader uses get_declared_classes to browse more classes

This one is tricky because, in PhpDumper, I can't easily do the same for each discovered class. That would require a tokenizer, or calling get_declared_classes in an isolated process => big overhead.

I fixed it by pre-filling the $preloaded parameter of the Preloader to prevent it to browse the classes I known to be not pre-loadable.

The generated .preload file is full of require_once included in the Dumped Container

I have no choice but removing the container from the .preload.php file. (in my case, the code preloaded 2300 files before, and 1900 after that change)

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

fabpot · 2021-12-07T09:52:14Z

This one is quite important as currently, Symfony 6.0 is broken when using pre-loading.

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

fabpot · 2021-12-07T16:20:19Z

See #44494 for an alternative

fabpot · 2021-12-08T08:40:57Z

Closing in favor of #44494

jderusse · 2021-12-08T09:18:15Z

reopening: Removing typehinted property in symfony/symfony: 6.0 is a workaround for our internal classes, but the bug still exists for other dependencies.

The current implementation of the preloads script discovers all classes used by the end-user. Every third-party package/bundle with type hinted property will hit the same bug.

Worst case, this PR should be merged in 6.1 and type-hint re-added.

This PR was merged into the 6.0 branch. Discussion ---------- Remove FQCN type hints on properties | Q | A | ------------- | --- | Branch? | 6.0 | Bug fix? | yes | New feature? | no  | Deprecations? | no  | Tickets | Closes #44364 | License | MIT | Doc PR | n/a This is an alternative for #44380. Basically, we are removing all property type hints on 6.0 and reverts that for 6.1. Commits ------- 52aaa56 Remove FQCN type hints on properties

fabpot · 2021-12-11T15:22:38Z

This one should be rebased on 6.1 then.

nicolas-grekas · 2021-12-13T10:28:54Z

The preloader uses get_declared_classes to browse more classes

This one is tricky because, in PhpDumper, I can't easily do the same for each discovered class. That would require a tokenizer, or calling get_declared_classes in an isolated process => big overhead.

Could we fix this by registering an autoloader that only collects autoloaded symbols before the real autoloader?

The generated .preload file is full of require_once included in the Dumped Container

I have no choice but removing the container from the .preload.php file. (in my case, the code preloaded 2300 files before, and 1900 after that change)

You mean in the initializer that is called on the first call to set() since #44010, right?
We might be able to fix this by not calling set() but listing the files/classes in the preload script instead, isn't it?

nicolas-grekas · 2021-12-13T11:36:47Z

What about another alternative: deprecate generating the preload script at all?

Since 8.1, preloading provides almost no benefits to me.

Preloading is (was?) also sensitive to "over-preloading" (aka preloading too many classes, making it under-perform due to hash table overhead), so that any benefits provided by preloading might easily be canceled by having it not properly configured. And since configuring it is quite difficult, the gain might not be reachable in practice.

Also, I guess 8.1 is able to cache the same class/symbol for different paths, while preloading cannot.

I'd be interested in knowing @nikic's thoughts on the topic!

Or we could make the preloading script extremely simpler, eg preloading only composer+some very basic stuffs.

nicolas-grekas · 2021-12-13T13:09:24Z

BTW, would it possible to turn that fatal error into a warning on PHP 8.0, as a bugfix?

nikic · 2021-12-16T20:33:54Z

BTW, would it possible to turn that fatal error into a warning on PHP 8.0, as a bugfix?

Nope, the fatal error is required for correctness in 8.0.

As to preloading in general, the usefulness is greatly diminished with 8.1, but I don't really have data on whether there are any cases where it still makes sense.

wouterj · 2022-02-09T10:28:54Z

As we've added the type to properties again in the 6.1-dev branch, what is needed to finish this PR? Is there anything I (or the community) can do to help move this forward?

nicolas-grekas · 2022-02-25T10:28:51Z

Replaced by #45377

Thank you very much for providing this patch @jderusse, it's been critical to make an informed decision on the topic! 🙏

This PR was squashed before being merged into the 6.1 branch. Discussion ---------- Bump minimum version of PHP to 8.1 | Q | A | ------------- | --- | Branch? | 6.1 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | Fix #44364 | License | MIT | Doc PR | - While reviewing #44380, I wondered: why should we maintain such a complex piece of logic while only PHP 8.0 is affected? So here we are: what about bumping Symfony 6.1 to PHP 8.1 minimum? That would free ourselves from maintaining this workaround. The good news is that Ubuntu 22.04LTS will ship PHP 8.1, so that it's going to be easy to have it widely installed. Also, looking at https://packagist.org/packages/symfony/framework-bundle/php-stats#6.0, early adopters of Symfony 6 are already using PHP 8.1 en masse, and it's growing fast. Let's do it? Commits ------- b0217c6 Bump minimum version of PHP to 8.1

carsonbot added Status: Needs Review Bug labels Dec 1, 2021

carsonbot added this to the 5.3 milestone Dec 1, 2021

jderusse force-pushed the preload branch 3 times, most recently from ea33197 to 83dbe08 Compare December 1, 2021 07:42

jderusse commented Dec 1, 2021

View reviewed changes

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Dec 1, 2021

View reviewed changes

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Dec 1, 2021

View reviewed changes

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Dec 1, 2021

View reviewed changes

jderusse force-pushed the preload branch from 83dbe08 to fa455bc Compare December 1, 2021 08:42

jderusse commented Dec 1, 2021

View reviewed changes

jderusse force-pushed the preload branch 3 times, most recently from 8c47b47 to ca03364 Compare December 1, 2021 09:35

jderusse force-pushed the preload branch from ca03364 to f90ffb7 Compare December 1, 2021 12:22

derrabus reviewed Dec 1, 2021

View reviewed changes

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php Outdated Show resolved Hide resolved

jderusse force-pushed the preload branch 3 times, most recently from e77efd2 to 4c87358 Compare December 2, 2021 11:58

nicolas-grekas mentioned this pull request Dec 3, 2021

Preloading error because of CsrfTokenManagerInterface in Form component #44445

Closed

stof reviewed Dec 7, 2021

View reviewed changes

fabpot mentioned this pull request Dec 7, 2021

Remove FQCN type hints on properties #44494

Merged

fabpot closed this Dec 8, 2021

jderusse reopened this Dec 8, 2021

fabpot modified the milestones: 5.3, 6.1 Dec 11, 2021

jderusse changed the base branch from 5.3 to 6.1 December 11, 2021 18:35

jderusse force-pushed the preload branch from 4c87358 to 263bace Compare December 11, 2021 18:45

Remove non-autoloadable classes from preload

fa0f7f6

jderusse force-pushed the preload branch from 263bace to fa0f7f6 Compare December 11, 2021 18:46

scheb mentioned this pull request Jan 4, 2022

Missing TrustedDeviceManagerInterface scheb/2fa#121

Closed

nicolas-grekas added the DependencyInjection label Feb 9, 2022

This was referenced Feb 9, 2022

Bump minimum version of PHP to 8.1 #45377

Merged

[DependencyInjection] Skip preloading on PHP 8.0 #45384

Closed

nicolas-grekas closed this Feb 25, 2022

alexislefebvre mentioned this pull request Jan 17, 2023

[Performance] Remove preloading? symfony/symfony-docs#17643

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove non-autoloadable classes from preload #44380

Remove non-autoloadable classes from preload #44380

jderusse commented Dec 1, 2021

nicolas-grekas left a comment •

edited

Loading

jderusse Dec 1, 2021

stof commented Dec 1, 2021

nicolas-grekas commented Dec 1, 2021

jderusse commented Dec 1, 2021

fabpot commented Dec 7, 2021

fabpot commented Dec 7, 2021

fabpot commented Dec 8, 2021

jderusse commented Dec 8, 2021

fabpot commented Dec 11, 2021

nicolas-grekas commented Dec 13, 2021

nicolas-grekas commented Dec 13, 2021 •

edited

Loading

nicolas-grekas commented Dec 13, 2021

nikic commented Dec 16, 2021

wouterj commented Feb 9, 2022

nicolas-grekas commented Feb 25, 2022

	private HtmlConverter $converter;

	public function __construct(Environment $twig, array $context = [])
	{
	$this->twig = $twig;
	$this->context = $context;
	if (class_exists(HtmlConverter::class)) {
	$this->converter = new HtmlConverter([
	'hard_break' => true,
	'strip_tags' => true,
	'remove_nodes' => 'head style',
	]);
	}

Remove non-autoloadable classes from preload #44380

Remove non-autoloadable classes from preload #44380

Conversation

jderusse commented Dec 1, 2021

nicolas-grekas left a comment • edited Loading

Choose a reason for hiding this comment

jderusse Dec 1, 2021

Choose a reason for hiding this comment

stof commented Dec 1, 2021

nicolas-grekas commented Dec 1, 2021

jderusse commented Dec 1, 2021

fabpot commented Dec 7, 2021

fabpot commented Dec 7, 2021

fabpot commented Dec 8, 2021

jderusse commented Dec 8, 2021

fabpot commented Dec 11, 2021

nicolas-grekas commented Dec 13, 2021

nicolas-grekas commented Dec 13, 2021 • edited Loading

nicolas-grekas commented Dec 13, 2021

nikic commented Dec 16, 2021

wouterj commented Feb 9, 2022

nicolas-grekas commented Feb 25, 2022

nicolas-grekas left a comment •

edited

Loading

nicolas-grekas commented Dec 13, 2021 •

edited

Loading