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

[DependencyInjection] unable to make lazy service from readonly class #53843

Closed
kor3k opened this issue Feb 7, 2024 · 7 comments
Closed

[DependencyInjection] unable to make lazy service from readonly class #53843

kor3k opened this issue Feb 7, 2024 · 7 comments

Comments

@kor3k
Copy link
Contributor

kor3k commented Feb 7, 2024

Symfony version(s) affected

6.4.3

Description

marking any readonly class as a lazy service results in an error.

the bug seems not on symfony side, but on
https://github.com/FriendsOfPHP/proxy-manager-lts
side, as it does not count for readonly classes while creating proxies.
(#53843 (comment))

How to reproduce

  1. create readonly class
<?php

namespace App;

readonly class TestService
{
    public function foo(): string
    {
        return 'bar';
    }
}
  1. mark it as lazy
services:
  App\TestService:
    lazy: true
  1. inject it anywhere, e.g. in controller
<?php

namespace App\Controller;

use App\TestService;
use Symfony\Component\Routing\Attribute\Route;

class TestController
{
    #[Route(path: '/test')
    public function __invoke(TestService $service): Response
    {
        return new Response($service->foo());
    }
}
  1. result is error
Compile Error: Non-readonly class ContainerBh60GB2\TestServiceGhost93bb697 cannot extend readonly class App\TestService

Possible Solution

No response

Additional Context

No response

@nicolas-grekas
Copy link
Member

We don't use that lib anymore so this is a Symfony thing. The DI component together with the VarExporter one are responsible for generating the proxies. Would you like to have a look?
This line is supposed to generate a readonly proxy class:

return (\PHP_VERSION_ID >= 80200 && $class?->isReadOnly() ? 'readonly ' : '').'class '.$proxyClass.ProxyHelper::generateLazyProxy($class, $interfaces);

Could you please see if you can debug and possibly fix the issue?

@kor3k
Copy link
Contributor Author

kor3k commented Feb 7, 2024

@nicolas-grekas that's weird, it does not get to this line at all.

i'll update

@nicolas-grekas
Copy link
Member

Oh, maybe we're missing the readonly logic on L108 then?

@kor3k
Copy link
Contributor Author

kor3k commented Feb 7, 2024

@nicolas-grekas

this is truthy

if ($asGhostObject) {

so the readonly is missing on

return 'class '.$proxyClass.ProxyHelper::generateLazyGhost($class);

@kor3k
Copy link
Contributor Author

kor3k commented Feb 7, 2024

but when i change the L108 to

return (\PHP_VERSION_ID >= 80200 && $class?->isReadOnly() ? 'readonly ' : '').'class '.$proxyClass.ProxyHelper::generateLazyGhost($class);

then i get (after c:c)

Warning: require(/app/var/cache/dev/ContainerUlZygQT/TestServiceGhost93bb697.php): Failed to open stream: No such file or directory

@kor3k
Copy link
Contributor Author

kor3k commented Feb 7, 2024

@nicolas-grekas

there seems to be a problem with filename. because it does generate the class, but names it class.php

  GNU nano 6.2 /app/var/cache/dev/ContainerUlZygQT/class.php                                                                                                                                 
<?php

namespace ContainerUlZygQT;
include_once \dirname(__DIR__, 4).'/src/TestService.php';

readonly class TestServiceGhost93bb697 extends \App\TestService implements \Symfony\Component\VarExporter\LazyObjectInterface
{
    use \Symfony\Component\VarExporter\LazyGhostTrait;

    private const LAZY_OBJECT_PROPERTY_SCOPES = [];
}

// Help opcache.preload discover always-needed symbols
class_exists(\Symfony\Component\VarExporter\Internal\Hydrator::class);
class_exists(\Symfony\Component\VarExporter\Internal\LazyObjectRegistry::class);
class_exists(\Symfony\Component\VarExporter\Internal\LazyObjectState::class);

if (!\class_exists('class', false)) {
    \class_alias(__NAMESPACE__.'\\class', 'class', false);
}

@nicolas-grekas
Copy link
Member

Another change is needed. Up to turn this into a PR and add a test case while doing so?

diff --git a/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php b/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
index 81b500c5ad7..23e65483b03 100644
--- a/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
+++ b/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
@@ -624,7 +624,9 @@ EOF;
                 $proxyCode = substr(self::stripComments($proxyCode), 5);
             }
 
-            $proxyClass = explode(' ', $this->inlineRequires ? substr($proxyCode, \strlen($code)) : $proxyCode, 3)[1];
+            $proxyClass = $this->inlineRequires ? substr($proxyCode, \strlen($code)) : $proxyCode;
+            $i = strpos($proxyClass, 'class');
+            $proxyClass = substr($proxyClass, 6 + $i, strpos($proxyClass, ' ', 7 + $i) - $i - 6);
 
             if ($this->asFiles || $this->namespace) {
                 $proxyCode .= "\nif (!\\class_exists('$proxyClass', false)) {\n    \\class_alias(__NAMESPACE__.'\\\\$proxyClass', '$proxyClass', false);\n}\n";
diff --git a/src/Symfony/Component/DependencyInjection/LazyProxy/PhpDumper/LazyServiceDumper.php b/src/Symfony/Component/DependencyInjection/LazyProxy/PhpDumper/LazyServiceDumper.php
index b1df5f0f0cd..0d4426c4403 100644
--- a/src/Symfony/Component/DependencyInjection/LazyProxy/PhpDumper/LazyServiceDumper.php
+++ b/src/Symfony/Component/DependencyInjection/LazyProxy/PhpDumper/LazyServiceDumper.php
@@ -105,7 +105,7 @@ final class LazyServiceDumper implements DumperInterface
 
         if ($asGhostObject) {
             try {
-                return 'class '.$proxyClass.ProxyHelper::generateLazyGhost($class);
+                return (\PHP_VERSION_ID >= 80200 && $class?->isReadOnly() ? 'readonly ' : '').'class '.$proxyClass.ProxyHelper::generateLazyGhost($class);
             } catch (LogicException $e) {
                 throw new InvalidArgumentException(sprintf('Cannot generate lazy ghost for service "%s".', $id ?? $definition->getClass()), 0, $e);
             }

nicolas-grekas added a commit that referenced this issue Feb 9, 2024
… readonly class (kor3k)

This PR was merged into the 6.4 branch.

Discussion
----------

[DependencyInjection] fix unable to make lazy service from readonly class

[DependencyInjection] unable to make lazy service from readonly class #53843

| Q             | A
| ------------- | ---
| Branch?       | 6.4 <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Issues        | Fix #53843 <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead -->
| License       | MIT

<!--
Replace this notice by a description of your feature/bugfix.
This will help reviewers and should be a good start for the documentation.

Additionally (see https://symfony.com/releases):
 - Always add tests and ensure they pass.
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the latest branch.
 - For new features, provide some code snippets to help understand usage.
 - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry
 - Never break backward compatibility (see https://symfony.com/bc).
-->

Commits
-------

5197b47 [DependencyInjection] fix unable to make lazy service from readonly class
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants