Repository navigation
Call onRegistered on elements of a fieldset - #197
BastianLedererIcinga wants to merge 1 commit into
Conversation
Elements of a fieldset now get `onRegistered()` called as well, whether they're added before or after the fieldset is registered.
38ee6a4 to
6b5b510
Compare
nilmerg
left a comment
nilmerg
left a comment
There was a problem hiding this comment.
Sound approach so far, but I was thinking about a new event Form::ON_HANDLE_REQUEST that is somewhat similar to what was FORM::ON_REQUEST before it was changed. The difference to this should be the time it is emitted, as it the form should be already be assembled otherwise the listeners wouldn't make any sense since all of my candidates are elements and thus the only reasonable listener to think about is not the one calling handleRequest in the first place, hence the requirement to assemble.
But anyway, since fieldsets do not get the form's element loader propagated yet, the related ipl-web change is not as useful as I thought it would be so this isn't a priority right now.
| protected $tag = 'fieldset'; | ||
|
|
||
| /** @var ?Form The form this fieldset is registered to */ | ||
| protected ?Form $form = null; |
There was a problem hiding this comment.
please use a WeakReference, since this is only used to be able to tell the fieldset's elements about the form.
| parent::onRegistered($form); | ||
|
|
||
| // Not getElements(), as ipl-web's TermInput assembles itself there, before it's decorated | ||
| foreach ($this->elements as $element) { |
There was a problem hiding this comment.
That's a now obsolete work-around in the TermInput class, so should be dropped in the accompanying change in ipl-web so this can just use getElements()
lippserd
left a comment
lippserd
left a comment
There was a problem hiding this comment.
Please also add tests for forwarding onRegistered() to the elements of a fieldset.
| protected $tag = 'fieldset'; | ||
|
|
||
| /** @var ?Form The form this fieldset is registered to */ | ||
| protected ?Form $form = null; |
| return $event === static::ON_ELEMENT_REGISTERED || $event === static::ON_ASSEMBLED; | ||
| } | ||
|
|
||
| public function onRegistered(Form $form) |
There was a problem hiding this comment.
Please add a docblock. It should say that the call is forwarded to all current and later added elements.
| // Not getElements(), as ipl-web's TermInput assembles itself there, before it's decorated | ||
| foreach ($this->elements as $element) { | ||
| $element->onRegistered($form); | ||
| } |
There was a problem hiding this comment.
This and ipl-web#317 only work together, and Director keeps working with only the changes of both. They land in the same release anyway. So if ipl-web#317 drops its getElements() override, this can use getElements() right away and the comment can go.
Claude's suggestion to allow calling
onRegisteredon elements of a fieldset.Elements of a fieldset now get
onRegistered()called as well, whether they are added before or after the fieldset is registered to a form. Registration does not trigger assembly.