Form plugin manager #2820

Closed
wants to merge 5 commits into
from

Projects

None yet

3 participants

@bakura10
Contributor

This PR tries to unify everything and bring plugin manager to Form elements, so that user can specify the name instead of the FCQN, and also to handle dependencies through the SM.

It's not working yet because I need your advice. As you can see, I've modified the factory code so that it uses the plugin manager (and fallback to directly instantiate the element as before if it does not exist in the plugin manager).

However, the problem is that the factory is created in the Form element, but as it does not have access to the service manager, I can't transfer the plugin manager from form to factory. Of course, we can't ask people to create a Form object using the SM.

Furthermore, I introduced back the init function. This is the only way I've found so that adding elements once dependencies are set can be possible. @weierophinney, any thoughts on that ?

The naming is wrong I think too (it should be something like FormElementManager instead of FormPluginManager).

EDIT :

Basically, it will becomes like this (in your controller) :

$form = $this->serviceLocator->get('FormElementManager')->get('Application\Form\Create');
@Slamdunk
Contributor

As for my issue #2792 I've implemented something similar: you have to adapt also Zend\Form\Factory::create(), otherwise it will always fail due to self::isSubclassOf() usage.

The solution to mantain BC it's not ease and clean: for now I'm using this https://gist.github.com/3931593 . I don't like it very much but preserves BC.

I think a bigger refactor is needed.

@bakura10
Contributor

Ha... I can see the problem with this isSubclassOf. But I think it's easy to modify this without BC. But for me the main problem is : how to have the SM in Form object (so that it can be given to the Factory).

@bakura10
Contributor

I may do this through a static variable like this : Factory::setFormPluginManager(...). What do you think ? This is the only simple solution I can see here.

@bakura10
Contributor

@Slamdunk @jurians : what about that ? I've changed the way the factory constructs elements. The tests pass and it should not do any major BC.

There is a small BC because I removed the createFieldset/createElement/createForm function and they became configureFieldset/configureElement/configureForm. I'll try to come up with a solution so that there is no BC at all.

@bakura10
Contributor

And voila !! No BC at all ;-).

If everyone is happy with this solution, I'll write some tests...

@Slamdunk
Contributor

Please no static!

The simple solution is to make the form plugin manager the main accessor to form:

// In a controller
$forms = $this->getServiceLocator()->get('form_plugins');

// In the 'form_plugins' factory service
public function createService(ServiceLocatorInterface $serviceLocator)
{
    $plugins = new PluginManager();
    $plugins->setServiceLocator($serviceLocator);

    // [...]

    $plugins->addInitializer(function($instance) use ($plugins) {
        // The "method_exists" part should not exists,
        // I've already opened a PR for that:
        // https://github.com/zendframework/zf2/pull/2793
        if ($instance instanceof FormFactoryAwareInterface or method_exists($instance, 'setFormFactory')) {
            $instance->setFormFactory(new Zend\Form\Factory($plugins));
        }
    });

    return $plugins;
}

So the new PluginManager is either the main accessor and the final element manager.

PS: please rebase your branch to develop

@Slamdunk
Contributor

Oh, another tip: In my cases, I never want a shared Form\Element, so:

    protected $shareByDefault = false;

it's needed

@bakura10
Contributor

I don't understand what you mean. You mean that every time the user want to create a form in a controller it should fetch it from Service Manager ? Plus it would mean the user would have to add an invokables for every form. I really don't like it, I like the fact that you can do this in your controller :

$form = new CreateForm();

I don't see why static would be bad here. It can have use cases, I think it's good here.

I'm ok about the shareByDefault, I'll change it :).

@Slamdunk
Contributor

Zend\ServiceManager\AbstractPluginManager has $autoAddInvokableClass = true;, no need to add invokables for every form; it was intentionally created so.

Yes I mean you should always fetch forms from FormPluginManager (not SM).

That's because Form\Factory is a factory for every ElementInterface, so even FieldsetInterface and FormInterface.
So, if the main Factory is built and meant to use FormPluginManager for EVERY form-releated element, why should not the user use it as well?

@bakura10
Contributor

Do you prefer like this ?

So if I understand correctly, such thing :

!php
$form = new CreateForm();

has to be replaced by :

!php
$form = $sm->get('form_elements')->get('MyApp\Form\CreateForm');

(note : this is not a BC, as people will still be able to use the old construction style, and even be able to specify short name for elements : Email instead of Zend\Form\Element\Email, but to handle dependencies for their own elements, they have to fetch it from service manager).

@bakura10 bakura10 referenced this pull request in doctrine/DoctrineModule Oct 23, 2012
Merged

Provides ObjectRepository for forms #114

@Slamdunk
Contributor

Let's clear the thoughts: I do not prefer anything, I'm trying to follow the logic. As said, if you use a Service Locator pattern (the FormPluginManager) to manage a set of Entities (the Forms) like you do in your Factory, I do expect you are using it everywhere you need to communicate with such entities.

The introduced verbosity may be reduced by a controller plugin as a proxy to the FormServiceLocator (https://gist.github.com/3938432), so you will be able to write:

$form = $this->form('My\Form');
@bakura10
Contributor

I understand :). Anyway, I've added the FormElementManager to ServiceListenerFactory, so you can do this to :

$this->serviceManager->get('FormElementManager')->get('...').

The form plugin is a good idea, though I'll wait for mwop for something like this.

@Slamdunk
Contributor

Anyone can use write and use the controller plugin i've linked, no need to implement it in Zend: my post was intended to avoid focusing on what's the shortest-in-use solution, and look on the most logical one instead :>

@Slamdunk
Contributor

I think hydrators can't be pulled from SM, cause of this: https://github.com/bakura10/zf2/blob/83e84b9ea086b777f37b0292895a4c382f9a08a1/library/Zend/Form/FormElementManager.php#L105-107

But it will be a nice feature too; often hydrators are not simply a type-transformer, but also a fetcher from a service.

@bakura10
Contributor

I don't pull the hydrator from the form element manager but from the base service manager so this check is not done :).

Envoyé de mon iPhone

Le 28 oct. 2012 à 07:33, Filippo Tessarotto notifications@github.com a écrit :

I think hydrators can't be pulled from SM, cause of this: https://github.com/bakura10/zf2/blob/83e84b9ea086b777f37b0292895a4c382f9a08a1/library/Zend/Form/FormElementManager.php#L105-107

But it will be a nice feature too; often hydrators are not simply a type-transformer, but also a fetcher from a service.


Reply to this email directly or view it on GitHub.

@weierophinney
Member

@bakura10 I hate to say it, but this is impossible to review in its current state; it's got some errant merge artifacts that are inflating the diff. Can you create a new branch off of develop and cherry-pick in your commits for me?

@bakura10
Contributor

Better @weierophinney ? I hope I did not forgot any code while doing this...

@weierophinney weierophinney commented on the diff Nov 9, 2012
library/Zend/Form/Element.php
@@ -66,6 +66,16 @@ public function __construct($name = null, $options = array())
}
/**
+ * This function is automatically called when creating element with factory. It
+ * allows to perform various operations (add elements...)
+ *
+ * @return void
+ */
+ public function init()
+ {
+ }
@weierophinney
weierophinney Nov 9, 2012 Member

I'm assuming this is an implementation detail, and not required by the interface, correct?

@bakura10
bakura10 Nov 9, 2012 Contributor

Yes. At first I added it in the interface but I remembered that you told me not to. So now the Factory just checks if there is a method called init, and call it. Maybe we should even remove it from the Element and let people add it if they want.

@weierophinney weierophinney commented on an outdated diff Nov 9, 2012
library/Zend/Form/Factory.php
@@ -72,16 +111,25 @@ public function create($spec)
$spec = $this->validateSpecification($spec, __METHOD__);
$type = isset($spec['type']) ? $spec['type'] : 'Zend\Form\Element';
- if (self::isSubclassOf($type, 'Zend\Form\FormInterface')) {
- return $this->createForm($spec);
+ // Try to instantiate from the plugin manager, and then fallback to manual instanciation
+ $formElementManager = $this->getFormElementManager();
+
+ if ($formElementManager->has($type)) {
+ $element = $formElementManager->get($type);
+ } else {
+ $element = new $type();
+ }
@weierophinney
weierophinney Nov 9, 2012 Member

Maybe extract the above into a separate method?

@weierophinney weierophinney commented on an outdated diff Nov 9, 2012
library/Zend/Form/Factory.php
@@ -139,11 +222,17 @@ public function createElement($spec)
$element->setAttributes($attributes);
}
+ // Hook to perform stuff, once all the configuration work has been done
+ // TODO for ZF3 : move init to ElementInterface
+ if (method_exists($element, 'init')) {
+ $element->init();
+ }
@weierophinney
weierophinney Nov 9, 2012 Member

How about instead have a separate interface, "InitializableElementInterface", with the "init()" method? (Alternately, create a Zend\Stdlib\InitializableInterface for that.) That allows for typehinting, and then the ElementInterface in ZF3 can extend it -- or it can remain an opt-in feature.

@weierophinney weierophinney commented on an outdated diff Nov 9, 2012
library/Zend/Form/Factory.php
+ // Try to pull from service locator first
+ if (is_string($hydratorOrName)) {
+ $serviceLocator = $this->getFormElementManager()->getServiceLocator();
+ if ($serviceLocator->has($hydratorOrName)) {
+ $hydrator = $serviceLocator->get($hydratorOrName);
+ } else {
+ if (!class_exists($hydratorOrName)) {
+ throw new Exception\DomainException(sprintf(
+ '%s expects string hydrator name to be a valid class name; received "%s"',
+ $method,
+ $hydratorOrName
+ ));
+ }
+
+ $hydrator = new $hydratorOrName;
+ }
}
@weierophinney
weierophinney Nov 9, 2012 Member

Let's extract the above into a separate method.

@weierophinney weierophinney and 1 other commented on an outdated diff Nov 9, 2012
library/Zend/Form/Factory.php
- */
- protected static function isSubclassOf($className, $type)
- {
- if (is_subclass_of($className, $type)) {
- return true;
- }
- if (version_compare(PHP_VERSION, '5.3.7', '>=')) {
- return false;
- }
- if (!interface_exists($type)) {
- return false;
- }
- $r = new ReflectionClass($className);
- return $r->implementsInterface($type);
- }
-}
@weierophinney
weierophinney Nov 9, 2012 Member

Why did you remove the above? Is it no longer used?

@bakura10
bakura10 Nov 9, 2012 Contributor

Nope. No longer used.

@weierophinney weierophinney was assigned Nov 9, 2012
@weierophinney
Member

@bakura10 This is looking great -- if you can address the notes I left earlier, I think we can go ahead and merge for 2.1!

@bakura10
Contributor
bakura10 commented Nov 9, 2012

Done !

@weierophinney weierophinney commented on the diff Nov 13, 2012
library/Zend/Form/Factory.php
@@ -30,6 +30,20 @@ class Factory
protected $inputFilterFactory;
/**
+ * @var FormElementManager
+ */
+ protected $formElementManager;
+
+ /**
+ * @param FormElementManager $formElementManager
+ */
+ public function __construct(FormElementManager $formElementManager = null) {
+ if ($formElementManager) {
@weierophinney
weierophinney Nov 13, 2012 Member

CS - move the opening brace to the next line, please.

@weierophinney weierophinney commented on the diff Nov 13, 2012
library/Zend/Form/Factory.php
+ }
+
+ /**
+ * Try to pull hydrator from service manager, or instantiates it from its name
+ *
+ * @param string $hydratorName
+ * @return mixed
+ * @throws Exception\DomainException
+ */
+ protected function getHydratorFromName($hydratorName)
+ {
+ $serviceLocator = $this->getFormElementManager()->getServiceLocator();
+
+ if ($serviceLocator->has($hydratorName)) {
+ $hydrator = $serviceLocator->get($hydratorName);
+ } else {
@weierophinney
weierophinney Nov 13, 2012 Member

Return immediately after you have the hydrator.

@weierophinney weierophinney commented on the diff Nov 13, 2012
library/Zend/Form/Factory.php
+ * @return mixed
+ * @throws Exception\DomainException
+ */
+ protected function getHydratorFromName($hydratorName)
+ {
+ $serviceLocator = $this->getFormElementManager()->getServiceLocator();
+
+ if ($serviceLocator->has($hydratorName)) {
+ $hydrator = $serviceLocator->get($hydratorName);
+ } else {
+ if (!class_exists($hydratorName)) {
+ throw new Exception\DomainException(sprintf(
+ 'Expects string hydrator name to be a valid class name; received "%s"',
+ $hydratorName
+ ));
+ }
@weierophinney
weierophinney Nov 13, 2012 Member

Once you've incorporated the previous comment, this can be moved out of the else condition.

@weierophinney weierophinney added a commit that referenced this pull request Nov 13, 2012
@weierophinney weierophinney [#2820] Incorporate feedback
- CS fixes
- logic fixes in getHydratorFromName()
88b22d9
@weierophinney weierophinney added a commit to zendframework/zend-servicemanager that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'feature/form-plugins' into develop 0eb7480
@weierophinney weierophinney added a commit to zendframework/zend-modulemanager that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'feature/form-plugins' into develop 3085bbe
@weierophinney weierophinney added a commit to zendframework/zend-stdlib that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'feature/form-plugins' into develop f1b95e9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment