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

Incompatible with PHP 8.1 #16

Closed
bobvandevijver opened this issue Feb 25, 2022 · 3 comments · Fixed by #22
Closed

Incompatible with PHP 8.1 #16

bobvandevijver opened this issue Feb 25, 2022 · 3 comments · Fixed by #22

Comments

@bobvandevijver
Copy link

In its current state, this library does not work fully with PHP 8.1. 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);).

Also posted at Ocramius/ProxyManager#737.

@nicolas-grekas
Copy link
Collaborator

Closing because Ocramius/ProxyManager#737 is enough

@bobvandevijver
Copy link
Author

@nicolas-grekas Actually, I do not agree with the other issue being enough. This library promotes itself as being compatible with a broader PHP range, while it currently isn't compatible with PHP 8.1. That is a clear and important issue for this project.

The library it is based on explicitly doesn't support PHP 8.1 at this time, so either work must be put in this library to add support (and hopefully to be backported to the upstream as well) or it should be marked as incompatible with PHP 8.1 because it is.

With Symfony requiring PHP 8.1 starting from 6.1, the proxy-manager-bridge will be broken as well...

@nicolas-grekas
Copy link
Collaborator

nicolas-grekas commented Feb 26, 2022

The fix for this issue should land in ocramius/proxy-manager. Once it will land there, I'll sync this fork, as I always do when a new release of ocramius/proxy-manager happens. As such, there is no need to keep track of this specific issue here.

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

Successfully merging a pull request may close this issue.

2 participants