Use NamedArgumentConstructorAnnotation for annotations #2243
Conversation
| { | ||
| /** @var string */ | ||
| public $name; | ||
|
|
||
| /** @var string */ | ||
| public $type = 'string'; | ||
| /** @var string|null */ |
| /** @var string|string[] */ | ||
| public $value; | ||
|
|
||
| /** @var string|null */ |
There was a problem hiding this comment.
I tried to fix all the wrong docblocks by the way
|
|
||
| public function __construct(string $value) | ||
| { | ||
| $this->value = $value; |
There was a problem hiding this comment.
Idea for a later PR (or commit): add validation to ctors
| { | ||
| /** @var string|null */ | ||
| public $db; | ||
|
|
||
| /** @var string|null */ | ||
| /** @var array|string|null */ |
There was a problem hiding this comment.
array is used for capped collection's config
| public $notSaved = true; | ||
| public function __construct( | ||
| string $name = 'chunkSize', | ||
| string $type = 'int', |
There was a problem hiding this comment.
Does it even make sense to allow overriding anything but $name here? Same goes for other File-related annotations
There was a problem hiding this comment.
No: only the name can be configured, the rest is specified in the GridFS Specification.
| @@ -35,7 +32,7 @@ final class ReferenceMany extends AbstractField | |||
| /** @var string|null */ | |||
| public $defaultDiscriminatorValue; | |||
|
|
|||
| /** @var string[]|null */ | |||
| /** @var string|string[]|null */ | |||
There was a problem hiding this comment.
turns out using string is also fine, it's converted to an array later while mapping field
bbb2f29 to
45bfb60
Compare
There was a problem hiding this comment.
As a general theme, I'd advocate to rename the $value arguments in constructors to something more meaningful. For example, in the AlsoLoad annotation, $value doesn't really tell me what this should be, especially with $name being the other field. This is especially true with these names being somewhat relevant to BC when people use named arguments.
| * | ||
| * @Annotation | ||
| */ | ||
| final class Inheritance extends Annotation |
There was a problem hiding this comment.
Given that this class is final and never checked for, it should be safe to remove it. I'm sure there's someone out there using it, not realising that it didn't do anything.
| /** | ||
| * Specifies a parent class that other documents may extend to inherit mapping | ||
| * information | ||
| * | ||
| * @Annotation | ||
| */ | ||
| final class MappedSuperclass extends AbstractDocument | ||
| final class MappedSuperclass extends AbstractDocument implements NamedArgumentConstructorAnnotation | ||
| { | ||
| /** @var string */ |
There was a problem hiding this comment.
| /** @var string */ | |
| /** @var string|null */ |
|
Getting back to the PR got me so long somebody opened doctrine/annotations#396 which actually is great :) |
|
Nah, let's close this one 👍 |
Brings doctrine/annotations#369 to the ODM, here's what can be done later: doctrine/orm#8266