Added form annotation builder factory #6712
Conversation
| @@ -0,0 +1,38 @@ | |||
| <?php | |||
|
|
|||
There was a problem hiding this comment.
license header here
/**
* Zend Framework (http://framework.zend.com/)
*
* @link http://github.com/zendframework/zf2 for the canonical source repository
* @copyright Copyright (c) 2005-2014 Zend Technologies USA Inc. (http://www.zend.com)
* @license http://framework.zend.com/license/new-bsd New BSD License
*/
|
Can you add the form annoation builder factory to the preconfigured stack? |
|
That was one of the outstanding questions: where is it best to add it to to get it into the preconfigured stack? My initial guess was in Zend\Mvc\Service\ServiceManagerConfig, however there seems to be very few classes configured in there so I was wondering if there was a better place? |
|
Perhabs in the skeleton application? Just my thoughts. |
|
Added it to the ServiceListenerFactory, which is where most of the other default MVC factories are. |
|
Ive added methods to allow for setting up custom annotations and listeners in the config. There are no tests yet, but if everyone agrees with the methodology used, I'll create some tests and this PR will be complete. |
|
If/When this PR is accepted: #6754 there is an addition to this PR to allow setting of options for the Annotation builder see this branch: |
|
Good to merge for me! |
carnage
changed the title
| * @param $fullyQualifiedClassName | ||
| * @return $this | ||
| */ | ||
| public function registerAnnotation($fullyQualifiedClassName) |
There was a problem hiding this comment.
This seems to be a SRP violation: simply inject a configured parser instead?
There was a problem hiding this comment.
Currently the parser implementation details are internal to the class, before this enhancement the parser was created inside setAnnotationManager and populated by this class with some default annotations - hiding this complexity from client code.
Whilst I agree that this does seem to be a SRP violation, resolving the violation would be a significant BC break.
There was a problem hiding this comment.
Yes, but you are already solving this issue via configuration and a factory, no? Can't the factory hide this additional complexity?
There was a problem hiding this comment.
I'm solving the complexity of adding custom annotations to the parser using the Factory + config, however the default ones are added by the class.
Current use case for this class is to call new FormAnnotationBuilder in consuming code; prior to this change, it wasn't registered with the service locator. Moving the default annotation registering into the factory would leave code which instantiates directly with an incomplete parser class.
The default annotations could be moved into the factory but doing so would be a big BC break, which would mean this needs to be bumped to v 3.0.0
There was a problem hiding this comment.
Can't you grab the AnnotationParser parser from the AnnotationBuilder inside the factory and registerAnnotation() on it instead?
…nnotations + listeners
…to be more defensive over passed listeners
carnage
force-pushed
the
annotation-factory
branch
from
d1a6393
to
3e89762
Compare
|
Made all the changes you requested; not sure why the build is failing? |
| } | ||
|
|
||
| return $this->annotationParser; | ||
| } |
There was a problem hiding this comment.
Not sure this abstraction is necessary, if there's no way to inject the parser otherwise.
There was a problem hiding this comment.
👍 yes, normal getters and setters are possible. inject the parser in the factory.
There was a problem hiding this comment.
If this were to be removedit would break bc -> users would have to instantiate via the factory or risk not having a parser
There was a problem hiding this comment.
@carnage I missed that it was being used in the factory as well, and, as such, is part of the public API you need to expose. We can keep it as-is.
Added form annotation builder factory Conflicts: library/Zend/Mvc/Service/ServiceListenerFactory.php
|
Merged to develop for release with 2.4. |
Two outstanding items for this pull request: