8000 [DependencyInjection] Preload on php 8 · Issue #38698 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DependencyInjection] Preload on php 8 #38698

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.

Sign up for GitHub

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

Jump to bottom
Closed
dotdevio opened this issue Oct 23, 2020 · 10 comments
Closed

[DependencyInjection] Preload on php 8 #38698

dotdevio opened this issue Oct 23, 2020 · 10 comments
Labels
Bug DependencyInjection Status: Needs Review
Milestone
4.4

Comments

@dotdevio
Copy link
dotdevio commented Oct 23, 2020 •
edited
Loading

Symfony version(s) affected: 4.4.* and i think 5.* to.

Description

PHP Fatal error:  Uncaught Error: Call to undefined method ReflectionUnionType::is          
 Builtin() in /php-app/vendor/symfony/dependency-injection/Dumper/Preloader          
 .php:97                                                                                                                
 Stack trace:                                                                                                           
 #0 /php-app/vendor/symfony/dependency-injection/Dumper/Preloader.php(85):           
 Symfony\Component\DependencyInjection\Dumper\Preloader::preloadType()                                                  
 #1 /php-app/vendor/symfony/dependency-injection/Dumper/Preloader.php(81):           
 Symfony\Component\DependencyInjection\Dumper\Preloader::doPreload()                                                    
 #2 /php-app/vendor/symfony/dependency-injection/Dumper/Preloader.php(103):          
  Symfony\Component\DependencyInjection\Dumper\Preloader::doPreload()                                                   
 #3 /php-app/vendor/symfony/dependency-injection/Dumper/Preloader.php(85):           
 Symfony\Component\DependencyInjection\Dumper\Preloader::preloadType()                                                  
 #4 /php-app/vendor/symfony/dependency-injection/Dumper/Preloader.php(41):           
 Symfony\Component\DependencyInjection\Dumper\Preloader::doPreload()                                                    
 #5 /php-app/apps/web/var/cache/prod/srcApps_Web_KernelProdContainer.preloa          
 d.php(714): Symfony\Component\DependencyInjection\Dumper\Preloader::preload()                                          
 #6 /php-app/apps/web/src/.preload.php(4): require('...')                            
 #7 {main}                                                                                                              
   thrown in /php-app/vendor/symfony/dependency-injection/Dumper/Preloader.          
 php on line 97

How to reproduce
PHP 8.0.0rc1 + symfony 4.4.15

With symfony cli:
symfony new --version=lts --full sf4php8 && cd sf4php8 && echo '8.0.0' > .php-version && symfony serve

Possible Solution

Additional context
In PHP 8, ReflectionType does not have an isBuiltin() method.

There is issue in on bugs.php.net related to this.
https://bugs.php.net/bug.php?id=80247
@dotdevio dotdevio added the Bug label Oct 23, 2020
@derrabus
Copy link
Member

Looks like you're using union types already, don't you? 😃

@dotdevio
Copy link
Author
dotdevio commented Oct 23, 2020 •
edited
Loading

Actually i dont use them explicity in the code, only in the phpdocs (and this is allowed in any php version). I'am just trying to run same project which is sf 4.4 which works fine on php7.4 but fails on php8. Check bug on php.net because on php8 this behavior is different and there is no method isBuiltin() on ReflectionType, only on ReflectionNamedType class which is subtype of ReflectionType.

symfony/src/Symfony/Component/DependencyInjection/Dumper/Preloader.php

Line 97 in d6cc026

if (!$t || $t->isBuiltin()) {

U can try it with empty symfony 4.4 project, it fails without any custom code:

symfony new --version=lts --full sf4php8 && cd sf4php8 && echo '8.0.0' > .php-version && symfony serve

[ERROR] PHP failed to start:                                                                                           
 [Fri Oct 23 18:57:37 2020] PHP Fatal error:  Uncaught Error: Call to undefined method ReflectionUnionType::is          
 Builtin() in php-app/vendor/symfony/dependency-injection/Dumper/Preloader          
 .php:97                                                                                                                
 Stack trace:                                                                                                           
 #0 php-app/vendor/symfony/dependency-injection/Dumper/Preloader.php(85):           
 Symfony\Component\DependencyInjection\Dumper\Preloader::preloadType()                                                  
 #1 php-app/vendor/symfony/dependency-injection/Dumper/Preloader.php(81):           
 Symfony\Component\DependencyInjection\Dumper\Preloader::doPreload()                                                    
 #2 php-app/vendor/symfony/dependency-injection/Dumper/Preloader.php(103):          
  Symfony\Component\DependencyInjection\Dumper\Preloader::doPreload()                                                   
 #3 php-app/vendor/symfony/dependency-injection/Dumper/Preloader.php(85):           
 Symfony\Component\DependencyInjection\Dumper\Preloader::preloadType()                                                  
 #4 php-app/vendor/symfony/dependency-injection/Dumper/Preloader.php(41):           
 Symfony\Component\DependencyInjection\Dumper\Preloader::doPreload()                                                    
 #5 php-app/apps/web/var/cache/prod/srcApps_Web_KernelProdContainer.preloa          
 d.php(714): Symfony\Component\DependencyInjection\Dumper\Preloader::preload()                                          
 #6 php-app/apps/web/src/.preload.php(4): require('...')                            
 #7 {main}                                                                                                              
   thrown in php-app/vendor/symfony/dependency-injection/Dumper/Preloader.          
 php on line 97

This is beacuse of:
https://bugs.php.net/bug.php?id=80247

It should be handled different for php7/php8.

@derrabus
Copy link
Member

All right, but then I wonder where this union type is coming from. Either way, we need to fix this. I'll look into it.

@derrabus derrabus mentioned this issue Oct 23, 2020
Merged
@derrabus
Copy link
Member

Can you test #38699 for me please?

@derrabus derrabus added this to the 4.4 milestone Oct 23, 2020
@dotdevio
Copy link
Author

Looks like error is gone, but i am confused isn't a fix should look like:

    private static function preloadType(?\ReflectionType $t, array &$preloaded): void
    {
        if (!$t) {
            return;
        }

        if (\PHP_VERSION_ID < 80000 && $t->isBuiltin()) {
            return;
        }

        foreach ($t instanceof \ReflectionUnionType ? $t->getTypes() : [$t] as $t) {
            if (!$t->isBuiltin()) {
                self::doPreload($t instanceof \ReflectionNamedType ? $t->getName() : $t, $preloaded);
            }
        }
    }

or i am wrong?

@derrabus
Copy link
Member

There's still the if (!$t->isBuiltin()) { inside the foreach loop. That should be sufficient.

@derrabus
Copy link
Member

I've extended the fixtures a bit to verify the isBuiltin() check still works.

@dotdevio
Copy link
Author

Im am only cofusing about that because if on php8 $t will be exactly \ReflectionType it will not have method isBuiltin(), but this method always exist in php 7.x in any subclass of \ReflectionType. So if in php8 $t will be exactly \ReflectionType (not \ReflectionUnionType) with ur fix script will go inside loop with [$t = \ReflectionType] and isBuiltin() of \ReflectionType will be called (because $t != \ReflectionUnionType) which not exist in php8 in \ReflectionType. If i am wrong and on php8 $t cannot be exactly \ReflectionType should be okay.

@derrabus
Copy link
Member

if on php8 $t will be exactly \ReflectionType

This won't happen. $t is either a ReflectionNamedType or a ReflectionUnionType. In the latter case, we unpack the union which gives us an array of ReflectionNamedType instances. So either way, on php 8.0, we can be sure we have a ReflectionNamedType in the body of the loop.

@derrabus
Copy link
Member

Also, let's move the code review to the actual PR. 😃

@fabpot fabpot closed this as completed Oct 24, 2020
fabpot added a commit that referenced this issue Oct 24, 2020
@fabpot
bug #38699 [DependencyInjection] Preload classes with union types cor…
bfa396d 
…rectly (derrabus)

This PR was merged into the 4.4 branch.

Discussion
----------

[DependencyInjection] Preload classes with union types correctly

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #38698
| License       | MIT
| Doc PR        | N/A

Commits
-------

a7b0f16 [DependencyInjection] Preload classes with union types correctly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug DependencyInjection Status: Needs Review
Projects
None yet
Milestone
4.4
Development

No branches or pull requests
4 participants
@fabpot @derrabus @carsonbot @dotdevio
0