doctrine / common

Doctrine Common
https://www.doctrine-project.org/projects/common.html
MIT License
5.79k stars 294 forks source link

Compile Error: Constant expression contains invalid operations #961

Open oleg-andreyev opened 2 years ago

oleg-andreyev commented 2 years ago

Bug Report

Compile Error: Constant expression contains invalid operations

Q A
BC Break no
Version 2.11.2

Summary

Entity with following method

class A {
    public function getActivePriceOverrideForCompany(
        Company $company,
        DateTimeImmutable $activeAt = new DateTimeImmutable('today')
    ): ?PriceOverride { ... }
}

as result getting proxy like this:

public function getActivePriceOverrideForCompany(\App\Entity\Company $company, \DateTimeImmutable $activeAt = DateTimeImmutable::__set_state(array(
   'date' => '2022-05-03 00',
   'timezone_type' => 3,
   'timezone' => 'UTC',
))): ?\App\Entity\PriceOverride { ... }
  1. Proxy is invalid
  2. Even if the code will be correct, proxy will be invalid on the next day.

This will work:

class A {
    public function getActivePriceOverrideForCompany(
        Company $company,
        ?DateTimeImmutable $activeAt = null
    ): ?PriceOverride { ... }
}
greg0ire commented 2 years ago

Transferring to doctrine/common since that is where the code that generate proxies is located.

malarzm commented 2 years ago

So far the only sensible way I've figured to get default's value definition from \ReflectionParameter is via its __toString method: https://3v4l.org/Tlt2m#v8.1.8 I went down the rabbit hole and unfortunately reflection itself is doing some dark magic to format the default value, including exporting AST: https://github.com/php/php-src/blob/3331832b04041d93f92a8a9a60602326e7ae9d2f/ext/reflection/php_reflection.c#L612-L655

Summing up: in my opinion we've reached limits of what we can achieve with current proxy generation but I'd like to hear a second thought @doctrine/doctrinecore

alcaeus commented 2 years ago

Pinging @Ocramius for his opinion on the matter - TBH ORM should really drop doctrine/common proxies and start using proxy-manager for this.

Ocramius commented 2 years ago

Even with proxy-manager, default expressions containing object instantiations aren't really supported yet.

That would require moving away from reflection, and instead use something like roave/better-reflection (more complexity/runtime overhead) or raw nikic/php-parser (loads of work to be done).

I completely missed the RFC (https://wiki.php.net/rfc/new_in_initializers): would've raised my concern there.

Ocramius commented 2 years ago

BTW, that is one of the largest areas of concern why ocramius/proxy-manager does not support PHP 8.1 yet: probably need a good week of focused work, which won't happen this year.

nicolas-grekas commented 2 years ago

New initializers are accessible as source code via reflection. I specifically asked Nikita to allow this for such use cases. See https://3v4l.org/F2bjF

nicolas-grekas commented 2 years ago

See https://github.com/php/php-src/pull/7540

nicolas-grekas commented 2 years ago

Oh, and I submitted support for them at https://github.com/Ocramius/ProxyManager/pull/755

malarzm commented 2 years ago

Hah, so parsing __toString it is, that's what I feared. Thanks @nicolas-grekas for your work in ProxyManager, we should be able to port it here 👍