PHP-DI / PHP-DI

The dependency injection container for humans
https://php-di.org
MIT License
2.68k stars 320 forks source link

PHP 8.0: CompiledContainer Mangling Named Parameters into Namespaced Ones #787

Open BusterNeece opened 3 years ago

BusterNeece commented 3 years ago

Hello! I'm using PHP-DI 6.3.4 and we're in the process of updating our app to make better use of new PHP 8.0 best practices. I discovered when including the following code in my service definition:

Symfony\Contracts\Cache\CacheInterface::class => function (
        App\Environment $environment,
        Psr\Log\LoggerInterface $logger,
    ) {
            $tempDir = $environment->getTempDirectory() . DIRECTORY_SEPARATOR . 'cache';
            $cacheInterface = new Symfony\Component\Cache\Adapter\FilesystemAdapter(
               directory: $tempDir
            );

        $cacheInterface->setLogger($logger);
        return $cacheInterface;
    },

...that the compiled container yielded this result:

 protected function get9()
    {
        return $this->resolveFactory(static function (
        \App\Environment $environment,
        \Psr\Log\LoggerInterface $logger
    ) {
          $tempDir = $environment->getTempDirectory() . DIRECTORY_SEPARATOR . 'cache';
          $cacheInterface = new \Symfony\Component\Cache\Adapter\FilesystemAdapter(
              \directory: $tempDir
          );

        $cacheInterface->setLogger($logger);
        return $cacheInterface;
    }, 'Symfony\\Contracts\\Cache\\CacheInterface');
    }

This yields the error:

Parse error: syntax error, unexpected token ":", expecting ")" in CompiledContainer.php on line 228

Presumably this is because it's detecting the named method argument as something it should be namespacing, which is causing parsing errors down the line.

mnapoli commented 3 years ago

Hi, thanks for the report! It seems to be caused by https://github.com/opis/closure/issues/90 and unfortunately I don't see any pull request to support that yet in opis/closure.

BusterNeece commented 3 years ago

@mnapoli looking into that issue, it looks like they did actually merge a fix for it into their 4.x branch, which is intended to support the new PHP 8.x features like this one.

Is that 4.x branch already in use on the beta branch of PHP-DI? I wouldn't have any issue switching over to that for now.

mnapoli commented 3 years ago

No currently PHP-DI uses v3. Maybe it could be possible to support v3 and v4 if they haven't changed the API that PHP-DI uses?

BusterNeece commented 3 years ago

@mnapoli I'm worried that may be the only way to actually take advantage of PHP 8.0-level features inside the closures that are compiled, since they've closed PRs that would add PHP 8.0 functionality to 3.x, saying those features were reserved exclusively for 4.x.

The problem, though, is that 4.x seems to require the php-ffi extension be installed. That would be a huge BC break and is a pretty significant roadblock to jumping to that version.

mnapoli commented 3 years ago

Oh… yeah very good point, thanks for bringing that up.

I've asked for some clarification in https://github.com/opis/closure/issues/59#issuecomment-874279069 as I see Laravel is also using opis/closure.