Decouple from annotations #246
Conversation
This is consistent with other packages such as doctrine/dbal
greg0ire
force-pushed
the
decouple-from-annotations
branch
from
acbc75e to
648bcc2
Compare
lib/Doctrine/Persistence/Mapping/Driver/ColocatedMappingDriver.php
Outdated
Show resolved
Hide resolved
greg0ire
force-pushed
the
decouple-from-annotations
branch
2 times, most recently
from
f0ce492 to
10a12d6
Compare
|
ping @derrabus 🙏 |
lib/Doctrine/Persistence/Mapping/Driver/ColocatedMappingDriver.php
Outdated
Show resolved
Hide resolved
| * @return self | ||
| */ | ||
| public static function pathRequired() | ||
| public static function pathRequired(string $driverClassName) |
There was a problem hiding this comment.
This is a BC break. Shall we add a new named constructor (e.g. pathRequiredForDriver) instead?
| */ | ||
| public function testGetAllClassNames(string $path): void | ||
| { | ||
| $driver = new MyDriver($path); |
There was a problem hiding this comment.
Shall we move the constructor call into a protected method that AnnotationDriverTest can override?
| * @param Reader $reader The AnnotationReader to use, duck-typed. | ||
| * @param string|string[]|null $paths One or multiple paths where mapping classes can be found. | ||
| */ | ||
| public function __construct($reader, $paths = null) |
There was a problem hiding this comment.
Should the constructor trigger a runtime deprecation?
greg0ire
force-pushed
the
decouple-from-annotations
branch
from
10a12d6 to
14b1eab
Compare
greg0ire
force-pushed
the
decouple-from-annotations
branch
from
14b1eab to
6b12f2b
Compare
| The Doctrine Persistence project offers a few base implementations that make it easy to implement your own XML, | ||
| Annotations or YAML drivers. | ||
| The Doctrine Persistence project offers a few base implementations that | ||
| make it easy to implement your own XML, Attributes or YAML drivers. |
|
|
||
| This driver requires the ``doctrine/annotations`` project. You can install it with composer. | ||
| This driver requires the ``doctrine/annotations`` project and is | ||
| deprecated because of that. |
There was a problem hiding this comment.
I'd add a note about attributes superseding annotations
There was a problem hiding this comment.
Also why removing the docs below? The feature is deprecated but documentation is still valuable :)
There was a problem hiding this comment.
I think it feels weird to deprecate a feature yet help people build on it. I thought I would make it harder for people to shoot themselves in the foot.
| /** | ||
| * The ColocatedMappingDriver reads the mapping metadata located near the code. | ||
| */ | ||
| trait ColocatedMappingDriver |
There was a problem hiding this comment.
why trait instead of an abstract class?
There was a problem hiding this comment.
There's a resolved comment about that in this PR.
Closes #217