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

Compile Error: Constant expression contains invalid operations #961

Open
oleg-andreyev opened this issue May 3, 2022 · 9 comments · Fixed by #966
Open

Compile Error: Constant expression contains invalid operations #961

oleg-andreyev opened this issue May 3, 2022 · 9 comments · Fixed by #966

Comments

@oleg-andreyev
Copy link

oleg-andreyev commented May 3, 2022

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 greg0ire transferred this issue from doctrine/orm May 3, 2022
@greg0ire
Copy link
Member

greg0ire commented May 3, 2022

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

@malarzm
Copy link
Member

malarzm commented Aug 2, 2022

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
Copy link
Member

alcaeus commented Aug 3, 2022

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

@Ocramius
Copy link
Member

Ocramius commented Aug 3, 2022

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
Copy link
Member

Ocramius commented Aug 3, 2022

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
Copy link
Member

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
Copy link
Member

See php/php-src#7540

@nicolas-grekas
Copy link
Member

Oh, and I submitted support for them at Ocramius/ProxyManager#755

@malarzm
Copy link
Member

malarzm commented Aug 3, 2022

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 👍

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.

6 participants