Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

Form Factory is not set when fieldsets are created via options #66

Closed
zluiten opened this issue Apr 29, 2016 · 7 comments
Closed

Form Factory is not set when fieldsets are created via options #66

zluiten opened this issue Apr 29, 2016 · 7 comments
Assignees
Labels
Milestone

Comments

@zluiten
Copy link
Contributor

zluiten commented Apr 29, 2016

I upgraded from 2.5 to the 2.8.1 (with zend-servicemanger 2.7.6).

I'm configuring a Form using the init function. That looks like this

$this->add([
     'type' => 'My\Form\FieldsetWithDefaultElements',
     'elements' => [
          // set some custom elements
     ],
     'fieldsets' => [
          [
               'spec' => [
                    'type' => 'Zend\Form\Element\Collection',
                    'options' => [
                         'target_element' => [
                              'type' => 'Some\Other\Fieldset',
                         ],
                    ],
               ],
          ]
     ],
]);

My\Form\FieldsetWithDefaultElements is instantiated via the FormElementManager inside the create method of the FormFactory. Right after the fieldset is created by the FormElementManager it is configured by the configureFieldset and eventually the prepareAndInjectFieldsets method iterates over the provided fieldsets options key showed above.

At this point the create method is called with the spec key listed above. This method fetches the Zend\Form\Element\Collection from the FormElementManager with the options key as the creationOptions. These creationOptions are processed on construction time because the constructor of the Zend\Form\Element is calling the setOptions method. At this point the Collection doesn't have a Form Factory set yet, since it's set by the FormElementManager (via an initializer) after the Element is constructed. (The FormFactory gets lazy loaded, but there will be no FormElementManager set.)

@zluiten
Copy link
Contributor Author

zluiten commented Apr 29, 2016

Looks the same as #58 . I only searched for this issue in open tickets. Testing with dev-master right now.

@zluiten
Copy link
Contributor Author

zluiten commented Apr 29, 2016

Dev-master (#65) doesn't fix it for me. It comes down to the __invoke method of the ElementFactory instantiating the Collection without setting the FormFactory and FormElementManager first. This is a problem when Some\Other\Fieldset must be constructed with a custom factory to set correct dependencies.

Ping @weierophinney

@weierophinney
Copy link
Member

@Netiul I've got a working unit test for this now, and am working on a solution.

The plan is to override setInvokableClass() in the FormElementManager to add a factory mapping the class to ElementFactory, and aliasing the service to the class. The problem is that the signatures between v2 and v3 differ (v3 makes the second argument optional, allowing you to pass just the class name). As such, I think I'll likely need to create a polyfill solution.

@weierophinney
Copy link
Member

Also, just to clarify something:

It comes down to the __invoke method of the ElementFactory instantiating the Collection without setting the FormFactory and FormElementManager first.

This is not technically what's happening. The FormFactory and FormElementManager are injected after instantiation, by initializers. (The FormElementManager is injected into the FormFactory, and the latter is what is injected, via the injectFactory() initializer method.)

What has happened in the 2.7 and 2.8 series is that, in prepping the component to be forwards compatible with v3 releases of zend-servicemanager, we've hit a number of cases that never had unit tests. These include:

  • The order in which initializers are executed. What several issues reported, and Ensure that the default initializers register last #65 addressed, is that the injectFactory() initializer was being forced to the first position of the initializer queue, and the callElementInit() initializer was being forced to the last position of the queue. This ensured that any required interface injections were injected by initializers prior to calling init(). This was not readily apparent when we did the migration, and, since no tests covered that, we broke it. Ensure that the default initializers register last #65 fixes that particular situation.
  • The second situation is that FormElementManager was overriding the ServiceManager::createFromInvokable() method it inherits in order to alter how form class invokables are handled, ensuring they get both the name and options arguments (instead of just the options, which is how normal invokables are handled). We semi-addressed this with the FormElementManager itself by introducing an ElementFactory with that logic, and mapping all formerly invokable classes to that factory in the factory configuration. However, this breaks user-mapped invokables, as they are now being instantiated using the default zend-servicemanager logic, and no longer getting the options passed to the correct constructor argument. That's the situation I've got a test for and am trying to resolve now.

I would have expected #65 to address the situation you've outlined in your issue, and the fact that it hasn't tells me that this may actually be a different use case. I can use your code snippet from above for a unit test, and will let you know what I find.

@zluiten
Copy link
Contributor Author

zluiten commented Apr 30, 2016

@weierophinney Thanks for clarifying what's changed and why. Very clear!
This morning I decided to compare the two processes of creating the form with zend-form 2.5.2 (where I came from) and current master. It didn't take long, because de difference is very early inside the process, even before the FormElementManager is involved.
Since 2.7.1 the options in the spec as defined in my example above are provided as creationOptions (This is the commit dbef78d).
Basically it is what I defined as the problem, that the options are processed during construction time, which wasn't before 2.5.1 since the options weren't available in the constructor. They were configured after construction via the configureFieldset method of the FormFactory.

@zluiten
Copy link
Contributor Author

zluiten commented Apr 30, 2016

@weierophinney See https://github.com/netiul/zend-form/commit/13a74999d1131339117a831ad90f7d95945e500a for a failing test. Currently this test just fails with a fatal error indicating that a dependency is not set when init is ran. If I'm not letting it crash than the FieldsetWithDependency will be correctly constructed by the configured FormElementManager.

The FormElementManager is configured with a custom factory (FieldsetWithDependencyFactory) to create FieldsetWithDependency.

So in this scenario the target element of a Collection is constructed twice:

  1. First by the ElementFactory (should be the FieldsetWithDependencyFactory!) via a lazy loaded FormFactory and FormElementManager (__construct of Collection -> setOptions).
  2. Second time by the configured FieldsetWithDependencyFactory via the with injectFactory initializer injected FormFactory and FormElementManager.

You will only notice the target is being constructed twice, when the first part is causing a fatal error. Otherwise you'll always end up with the second correct constructed target element which works fine.

It's not easily done writing a proper test for this without good mocking.

weierophinney pushed a commit to weierophinney/zend-form that referenced this issue May 1, 2016
weierophinney added a commit to weierophinney/zend-form that referenced this issue May 1, 2016
This patch removes the change introduced in 2.7.1's
`Zend\Form\Factory::create()` method that aggregated options and passed
them to the `FormElementManager::get()` call. This usage was incorrect,
as it creates an order-of-operations problem: most instances call their
own `setOptions()` method from the constructor if options are present,
and many of the actions this executes assume things like a populated
`FormElementManager`, using a default instance if none is found!

As such, reverting those changes to pre-2.7.1 usage fixes the issues
reported in zendframework#66. Since the changes were done under an erroneous
assumption, a test related to the incorrect functionality was also
removed.
weierophinney added a commit that referenced this issue May 1, 2016
@robob4him
Copy link

robob4him commented May 17, 2016

@weierophinney Does this also pertain to Fieldsets with factories rather than invokables? This problem is similar to my current issue except the main form is created via the FormAbstractServiceFactory and the fieldset has a factory mapping.

Fatal error: Uncaught Zend\ServiceManager\Exception\ServiceNotFoundException: Zend\Form\FormElementManager\FormElementManagerV2Polyfill::get was unable to fetch or create an instance for Service\User\Fieldset in <project>\vendor\zendframework\zend-servicemanager\src\ServiceManager.php on line 941

Prior to 2.7.0 the factory was used. Also, note, however, that the parent service locator actually contains the fieldset; it's the FormElementManager's service locator that does not.

Correction:
The problem was with the lack of configuration according to http://framework.zend.com/manual/2.2/en/modules/zend.form.advanced-use-of-forms.html#comment-1899104120 in which my fieldset factory required an entry under form_elements instead of the main service locator.

Knowing this, is there an understanding for the BC break? Could this be noted somewhere?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants