No hook alter to override Field Widget wrappers created by WidgetBase::form

Why does this need work? Patch in #3 works great for me.

I think we also need to add the test for this.

Patch based on #3, including documentation in field.api.php

Hello,

Patch #10 works fine for me

After #2940201: hook_field_widget_form_alter() can no longer affect the whole widget for multi-value fields was committed, #2942097: Deprecate hook_field_widget_form_alter() in favor of hook_field_widget_multiple_form_alter() was proposed, but never happened so far.

So I think to prevent some hook mess, we should deprecate hook_field_widget_form_alter and hook_field_widget_multiple_form_alter in this patch.

I discussed this with @alexpott and we would like to propose the following plan:

1. Introduce hook_field_widget_complete_form_alter

This new hook makes it possible to alter the whole field widget form at the very end.

2. Deprecate hook_field_widget_multiple_form_alter

This hook is then not needed anymore and having it additionally would just be confusing. Also all use-cases catched by hook_field_widget_multiple_form_alter, can be realized with hook_field_widget_multiple_form_alter as well.

3. Rename hook_field_widget_form_alter

To prevent further confusion, we should rename hook_field_widget_form_alter to hook_field_widget_single_element_form_alter, by deprecating hook_field_widget_form_alter and introducing hook_field_widget_single_element_form_alter instead. We think hook_field_widget_single_element_form_alter is still useful because it allows you to just alter a single thing.

Also #2942097: Deprecate hook_field_widget_form_alter() in favor of hook_field_widget_multiple_form_alter() should be closed as “Wont’t fix”.

Thoughts?

Sounds like a great plan to me! I'm not 100% sure about renaming hook_field_widget_form_alter to hook_field_widget_single_element_form_alter, but I guess I can live with it :)

I probably missed some places, but I want to know how much tests are failing.

1. +++ b/core/lib/Drupal/Core/Field/WidgetBase.php
   @@ -119,7 +119,7 @@ public function form(FieldItemListInterface $items, array &$form, FormStateInter
   +    \Drupal::moduleHandler()->alterDeprecated('Implement hook_field_widget_complete_form_alter or hook_field_widget_WIDGET_TYPE_complete_form_alter instead.', [
   

   I think this should be like a proper deprecation message. Saying which version the hook is deprecated in and which it will be removed in. And also point to the change record.

2. +++ b/core/lib/Drupal/Core/Field/WidgetBase.php
   @@ -135,7 +135,7 @@ public function form(FieldItemListInterface $items, array &$form, FormStateInter
   +    $element = [
   

   $element is a interesting variable name - I think we should be verbose here and use $field_widget_complete_form

3. +++ b/core/lib/Drupal/Core/Field/WidgetBase.php
   @@ -149,6 +149,17 @@ public function form(FieldItemListInterface $items, array &$form, FormStateInter
   +    \Drupal::moduleHandler()->alter(['field_widget_complete_form', 'field_widget_' . $this->getPluginId() . '_complete_form'], $element, $form_state, $context);
   

   I think the alter should field_widget_complete_PLUGIN_ID_form

4. +++ b/core/lib/Drupal/Core/Field/WidgetBase.php
   @@ -350,7 +361,8 @@ protected function formSingleElement(FieldItemListInterface $items, $delta, arra
   -      \Drupal::moduleHandler()->alter(['field_widget_form', 'field_widget_' . $this->getPluginId() . '_form'], $element, $form_state, $context);
   

   It's good to be getting rid of this... imagine you have a plugin ID of complete or multivalue.

   I think though that we might have a problem if a site did have a plugin ID of complete. Not sure about the best way of fixing that. Also we have same problem if the ID is single_element... tricky.

5. +++ b/core/lib/Drupal/Core/Field/WidgetBase.php
   @@ -350,7 +361,8 @@ protected function formSingleElement(FieldItemListInterface $items, $delta, arra
   +      \Drupal::moduleHandler()->alterDeprecated('Implement hook_field_widget_single_element_form_alter or hook_field_widget_WIDGET_TYPE_single_element_form_alter instead.', ['field_widget_form', 'field_widget_' . $this->getPluginId() . '_form'], $element, $form_state, $context);
   

   I think this should be like a proper deprecation message. Saying which version the hook is deprecated in and which it will be removed in. And also point to the change record.

6. +++ b/core/lib/Drupal/Core/Field/WidgetBase.php
   @@ -350,7 +361,8 @@ protected function formSingleElement(FieldItemListInterface $items, $delta, arra
   +      \Drupal::moduleHandler()->alter(['field_widget_single_element_form', 'field_widget_' . $this->getPluginId() . '_single_element_form'], $element, $form_state, $context);
   

   I think the alter should be field_widget_single_element_PLUGIN_ID_form

7. +++ b/core/modules/field/field.api.php
   @@ -184,6 +184,9 @@ function hook_field_widget_info_alter(array &$info) {
   + * @deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Use
   + *   hook_field_widget_single_element_form_alter instead.
   + *
     * @see \Drupal\Core\Field\WidgetBaseInterface::form()
     * @see \Drupal\Core\Field\WidgetBase::formSingleElement()
     * @see hook_field_widget_WIDGET_TYPE_form_alter()
   
   @@ -219,6 +222,9 @@ function hook_field_widget_form_alter(&$element, \Drupal\Core\Form\FormStateInte
   + * @deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Use
   + *   hook_field_widget_WIDGET_TYPE_single_element_form_alter instead.
   + *
     * @see \Drupal\Core\Field\WidgetBaseInterface::form()
     * @see \Drupal\Core\Field\WidgetBase::formSingleElement()
     * @see hook_field_widget_form_alter()
   

   Needs an @see to the CR too.

Thanks for the review, @alexpott.

I addressed almost all your points. Not sure what to do about the naming issue...

Here's an initial review:

1. +++ b/core/lib/Drupal/Core/Field/WidgetBase.php
   @@ -149,6 +149,17 @@ public function form(FieldItemListInterface $items, array &$form, FormStateInter
   +    // Allow module to alter the field widget form element.
   

   module -> modules :)

2. +++ b/core/modules/field/field.api.php
   @@ -231,6 +239,141 @@ function hook_field_widget_WIDGET_TYPE_form_alter(&$element, \Drupal\Core\Form\F
    
   +
   +/**
   

   There's an extra empty line here.

3. +++ b/core/modules/field/field.api.php
   @@ -231,6 +239,141 @@ function hook_field_widget_WIDGET_TYPE_form_alter(&$element, \Drupal\Core\Form\F
   + * @param $element
   ...
   + * @param $form_state
   ...
   + * @param $context
   ...
   + * @param $element
   ...
   + * @param $form_state
   ...
   + * @param $context
   

   We should add type hints for these arguments.

4. +++ b/core/modules/field/field.api.php
   @@ -231,6 +239,141 @@ function hook_field_widget_WIDGET_TYPE_form_alter(&$element, \Drupal\Core\Form\F
   + * cannot alter the top level (parent element) for multi-value fields. In most
   + * cases, you should use hook_field_widget_complete_WIDGET_TYPE_form_alter()
   + * instead and loop over the elements.
   

   The existing hook that can only alter individual elements has worked fine in most cases for many years, so I don't think we should recommend that people use the new one instead.

   It's fine to mention it as an alternative, but not really as the "preferred" one.

5. +++ b/core/modules/field/field.api.php
   @@ -231,6 +239,141 @@ function hook_field_widget_WIDGET_TYPE_form_alter(&$element, \Drupal\Core\Form\F
   +  // hook_field_widget_mymodule_autocomplete_form_alter() will only act on
   

   The hook example from the comment needs to be updated to match the new name.

6. +++ b/core/modules/field/field.api.php
   @@ -231,6 +239,141 @@ function hook_field_widget_WIDGET_TYPE_form_alter(&$element, \Drupal\Core\Form\F
   + * Alter forms for field widgets container provided by other modules.
   ...
   + * Alter forms for field widgets container provided by other modules.
   

   'field widgets container' sounds a bit.. vague. I think that the "complete form" of a widget being wrapped in a container element is an implementation detail, so maybe we can come up with a better way to describe what users can alter with this hook :)

I'm still thinking through the naming issues...

Thanks for the review @amateescu

#24 1-3: Fixed
#24 4: I can change that, but it's the same text we had before on hook_field_widget_form_alter
#24 5-6: Fixed

I looked through http://grep.xnddx.ru and couldn't find an existing field type in contrib that is currently named "complete", "single_value" or "multivalue".

Reviewed the patch once more and it looks ready to go :)

#25 Patch Failed to Apply.
Reroll the patch for 9.2.x, please review the patch.

Please ignore #28.

#25 'Patch Failed to Apply'.
Reroll the patch for 9.2.x, please review the patch.

#29 contained some unrelated changes in the dictionary.

I rerolled #25

Ok, fixed the coding style issues

Looked at the diff between 2872162-25.patch and 2872162-31.patch. Changes were regarding assertions and cs.
Lgtm.

Committed 81e2d9c and pushed to 9.2.x. Thanks!

In case someone needs a patch that applies on D8.9. Here it is