laminas / laminas-form

Validate and display simple and complex forms, casting forms to business objects and vice versa
https://docs.laminas.dev/laminas-form/
BSD 3-Clause "New" or "Revised" License
80 stars 52 forks source link

Fixes #3 - Element::init is called before Element::setOptions when using Factory but not when using FormElementManager #251

Open froschdesign opened 9 months ago

froschdesign commented 9 months ago
Q A
Documentation no
Bugfix yes
BC Break no
New Feature no
RFC no
QA no

Description

Fixes #3: Element::init is called before Element::setOptions when using Zend\Form\Factory but not when using FormElementManager

Xerkus commented 9 months ago

Hold off merging this. Implications need to be understood.

I remember that creating form via spec is not compatible with custom forms (or elements) that rely on init for self-configuration. Form configuration via spec is meant for generic forms and those do not have init().

froschdesign commented 9 months ago

@Xerkus Unfortunately, this also affects the form builders, which is why I stumbled over it. But rewriting the elements so that the factory creates them correctly cannot be the solution.

Xerkus commented 9 months ago

Laminas\Form\Factory (Factory) modifies state of objects returned by FormElementManager (manager). Element::init() is a part of manager creation flow. Since Factory performs its logic after manager none of it can be used in initializer. For that reason initializer can only depend on what is provided by manager factory or delegators.

laminas-servicemanager plugin manager provides build() which allows to create new instance with specified build parameters which are passed to the plugin manager factories as third argument. So as long as factory supports that... Default InvokableFactory does not but FormElementManager is configured with ElementFactory and it uses ElementFactory for "invokable" from configuration. That leaves factories explicitly defined by users.

One significant difference of build() as compared to get() is that it entirely ignores shared service setting and always creates new instance. Change here is a potential BC break. However FormElementManager is set as not shared by default. If create spec does have options or they are empty the potential impact is further mitigated.

Overall change here makes sense. I am rather inclined to treat this as a new feature rather than a bug fix. Options are already set as they meant to.

I am conflicted about this somewhat because of a surprise factor. Documentation is already pretty unclear or even misleading. With this change spec options become available for init() but nothing else does. Worse, if those options are set via manager factory and then overridden in init() the Factory would proceed to set them a second time as the very last action. I was worried about setting options twice but it does not seem to have any other negative effect.

As a followup of slack discussion I tried to identify needed documentation improvements only some of which should be handled here: