AutocompleteHelper::alterElement() causes duplicates with "#group"

MR is ready for review. Attached is a static patch for composer builds.

code cleanup

Thanks for reporting this issue!
However, I cannot reproduce the behavior where the #process callbacks are added twice. For me, the #process key looks just like it should at the time the process callbacks are invoked, without any duplicates, and processGroup() is consequently also only called once.
Do you maybe have some other modules or custom code installed that might interfere? Alternatively (or even better), could you provide a regression test that demonstrates the faulty behavior?

@drunken monkey - I am using the better_exposed_filters → . It appears that it builds the element with its element info process callbacks before any form alter would be called, for example "search_api_autocomplete_form_views_exposed_form_alter".

MR Updates:
I have added tests to simulate the better_exposed_filters exposed form plugin.
I manually ran the "test-only" tests on the MR and it failed as expected for "There should not be duplicate element #process callbacks."

better_exposed_filters: 6.0.6 or 7.0.2: \Drupal\better_exposed_filters\Plugin\views\exposed_form\BetterExposedFilters::exposedFormAlter().

    // Ensure default process/pre_render callbacks are included when a BEF
    // widget has added their own.
    foreach (Element::children($form) as $key) {
      $element = &$form[$key];
      $this->addDefaultElementInfo($element);
    }

Drupal: 10.3.10
better_exposed_filters: 6.0.6
"#process" IN:

['Drupal\Core\Render\Element\Textfield',' processAutocomplete']
['Drupal\Core\Render\Element\Textfield',' processAjaxForm']
['Drupal\Core\Render\Element\Textfield',' processPattern']
['Drupal\Core\Render\Element\Textfield',' processGroup']

WITH BUG:
"#process" OUT:

['Drupal\Core\Render\Element\Textfield',' processAutocomplete']
['Drupal\Core\Render\Element\Textfield',' processAjaxForm']
['Drupal\Core\Render\Element\Textfield',' processPattern']
['Drupal\Core\Render\Element\Textfield',' processGroup']
['Drupal\search_api_autocomplete\Element\SearchApiAutocomplete',' processSearchApiAutocomplete']
['Drupal\search_api_autocomplete\Element\SearchApiAutocomplete',' processAutocomplete']
['Drupal\search_api_autocomplete\Element\SearchApiAutocomplete',' processAjaxForm']
['Drupal\search_api_autocomplete\Element\SearchApiAutocomplete',' processPattern']
['Drupal\search_api_autocomplete\Element\SearchApiAutocomplete',' processGroup']

WITH THE FIX IN THIS ISSUE:

['Drupal\search_api_autocomplete\Element\SearchApiAutocomplete',' processSearchApiAutocomplete']
['Drupal\search_api_autocomplete\Element\SearchApiAutocomplete',' processAutocomplete']
['Drupal\search_api_autocomplete\Element\SearchApiAutocomplete',' processAjaxForm']
['Drupal\search_api_autocomplete\Element\SearchApiAutocomplete',' processPattern']
['Drupal\search_api_autocomplete\Element\SearchApiAutocomplete',' processGroup']

Wow, thanks for the regression test, that’s very helpful! With this, it is easy to see the problem.

However, I’m not convinced by your proposed solution. In fact, the more I think about it, the more it seems like our previous solution wasn’t very thought-through, either, and the comment explaining the problem somewhere between misleading and plain wrong.

As far as I can see, all the second part of the code in AutocompleteHelper::alterElement() is trying to do is make sure our processSearchApiAutocomplete() callback will be invoked. And if we just look in general at the point in the code where AutocompleteHelper::alterElement() is invoked (completely ignoring the specifics of what BEF might do) there are two possibilities:

1. $element['#process'] is not set. Then everything is fine as FormBuilder will just set the default process callbacks later, at which point our processSearchApiAutocomplete() callback will be included.
2. $element['#process'] is set. Then whatever code set that key is actually responsible for including the defaults as appropriate, otherwise it will already be causing bugs in lots of other places. We have to assume that any default callbacks that are not included are excluded on purpose.
   The problem then is only that whatever code set the #process key could not know about our processSearchApiAutocomplete() callback as the element type was different back then. So, we just have to hope that adding it won’t go against what that code is trying to do. However, there is really no point in adding our parent element’s default callbacks here.

In light of this, and assuming my above reasoning is correct, it seems to me like we should just check whether $element['#process'] is set and, if so, prepend our own processSearchApiAutocomplete() callback.
I changed the MR to this fix, keeping the tests, and it seemed to work fine. Please try it out, and also review! (I made some code style changes to the tests as well.)

@recrit: Thanks a lot for your review, both your comments make a lot of sense. I now amended the MR accordingly, please review!

Luckily, we didn’t even publish a change record about the new required argument for AutocompleteHelper, so hopefully just removing it will be fine. (Anyways, removing a parameter in general seems pretty harmless – unless we add a new one in the future.)

To be on the safe side, I re-added the constructor with a deprecation warning in case it is called with any parameters. That way we will know to handle this properly in case we ever add a new parameter.

@drunken monkey - MR looks good to me and works as expected. There is conflict with src/Utility/AutocompleteHelper.php but other than that looks good. Thanks!

Great to hear, thanks for reporting back!
Fixed the conflict and merged.
Thanks again!