- 🇩🇪Germany donquixote
Also, with this proposed change, it is no longer possible to fully replace or remove the callbacks from '#type'.
A solution could be to introduce a placeholder callback, so that we can:
$element = [ '#type' => 'radios', '#process' => [ 'my_custom_process_callback', Element::PROCESS_CALLBACK_PLACEHOLDER, 'my_custom_late_process_callback', ], ];
and we get this:
$element = [ '#type' => 'radios', '#process' => [ 'my_custom_process_callback', [Radios::class, 'processRadios'], 'my_custom_late_process_callback', ], ];
The placeholder should be a valid callback that, if not replaced, just returns the first parameter.
At the same time, it should be a constant that is easy to compare against or look up in an array.This leaves some design choices to make:
- What exactly is the placeholder value? If it is a static method, how do we call the class and method?
- Where do we do the placeholder replacement? I was thinking of a new service ElementTypePopulator, which handles all of the '#type' replacement, so that we don't have to clutter up Renderer and FormBuilder.(I did post some proof of concept code in a slack convo, but I am still too undecided about the class naming.)
- 🇩🇪Germany donquixote
It seems for all the callback types #process, #after_build, #pre_render and #post_render, the placeholder can be a simple identify function.
fn ($x) => $x
(but as regular named function or static method, not as closure) - 🇬🇧United Kingdom joachim
Can't Element::PROCESS_CALLBACK_PLACEHOLDER be a method that just does this:
1. look at #type in the render array it's working on
2. get the Element plugin class for that #type
3. call ELEMENTCLASS::getInfo()
4. get the callback names from the returned info array
5. call them - 🇮🇳India mdsohaib4242
To resolve this, you need to ensure that your custom process callback is added while preserving the existing callbacks. Modify your my_module_field_widget_form_alter function to append the custom callback to the existing #process array, rather than replacing it.
function my_module_field_widget_form_alter(&$element, \Drupal\Core\Form\FormStateInterface $form_state, $context) { if ($context['items']->getFieldDefinition()->getName() !== 'field_tags') { return; } if (isset($element['#process'])) { $element['#process'][] = '_my_module_field_widget_form_process'; } else { $element['#process'] = ['_my_module_field_widget_form_process']; } }
- 🇩🇪Germany donquixote
@joachim
Can't Element::PROCESS_CALLBACK_PLACEHOLDER be a method that just does this:
1. look at #type in the render array it's working on
2. get the Element plugin class for that #type
3. call ELEMENTCLASS::getInfo()
4. get the callback names from the returned info array
5. call themYes, this would also do the trick.
However, it does require a call to \Drupal::service(), if it is to be a constant.
Or alternatively, we could make this a class with __invoke() and DependencySerializationTrait. Because these things tend to get serialized. Actually not so bad! - 🇩🇪Germany donquixote
call ELEMENTCLASS::getInfo()
Just to clarify, we would not call it like this, because it is not a static method.
Instead, we would call `$info = $this->elementInfoManager->getInfo($element['#type'])` as we already do in FormBuilder and Renderer.
I am working on something, just need to add tests.. - 🇩🇪Germany donquixote
I created a new MR with these placeholder objects following the idea by @joachim.
I want to add tests, but not sure yet where to put them.
- 🇩🇪Germany donquixote
Here we go, some unit tests.
Let's discuss if this is a good direction, and how we can refine it. - 🇩🇪Germany donquixote
We can't use readonly in combination with DependencySerializationTrait.
I am setting to "Needs review" for one round of feedback, but I am sure we will have to change some parts.
- 🇩🇪Germany donquixote
Perhaps we want to move the ElementProcessCallback* classes to `Drupal\Core\Form\` namespace?
For now I had it in `Drupal\Core\Form\Render\Callback`, because form element type plugins are typically in the same namespace as render element type plugins.
I ma not changing it now because I want to see more feedback first. - 🇩🇪Germany donquixote
Can't Element::PROCESS_CALLBACK_PLACEHOLDER be a method that just does this:
Yes, this would also do the trick.
However, it does require a call to \Drupal::service(), if it is to be a constant.
Or alternatively, we could make this a class with __invoke() and DependencySerializationTrait. Because these things tend to get serialized. Actually not so bad!Actually one more problem with this approach.
If multiple operations (e.g. form alter hooks) want to add '#process' callbacks, while preserving the element type default callbacks, they need to make sure that the element defaults are not executed more than once.If the callback is a constant, we can easily check for in_array().
But if it is an object?
Actually, in_array(*, *, FALSE) applies weak object comparison, and this actually seems to work in this case.
But I am a bit afraid of weak object comparisons, I don't trust them. - 🇩🇪Germany donquixote
If multiple operations (e.g. form alter hooks) want to add '#process' callbacks, while preserving the element type default callbacks, they need to make sure that the element defaults are not executed more than once.
The following pattern works in most cases:
$element['#process'] ??= [new ElementTypeProcessCallback($this->elementInfoManager)]; $element['#process'][] = 'my_process_callback';
- 🇺🇸United States nicxvan
@donquixote which MR? The newer one in draft?
@mdsohaib4242 that is exactly what the code in the issue summary is doing, that's why this issue exists.
- 🇩🇪Germany donquixote
I notice that '#pre_render' does not allow object with __invoke() method, if it is not a closure.
See 📌 Support object with __invoke() for #pre_render. Active
(We can eventually handle #pre_render in a separate issue, but for now I find it interesting to include it here as proof-of-concept, to see the bigger picture of where this is going.)
- 🇩🇪Germany donquixote
@donquixote which MR? The newer one in draft?
Well, I am proposing to use the newer MR which is in draft.
Imo the old MR is a dead end, because it breaks existing behavior and makes it impossible to fully remove/replace the callbacks from #type.But I don't want to remove the old one yet, unless everybody agrees on the new direction.
Having both means we can discuss which direction is preferable. - 🇩🇪Germany donquixote
@mdsohaib4242
Actually this does not do anything, because at the time you add your own '#process' callback, the '#process' array is still empty.
The would-be-replaced callbacks are added later by the form builder which is after hook_form_alter() based on $element['#type']. The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.