NEW Add AnnotationTransformer to allow configuration through PHP docblock annotations#35
Conversation
ScopeyNZ
force-pushed
the
pulls/1.2/annotation-collector
branch
from
fdefdd3 to
b95de83
Compare
ScopeyNZ
force-pushed
the
pulls/1.2/annotation-collector
branch
from
b95de83 to
2872165
Compare
robbieaverill
left a comment
robbieaverill
left a comment
There was a problem hiding this comment.
This RFC has been sitting dormant for a while now. I think if you mark each of the new APIs with @internal or something similar then we can merge it in and test it out before we publicise it as a new feature - since it has the potential to have quite an impact on how dependency injection is done in SilverStripe
| /** | ||
| * AnnotationTransformer constructor. | ||
| * @param callable $classResolver | ||
| * @param array $annotationDefinitions |
There was a problem hiding this comment.
You could annotate AnnotationDefinitionInterface[] here to make it clearer what is expected. Also worth adding a @throws for the exception if one of the entries isn't one of these interfaces
| * @return MutableConfigCollectionInterface | ||
| * @throws \ReflectionException | ||
| */ | ||
| public function transform(MutableConfigCollectionInterface $collection) |
There was a problem hiding this comment.
| public function transform(MutableConfigCollectionInterface $collection) | |
| public function transform(MutableConfigCollectionInterface $collection): MutableConfigCollectionInterface |
| use ReflectionClass; | ||
| use ReflectionMethod; | ||
|
|
||
| interface AnnotationDefinitionInterface |
There was a problem hiding this comment.
Might be worth a doc block to explain the purpose of classes implementing this interface
|
This pull request hasn't had any activity for a while. Are you going to be doing further work on it, or would you prefer to close it now? |
|
I'm happy to work on it further if I get the indication it'll be merged 🙂 . This is part of a larger change request involving a couple of framework PRs: silverstripe/silverstripe-framework#9006 |
|
I'd say pretty unlikely to be prioritized by Silverstripe CMS Squad at this stage because it doesn't seem to have an obvious benefit compared to something like fixing a bug. I could be totally wrong though. Would probably need to be a core committer thing. I've added the feedback-required/core-team label. |
|
I'd argue that auto constructor DI is a pretty obvious benefit, but I understand that it's a bit abstract how this PR related to that feature. Anyway, I'm hoping that work/discussion on this PR is driven by silverstripe/silverstripe-framework#8564 |
|
I'm going to close this - I think if we were to ever merge something like this today it would be using attributes rather than annotations |
4 participants
Adds
AnnotationTransformerwhich taks a list of classes and an array ofAnnotationDefinitionInterfaces that can parse annotations in doc blocks and convert them into configuration.This bumps PHP dependency to 7.1 in line with recent core trends and takes advantage of strict typing in new code.