!!! BUGFIX: Avoid broken proxy docblocks #2568
Conversation
kdambekalns
requested review from
robertlemke,
bwaidelich,
kitsunet and
albe
| "use Neos\\Flow\\Annotations as Flow;\n" . | ||
| "\n" . | ||
| $this->buildClassDocumentation() . | ||
| $classCode = $this->buildClassDocumentation() . |
There was a problem hiding this comment.
That's the actual fix…
There was a problem hiding this comment.
Looks good to me.
Note, if you use property introduction via AOP, those properties must from now on use fully-qualified classnames for annoatations on them!
What happens if you don't use a fully-qualified class name? If you'll get an error during compile time and can make some sense of it to get it right, that would be fine.
You get an error like this one:
That lacks the information about where that property actually comes from (as it's not really in Do you think that's sufficient? |
Could be better, but since it's a Doctrine exception, there's not much we can do, I guess? |
Nope. We could invest a yet unknown amount of time into detecting and/or fixing this during compile time, but… 🤷♂️ |
There was a problem hiding this comment.
Generally approve, though I'm having a bit of a hard time with the "fully qualified namespace annotation required" break. Why is that needed if the original class imports the namespace? Is it really not possible to use a shortened namespace in the annotations?
| @@ -70,8 +70,8 @@ public function isEntityOrValueObject() | |||
|
|
|||
| /** | |||
| * @var string | |||
| * @ORM\Id | |||
| * @ORM\Column(length=40) | |||
| * @Doctrine\ORM\Mapping\Id | |||
There was a problem hiding this comment.
Curious, but doesn't the "manual" import in lines 14+15 suffice to have the short named annotation?
There was a problem hiding this comment.
No, because that docblock (of the aspect class) is not copied into the class the property is introduced into…
There was a problem hiding this comment.
Ah, right. Could we possibly monkey-patch https://github.com/neos/flow-development-collection/blob/master/Neos.Flow/Classes/Aop/PropertyIntroduction.php#L83 for a b/c fix to this? I guess it's still hard to resolve the string class name without parsing the use statements of the Aspect class... maybe at least resolve @ORM\ and @Flow\ there to somewhat lower breakyness?
Edit: Did it even ever work for anything besides ORM and Flow? Looking at https://github.com/neos/flow-development-collection/pull/2568/files#diff-cd6bee66fb3ab0580a3e15bfdfa76d82c2c5983e69ec8a5d3211790c7d6c950bL47-L49 I think no?
There was a problem hiding this comment.
It did, because we did not copy the docblock, but re-rendered any annotations, making them fully qualified. As can be seen in the example with the Entity annotation in #2564.
There was a problem hiding this comment.
About the breakiness… TBH I have no clue how much property introduction is actually used and if those few places cannot be fixed faster, than it would take us to come up with a fix that doesn't come with additional (potential) bugs…
There was a problem hiding this comment.
https://github.com/search?l=PHP&p=8&q=%27%40Flow%5CIntroduce%27&ref=advsearch&type=Code is almost exclusively our (test) code.
There was a problem hiding this comment.
Okay - still, wdyt about making Flow and ORM work by hardcoding the FQCN resolution for those? Whenever you want to inject a property, it's most likely a DB field or some Flow injected property.
There was a problem hiding this comment.
That would reduce the pain. But for the record, in random projects I randomly don't use the general Flow\Annotations import, but rather import @Inject() (for example) directly, because Phpstorm let's me do that very easily. These cases would still break.
There was a problem hiding this comment.
That would reduce the pain. But for the record, in random projects I randomly don't use the general
Flow\Annotationsimport, but rather import@Inject()(for example) directly, because Phpstorm let's me do that very easily. These cases would still break.
uh, would not break, right? Because I imported it and I'm not using the FQCN.
There was a problem hiding this comment.
Would break, because your import in the aspect class' docblock is never copied to the class you introduce a property to. This is only about annotations on introduced properties…
We can of course expand @ORM\ and @Flow, feel free to improve this (I won't have time today)…
Neos.Flow/Documentation/TheDefinitiveGuide/PartIII/AspectOrientedProgramming.rst
Show resolved
Hide resolved
With PR #2533 docblocks are copied from the original class to the proxy class. This breaks when using annotations without the "standard" imports
FlowandORM. One example is the ImportedAsset domain model.This fixes that by some changes to the proxy building.
Note, if you use property introduction via AOP, those properties must from now on use fully-qualified classnames for annoatations on them!
Fixes #2564