hook_field_widget_form_alter() can no longer affect the whole widget for multi-value fields

I can't think of a way to make proposed resolution #1 work without breaking any existing code, so here's a review for the current patch:

1. +++ b/core/lib/Drupal/Core/Field/WidgetBase.php
   @@ -241,8 +241,23 @@ protected function formMultipleElements(FieldItemListInterface $items, array &$f
   +      }
   +
   +    }
        return $elements;
   

   The empty line here needs to be moved after the last } :)

2. +++ b/core/modules/field/field.api.php
   @@ -220,6 +222,77 @@ function hook_field_widget_WIDGET_TYPE_form_alter(&$element, \Drupal\Core\Form\F
   + *   - delta: The order of this item in the array of subelements (0, 1, 2, etc).
   

   We can not pass a value for the delta because multiple value widgets handle "all deltas" with a single form element.

3. +++ b/core/modules/field/field.api.php
   @@ -220,6 +222,77 @@ function hook_field_widget_WIDGET_TYPE_form_alter(&$element, \Drupal\Core\Form\F
   + * @see \Drupal\Core\Field\WidgetBase::formSingleElement()
   ...
   + * @see \Drupal\Core\Field\WidgetBase::formSingleElement()
   

   This references needs to point to formMultipleElements() instead.

4. +++ b/core/modules/media/media.module
   @@ -172,3 +173,125 @@ function media_form_field_ui_field_storage_add_form_alter(&$form, FormStateInter
   +  if (!empty($element['target_id']['#type']) && ($element['target_id']['#type'] === 'entity_autocomplete') && ($element['target_id']['#target_type'] === 'media')) {
   

   Since this hook is altering a specific widget (entity_reference_autocomplete), we don't need to check the #type of the form element.

5. +++ b/core/modules/media/media.module
   @@ -172,3 +173,125 @@ function media_form_field_ui_field_storage_add_form_alter(&$form, FormStateInter
   +    // @todo There must be an easier way to get this.
   +    $cardinality = $context['items']->getFieldDefinition()->getFieldStorageDefinition()->getCardinality();
   

   How about setting a #cardinality property in \Drupal\Core\Field\WidgetBase::formSingleElement(), right at the top where we append various stuff to $element?

6. +++ b/core/modules/media/media.module
   @@ -172,3 +173,125 @@ function media_form_field_ui_field_storage_add_form_alter(&$form, FormStateInter
   +function media_field_widget_multiple_entity_reference_autocomplete_form_alter(array &$elements, FormStateInterface $form_state, array $context) {
   

   The entity_reference_autocomplete widget does not handle multiple values, only entity_reference_autocomplete_tags does, so I assume this hook implementation is broken?

And as a general observation, the two hook implementations are duplicating quite a bit of non-trivial code (like access checking), is there a way to re-use some of that code?

You forgot your attachment @xjm :)

+++ b/core/lib/Drupal/Core/Field/WidgetBase.php
@@ -241,8 +241,23 @@ protected function formMultipleElements(FieldItemListInterface $items, array &$f
+      if ($is_multiple) {
+        // Allow modules to alter the field multiple widget form element.
+        $context = [
+          'form' => $form,
+          'widget' => $this,
+          'items' => $items,
+          'delta' => $delta,
+          'default' => $this->isDefaultValueWidget($form_state),
+        ];
+        \Drupal::moduleHandler()->alter([
+          'field_widget_multiple_form',
+          'field_widget_multiple_' . $this->getPluginId() . '_form',
+        ], $elements, $form_state, $context);
+      }
+
+    }

It might need different name but what would be the downside calling the hook from this point always not just if $is_multiple == TRUE.

I thinking if I had some alter logic I wanted to apply to say all entity reference widgets and that logic covered both single value and multi-value fields I would have to implement this hook and hook_field_widget_WIDGET_TYPE_form_alter.

Where as if this was always fired then I could just implement this new hook.

It looks as though #1356824: No way for hook_field_widget_form_alter() to know if it is on a form or in defaults (currently NR for the D7 patch) added code to test hook_field_widget_form_alter() in field/lib/Drupal/field/Tests/FormTest.php and field_ui/lib/Drupal/field_ui/Tests/AlterTest.php, and then #1875992: Add EntityFormDisplay objects for entity forms removed AlterTest.php, and then #2084987: Remove usage of field_ui_default_value and recommend proper replacement added the code to ManageFieldsTest.php.

As far as I can tell from checking the git log, the second bit of test code was missing in between those two commits.

I hope this helps move the issue along a little.

I have implemented the two new hooks in the test module, adding some text to the descriptions. Then I test for the added text. I am having trouble running the tests locally, so I am not sure I got it right. :-(

Please see the comments in the test function.

Edit: this review is for #25.

1. +++ b/core/lib/Drupal/Core/Field/WidgetBase.php
   @@ -241,6 +241,19 @@ protected function formMultipleElements(FieldItemListInterface $items, array &$f
   +        'cardinality' => $cardinality,
   
   @@ -329,6 +344,7 @@ protected function formSingleElement(FieldItemListInterface $items, $delta, arra
   +        'cardinality' => $cardinality,
   
   +++ b/core/modules/field/field.api.php
   @@ -179,10 +184,13 @@ function hook_field_widget_info_alter(array &$info) {
   + *   - cardinality: The cardinality of the field in the field storage. See
   + *     \Drupal\Core\Field\FieldStorageDefinitionInterface for possible values.
   
   @@ -220,6 +234,76 @@ function hook_field_widget_WIDGET_TYPE_form_alter(&$element, \Drupal\Core\Form\F
   + *   - cardinality: The cardinality of the field in the field storage. See
   + *     \Drupal\Core\Field\FieldStorageDefinitionInterface for possible values.
   

   Not sure why we would include the cardinality as a context, isn't it enough that it is available in the $elements FAPI array?

2. +++ b/core/modules/field/field.api.php
   @@ -220,6 +234,76 @@ function hook_field_widget_WIDGET_TYPE_form_alter(&$element, \Drupal\Core\Form\F
   + * \Drupal\Core\Render\Element::children().
   ...
   + * \Drupal\Core\Render\Element::children().
   

   Shouldn't we put $elements as the argument for children()?

3. +++ b/core/modules/field/field.api.php
   @@ -220,6 +234,76 @@ function hook_field_widget_WIDGET_TYPE_form_alter(&$element, \Drupal\Core\Form\F
   + *   \Drupal\Core\Field\WidgetBaseInterface::formMultipleElements().
   ...
   + *   \Drupal\Core\Field\WidgetBaseInterface::formMultipleElements().
   

   WidgetBaseInterface does not have a formMultipleElements(), that's a protected method from WidgetBase.

4. +++ b/core/modules/field/field.api.php
   @@ -220,6 +234,76 @@ function hook_field_widget_WIDGET_TYPE_form_alter(&$element, \Drupal\Core\Form\F
   + *   An associative array. See hook_field_widget_form_alter() for the structure
   

   I think this needs to reference hook_field_widget_multiple_form_alter() instead.

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

   Missing a 'multiple' in the example hook name.

6. +++ b/core/modules/field/src/Tests/FormTest.php
   @@ -634,4 +634,29 @@ public function testLabelOnMultiValueFields() {
   +    \Drupal::state()->set('field_test.widget_form_alter', TRUE);
   
   +++ b/core/modules/field/tests/modules/field_test/field_test.module
   @@ -117,6 +108,21 @@ function field_test_field_widget_form_alter(&$element, FormStateInterface $form_
   +  if (\Drupal::state()->get('field_test.widget_form_alter')) {
   

   The name of this state entry makes it sounds like it's testing the existing widget_form_alter hook, not the new "multiple" one.

#29.1

Not sure why we would include the cardinality as a context, isn't it enough that it is available in the $elements FAPI array?

I agree with @amateescu that having it in the FAPI array is enough.

re @xjm

I actually wasn't sure why we were including it in the FAPI array. The cardinality seems a cleaner place to add it. :)

Did you mean "The context seems a cleaner place to add it."?

I think though it is already in the FAPI array so we should remove it. So I think removing it from the $context is better.

#29 2-5 were fixed

#29.6

The name of this state entry makes it sounds like it's testing the existing widget_form_alter hook, not the new "multiple" one.

Yes this is now fixed in the patch because the actual hook names are used as the state key.

So other than #29.1 this looks good to me.

Another way around this is to use the one-per delta existing hook and add a process callback when the delta is 0. The process callback has access to the complete form by reference

ER in d7 used to do this for Ajax stuff

Would work without any code changes to core

I also think the "can no longer affect" part isn't really true, I don't think this is a regression. In 7.x, influencing the whole widget and not just a single item was much harder as it was not OOP, also for the widgets themself. The only thing that existed was the field behavior constants as used by https://api.drupal.org/api/drupal/modules%21image%21image.field.inc/func... for example.

https://api.drupal.org/api/drupal/modules%21field%21field.api.php/functi... also had delta context, and is called in the loop in https://api.drupal.org/api/drupal/modules%21field%21field.form.inc/funct....

What additionally existed was hook_field_attach_form(), but that is called on the whole form and not just a single field. And in 8.x, you can reproduce that by implement hook_form_alter() and then checking that $form_state->getFormObject() is a ContentEntityForm instance (and not a confirm form, to exclude delete and similar confirmation forms).

That said, I'm +1 on adding this hook, but it might "only" be a DX improvement, not a major bug/regression...

Well this still doesn't allow modifying the outside wrapper. So I think the hook is till the way to go.

It does, a process callback gets the following signature - you have the whole form by reference

function myprocess_callback($element, FormStateInterface $form_state, array &$complete_form) {
  $array_parents = $element['#array_parents'];
  // Remove self.
  array_pop($array_parents);
  $wrapper = NestedArray::getValue($complete_form, $array_parents);
  // Have your way with the $wrapper.
  // ...
  // Set the value back.
  NestedArray::set($complete_form, $array_parents, $wrapper);
  return $element;
}

From FormBuilder

$element = call_user_func_array($form_state->prepareCallback($callback), [&$element, &$form_state, &$complete_form]);

This is one of those things that I see and ask myself "how did we get by without this?"
Which I think we also asked when hook_field_widget_form_alter() was added (also by @xjm!!)

The changes to existing code look good, the added hook looks good, the docs/example look good, and the comparison to the old hook is good.
RTBC!

Patch looks good, bugger that we have to add another hook, but the only other way around it at the moment is to use some Form API voodoo with process callbacks.

Can we get an IS update here, as the 'remaining tasks' seems out of date.

Thanks.

Fixed a couple of things which were not addressed in #37.

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!

The test added in this issue fails when applying #2915036: Display mode configurations don't get updated with new fields

The test added here fails when, while running the test-suite, entity_form_display are loaded via the entityTypeManager. Note that this is only loading of data, nothing is saved here!
In 2915036 this happens like this $this->entityTypeManager->getStorage('entity_form_display')->loadMultiple($form_mode_ids);.

Issue 2915036 loops over all form_displays after saving/updating to assure correct field settings.

I tried to understand what is happening in this test, but I have a really hard to time understanding it.

+++ b/core/modules/field/src/Tests/FormTest.php
@@ -634,4 +637,44 @@ public function testLabelOnMultiValueFields() {
+    FieldConfig::create($this->field)->save();
+    entity_get_form_display($this->field['entity_type'], $this->field['bundle'], 'default')
+      ->setComponent($field_name, [
+        'type' => 'test_field_widget_multiple',
+      ])
+      ->save();
+
+    $this->drupalGet('entity_test/add');
+    $this->assertUniqueText("From $hook(): prefix on field_test_text parent element.");
+    $this->assertText("From $hook(): description on field_test_text child element.");

Is this testing that a specific creating message in shown on the page?

If someone can give me advice where to take a next step that would be appreciated.