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

Promoted readonly properties are not respected #737

Open
bobvandevijver opened this issue Feb 25, 2022 · 7 comments · Fixed by FriendsOfPHP/proxy-manager-lts#22
Open

Promoted readonly properties are not respected #737

bobvandevijver opened this issue Feb 25, 2022 · 7 comments · Fixed by FriendsOfPHP/proxy-manager-lts#22

Comments

@bobvandevijver
Copy link

bobvandevijver commented Feb 25, 2022

When using a promoted readonly only property in a class, this library will generate code that tries to change the readonly property and therefor breaking execution. Consider the following class (all have been stripped to only show the constructor):

class SystemNotificationService extends AbstractCacheableService
{
  public function __construct(
      private readonly SystemNotificationRepository $systemNotificationRepository,
      private readonly SerializerInterface $serializer,
      Features $features,
      string $kernelEnvironment,
      string $commitHash,
      string $httpHost,
      string $cacheDir)
  {
    parent::__construct($features, $kernelEnvironment, $commitHash, $httpHost, $cacheDir);
  }
}
abstract class AbstractCacheableService
{
  public function __construct(
      protected readonly Features $features,
      string $kernelEnvironment,
      string $commitHash,
      string $httpHost,
      private readonly string $cacheDir)
  {
  }
}

will generate the following proxy:

class SystemNotificationService_6ddbdeb extends \App\Service\SystemNotificationService implements \ProxyManager\Proxy\VirtualProxyInterface
{
    public function __construct(private \JMS\Serializer\SerializerInterface $serializer, \App\Features $features)
    {
        static $reflection;

        if (! $this->valueHolder06ae0) {
            $reflection = $reflection ?? new \ReflectionClass('App\\Service\\SystemNotificationService');
            $this->valueHolder06ae0 = $reflection->newInstanceWithoutConstructor();
        unset($this->features);

        \Closure::bind(function (\App\Service\SystemNotificationService $instance) {
            unset($instance->systemNotificationRepository, $instance->serializer);
        }, $this, 'App\\Service\\SystemNotificationService')->__invoke($this);

        \Closure::bind(function (\App\Service\AbstractCacheableService $instance) {
            unset($instance->cache, $instance->cacheHash, $instance->clearedTags, $instance->clearedKeys, $instance->cacheDir);
        }, $this, 'App\\Service\\AbstractCacheableService')->__invoke($this);

        }

        $this->valueHolder06ae0->__construct($systemNotificationRepository, $serializer, $features, $kernelEnvironment, $commitHash, $httpHost, $cacheDir);
    }
}

and thus throw Cannot unset readonly property App\Service\AbstractCacheableService::$features from scope ContainerXH6sJpc\SystemNotificationService_6ddbdeb, caused by the unset($instance->features).

Note that the same will also happen with the systemNotificationRepository and serializer properties (unset($instance->systemNotificationRepository, $instance->serializer);).

@Ocramius
Copy link
Owner

Be aware that this library is not compatible with PHP 8.1.0: there's still lots to be done to include 8.1.0 within the supported version range.

@Ocramius
Copy link
Owner

In fact, 8.1.0 is explicitly excluded:

"php": "~7.4.1 || ~8.0.0",

@bobvandevijver
Copy link
Author

bobvandevijver commented Feb 25, 2022

In fact, I'm using https://github.com/FriendsOfPHP/proxy-manager-lts which allows everything above 7.1.

https://github.com/FriendsOfPHP/proxy-manager-lts/blob/72294147d32a227ffadf1abd2f712da128e36a52/composer.json#L29

They state that issues should be opened here, but I will cross post one there that references this issue.

@Ocramius
Copy link
Owner

Ocramius commented Feb 25, 2022

That project is literally "how to step on @Ocramius' face without any respect", so I really don't care about what they do

@Ocramius
Copy link
Owner

@bobvandevijver to be clear, the issue you reported is very clear and very much in need of research (which I will do, once I have time - mostly focused on roave/better-reflection for PHP 8.1, so I'm "recovering" from that 🤣).

My previous comments is just a rant about the fact that these forks are generally irresponsible towards end users, so it makes me extremely angry (to the point that I skip working on the library due to my mood) every time they come up.

@bobvandevijver
Copy link
Author

@Ocramius Luckily I understood :) I am sincerely sorry to hear you feel this way, unfortunately not something I can directly influence as the fork is being forced by default by the Symfony framework in order to use lazy services, which is how I ended up here. I almost regret mentioning it as it clearly triggered you 😶

Anyways, I do hope you will find some time in the near future to bring full PHP 8.1 support to this library (I only just hit this issue and am using 8.1 already since the 8.1.1 release, so it feels like it mostly works fine already). Keep up the good work! 👍🏻

@nicolas-grekas
Copy link
Contributor

FYI I'm working on this at FriendsOfPHP/proxy-manager-lts#22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants