[random test failure] Random test fail in EntityAutocompleteTest

Nice find. Do we know why this is failing?

Here are some failing values:
$request->query->get('q') = "l`c>&DX and $selection_settings_key = $krg>&73
$request->query->get('q') = "tWv>&\A and $selection_settings_key = eITL>&OU
$request->query->get('q') = "TaM>&Nj and $selection_settings_key = cv/U>&?V

I think this indicates that there is a problem when we set $request->query->get('q') to something beginning with a quote. And indeed changing the code to
$request->query->set('q', '"' . $this->randomString()); will always fail.

So now to work out why.

This is a recently introduced random fail. It's caused by #3264050: Fuzzed tag values to EntityAutocompleteController::handleAutocomplete can cause deprecation warning - I think we can move code around so that the old behaviour is maintained.

Nice sleuth work

We should look to make random string exclude < and > because its bitten us so many times with random fails

@larowlan well here it is the " character. FWIW \Drupal\Tests\RandomGeneratorTrait::randomString() includes '>&' on all random strings over 4 characters long on purpose so I think #7 is past memories of being burnt by that :)

Maybe #1959806: Provide a generic 'entity_autocomplete' Form API element contains info about why we're using Tags::explode() here. That was the issue that added it.

That issue says

It's what the previous code was doing, and it's not weird because we're doing Tags::encode() on every label in EntityAutocompleteMatcher :)

So an alternate fix would be to do $request->query->set('q', Tags::encode($this->randomString())); - however given the autocomplete route is publicly available and a user can easily change the q value to whatever they like I think it might be worth leaving the test as is as it adds a robustness around our expectations.

Also looking at Drupal.autocomplete.autocompleteSplitValues we're going to struggle big time to autocomplete on any label that includes a quote. See #1329742: Autocomplete with tagging silently discards invalid input for more info about the problems that JS code is causing.

Doing a self review I've realised that #3264050: Fuzzed tag values to EntityAutocompleteController::handleAutocomplete can cause deprecation warning has caused another behaviour change and one that it added a test for. Prior to that change setting q to something like "l!J>&Tw would result in it sending some matches back because q is not empty but $typed_string would be. I think we should not be changing behaviour like this in a fix for a random PHP 8.1 issue. Someone somewhere is probably relying on the very odd functionality and changing it without a CR and in random fail fix (that didn't actually fix the random fail) feels wrong.

Ugh... realised that we changed something in #3264050: Fuzzed tag values to EntityAutocompleteController::handleAutocomplete can cause deprecation warning that has a high chance of being relied on somewhere. If you send a single comma as q you used to get a list of entities. Now you don't.

I think if we do want to change this behaviour we need to do it only in a the next minor rather than as a bug-fix and have a CR. The attached patch reverts the functionality to the way it was prior to #3264050: Fuzzed tag values to EntityAutocompleteController::handleAutocomplete can cause deprecation warning but continue to avoid the deprecations being triggered in PHP 8.1.

I think this is critical to land in 9.3.x before the next patch release as it reverts the unintended behaviour changes in #3264050: Fuzzed tag values to EntityAutocompleteController::handleAutocomplete can cause deprecation warning

FWIW #3264050: Fuzzed tag values to EntityAutocompleteController::handleAutocomplete can cause deprecation warning landed in 9.3.7

I'm reviewing this issue and wanted to post a little diff I made. This is the combined diff of the patch in #16 applied on top of the patch that was committed in #3264050: Fuzzed tag values to EntityAutocompleteController::handleAutocomplete can cause deprecation warning. This diff shows the net effect of the two patches and I think offers a clear picture of what's going on here. We can think of this diff as the fix we would make in #3264050: Fuzzed tag values to EntityAutocompleteController::handleAutocomplete can cause deprecation warning if we could go back in time.

The patch in #16 looks great. It uses null coalesce to avoid triggering the php error instead of using an if-block. This then allows an empty string to continue to be processed instead of being skipped, thus removing the unintended behavior changes. The added test coverage here is awesome. RTBC.

That failure on 10.0.x looks random, FWIW.

Committed/pushed to 10.0.x, cherry-picked to 9.4.x and 9.3.x, thanks!