Issue 325 php8 attributes #326
Conversation
|
@ixarlie FYI CI has been fixed in |
|
@W0rma branch was updated with |
ixarlie
force-pushed
the
issue-325-php8-attributes
branch
from
1dd390b to
ac5771e
Compare
|
Is anyone handling this PR? |
| * @param string|array $content | ||
| * @param Exclusion|null $exclusion | ||
| */ | ||
| public function __construct(array $values = [], $content = null, ?string $type = null, ?string $xmlElementName = null, $exclusion = null) |
There was a problem hiding this comment.
| public function __construct(array $values = [], $content = null, ?string $type = null, ?string $xmlElementName = null, $exclusion = null) | |
| public function __construct(array $values = [], $content = null, ?string $type = null, ?string $xmlElementName = null, ?Exclusion $exclusion = null) |
| /** | ||
| * @Annotation | ||
| * @Target("ANNOTATION") | ||
| */ | ||
| #[\Attribute(0)] |
There was a problem hiding this comment.
What's the purpose of passing "0"?
(the same question applies to all usages of #[\Attribute(0)] in this PR
There was a problem hiding this comment.
I tried to resolve an issue that may not be an issue. I intended not to allow the attribute to be used on any of the existing targets. It seems the 0 produces this behaviour.
Attribute::TARGET_CLASS
Attribute::TARGET_FUNCTION
Attribute::TARGET_METHOD
Attribute::TARGET_PROPERTY
Attribute::TARGET_CLASS_CONSTANT
Attribute::TARGET_PARAMETER
Attribute::TARGET_ALL
For example, Embedded can be used only as a nested attribute.
I can remove the 0 for Embedded, Exclusion and Route as I am not sure if it could be a problem for certain configurations.
| /** | ||
| * @Required | ||
| * @var mixed | ||
| */ | ||
| #[Required] |
There was a problem hiding this comment.
The @Required annotation does not reference a symfony class but Doctrine\Common\Annotations\Annotation\Required.
I suggest to remove #[Required] everywhere in this PR.
There was a problem hiding this comment.
Yes, I was wrong to use the Symfony Required class which is about the service auto-wiring. So, it has nothing to do with the Doctrine annotation purpose.
I will remove #[Required]
| */ | ||
| private $reader; | ||
|
|
||
| public function __construct(Reader $reader) |
There was a problem hiding this comment.
From the class name I would expect that this class is only about reading attributes.
An approach similar to schmittjoh/serializer#1469 would be nice.
This way the annotation reader would become optional.
There was a problem hiding this comment.
To be honest, I didn't want to do something very different from how JMS was doing it back then. That PR was created after I created mine.
But I totally agree with that decoupling approach.
I believe it answers my concern in this comment #326 (comment)
|
@ixarlie Are you still working on this PR or should someone else take it over? |
@W0rma Hi, sorry for the late response. Yes, I can still work on it. I worked on this some time ago, my concern is whether the approach I've copied from JMS is still valid for this or whether there are better ways to accomplish it. In any case, let me go over your comments. Thanks |
ixarlie
force-pushed
the
issue-325-php8-attributes
branch
from
9d3d3d6 to
312caef
Compare
|
I will look at it mid February, sorry busy times 😞😢 |
|
Hello, |
|
This PR can also move the doctrine/annotation in suggested requirement to enable some project using it to remove the doctrine/annotation dependencies? |
resolves #325
To be precise the php version must be 8.1 or greater to support nested attributes.