Entity autocomplete form element ignores entities with label "0"

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch 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.

We can hide patches from the issue summary so that the testbot ignores them if they don't match the issue version.

Thank you for reporting.

Can we get a test case showing the issue please

Thanks for adding those! For next time super helpful to add a test-only patch with the full patch in that order. So we can see the red/green quickly. But not a big deal!

Ran the tests locally without the fix

testValidEntityAutocompleteElement:
Failed asserting that null matches expected '3'.
Expected :3
Actual :null

testEntityReferenceAutocompletion:
Undefined array key 0
/var/www/html/vendor/symfony/phpunit-bridge/DeprecationErrorHandler.php:104
/var/www/html/web/core/tests/Drupal/KernelTests/Core/Entity/EntityAutocompleteTest.php:137
/var/www/html/web/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

So believe the tests are good.

Issue summary is clearly filled out so I think this is good for RTBC bucket.

Seems unrelated, retesting

OK, this bug is hilarious. Great find!

I'd be surprised if there hasn't already been an issue filed for this, but on the other hand I wasn't able to find one myself in a few minutes of searching. (It's harder because 0 is less than three characters and therefore ignored by d.o's issue search.)

1. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -203,7 +203,7 @@ public static function processEntityAutocomplete(array &$element, FormStateInter
   -    if (!empty($element['#value'])) {
   +    if (isset($element['#value']) && $element['#value'] != '') {
   

   My first thought when opening this patch: What about other empty values besides 0 and ''? Let's think through them:

   • NULL is handled by the isset().
   • FALSE is not an expected value by default. However, the element (like all form data structures) is passed around by reference and alterable at every level by every module under the sun, so theoretically someone could have FALSE here for $weird_custom_code_reasons, in which case this would cause this code path to execute when it did not previously.
   • The above also goes for [] (which is also a totally conceivable thing for contrib to alter in since entity references can very logically be multivalue.
   • I'm unsure whether we should support ints and floats (0, 0.0), but it is a behavior change to start executing this code path for them when we didn't previously.

   Therefore, I think this should be changed to something along the lines of:

   if (isset($element['#value']) && ($element['#value'] === '0' || !empty($element['#value']))) {
   

   or perhaps:

   if (isset($element['#value']) && (is_numeric($element['#value']) || !empty($element['#value']))) {
   

   It should also have an inline comment, because it's non-obvious why we're doing this either way. Or, maybe we could add a method? Something like isNotEmpty() or whatever.

2. +++ b/core/modules/system/src/Controller/EntityAutocompleteController.php
   @@ -78,7 +78,7 @@ public static function create(ContainerInterface $container) {
   -    if ($input = $request->query->get('q')) {
   +    if (($input = $request->query->get('q')) != '') {
   

   As above, this should probably be:

   $input = $request->query->get('q');
   if ($input === '0' || !empty($input)) {
   

   or:

   $input = $request->query->get('q');
   if (is_numeric($input) || !empty($input)) {
   

   Or use the method if we add that.

Based on the above, we should probably also add test coverage for other empty value types.

Thanks!

The updated patch in #14 works nicely!

As far as expanding the test coverage goes, would adding this to EntityAutocompleteTest.php suffice?

    $emptyValues = [
      [],
      NULL,
      FALSE,
      0,
      0.0,
    ];
    foreach($emptyValues as $emptyValue) {
      $data = $this->getAutocompleteResult($emptyValue);
      $this->assertEmpty($data);
    }

@WalkingDexter fyi please include an interdiff to help see the changes

@marcoliver think that would be good!

@WalkingDexter Would you mind adding the lines I proposed in #15 (or something similar you like) to your patch / diff? Then we’d also have the expanded test coverage for all the other empty values @xjm suggested in #13.

I’d do it myself, but I don’t have access to a workable setup at this time.

Thanks @marcoliver. #15 is a decent suggestion. Usually I'd suggest using a data provider since that is cleaner, but since core/tests/Drupal/KernelTests/Core/Entity/EntityAutocompleteTest.php is already a kernel test set up to test many API values with a single test setup, it is more performant to just add code along the lines of what you suggest.

Saving issue credits.

I added the suggested additional tests per comment #15 to the EntityAutocompleteTest.php file.

Added the tests to a new method instead of an existing method as I should've -- will update with new patch and interdiff shortly.

Updated the wrong method will post updated interdiff and patch shortly.

Updating existing method with test

CC failure.

Thanks at @WalkingDexter, I have added a test-only patch which should fail to prove the test coverage for #27

Thanks @allisonherodevs and @ashley_herodev! The test-only patch fails in the expected manner. 👍

I forgot to tell Ashley to re-upload the passing patch after the failing one so that it gets retested instead of that. So doing that here. Not my work, just a little housekeeping. :)

Confirmed additional test in #15 has been added.

1. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -203,7 +203,8 @@ public static function processEntityAutocomplete(array &$element, FormStateInter
   +    if (isset($element['#value']) && ($element['#value'] === '0' || $element['#value'])) {
   

   It seems that is_string with strlen or checking explicitly for empty string is how we would check emptiness of strings.

   I think !empty($element['#value']) || (is_string($element['#value']) && strlen($element['#value'])) might be more consistent with that.

2. +++ b/core/modules/system/src/Controller/EntityAutocompleteController.php
   @@ -77,8 +77,12 @@ public static function create(ContainerInterface $container) {
   +    if ($input === '0' || $input) {
   

   Same here, I don't think we need to hardcode '0' here. We could do either is_string($input) && $input !== '' or is_string($input) && strlen($input).

Think this would be a good task for a novice user

Based on the comments and suggestions #32, here's the updated code with more explicit checks for empty strings.

@roshni27 same post as the other issue thank you for working on it. Looking at your post history believe you can work on non-novice issue now. Novice issues are meant to be reserved for new users. Thanks!

Thank you for your feedback! I appreciate your guidance on the issue categorization. I'll make sure to focus on non-novice issues going forward.

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Hiding the other patches but I think this one was small enough that patches should still be fine.

Thanks @lauriii; that's cleaner.

+++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
@@ -203,7 +203,8 @@ public static function processEntityAutocomplete(array &$element, FormStateInter
+    if (isset($element['#value']) && (!empty($element['#value']) || (is_string($element['#value']) && strlen($element['#value'])))) {

Checking isset() is redundant with !empty() because !empty() will always be false if isset() is false. So we can remove the isset() check and simplify the first condition.

Made changes as per #39

Please include any patches with an interdiff,

Also encouraged patches be converted to MRs.

Raised an MR as requested.
Also attaching an interdiff of the patches in #34 and #40.

Thanks!

Feedback appears to be have been addressed

Also remember to queue tests when using patches. Merge requests will queue tests automatically and be much faster.

I queued the tests now, but we have to wait for them to finish running.

Ah, I see there is now an MR. Please remember to hide patch files from the IS when opening an MR. Thanks!

Chasing my own tail here, re-rebasing after I make other commits. Unfortunately the test-only job only works properly if the MR is rebased against the branch tip. This is a good reason for contributors to run the test-only job before RTBCing.

Finally have the test-only results:

Time: 00:03.659, Memory: 4.00 MB
There was 1 failure:
1) Drupal\KernelTests\Core\Entity\Element\EntityAutocompleteElementFormTest::testValidEntityAutocompleteElement
Failed asserting that null matches expected '3'.
/builds/project/drupal/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
/builds/project/drupal/core/tests/Drupal/KernelTests/Core/Entity/Element/EntityAutocompleteElementFormTest.php:288
/builds/project/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!
Tests: 4, Assertions: 56, Failures: 1.

I wonder why core/tests/Drupal/KernelTests/Core/Entity/EntityAutocompleteTest.php shows no failures? I would have expected these two assertions to fail:

    // Try to autocomplete an entity label with '0' character.
    $input = '0';
    $data = $this->getAutocompleteResult($input);
    $this->assertSame(Html::escape($entity_1->name->value), $data[0]['label'], 'Autocomplete returned the first matching entity');
    $this->assertSame(Html::escape($entity_2->name->value), $data[1]['label'], 'Autocomplete returned the second matching entity');

Can someone try it locally?

Ran EntityAutocompleteTest ::testEntityReferenceAutocompletion and got

Undefined array key 0

OK, so it seems like a bug in the test-only job then. Thanks @smustgrave!

Haven't review but saw this in the queue #3395977: Test-only changes reverts changes to test modules could be completely unrelated though

There are no changes to a test module or other such fixture in the MR though.

Saving credits.

I did one more full review of the whole merge request. It looks pretty great!

Committed to 11.x, 10.2.x, and 10.1.x as a patch-safe bugfix. (We were careful to eliminate disruptions by not changing the behavior of empty values other than string-zero.) Thanks everyone for your work on this!

Sorry, I realized this morning why the test failure probably wasn't being surfaced correctly. (Let's call it a working illustration of why doing code reviews for 11 hours straight decreases your effectiveness.) The failure in #50 is a notice. We should make the fail more explicit so that an assertion fails -- the test is not working as intended currently.

Reverted to fix that. Thanks!

Seems to have a unit test failure now.

LGTM

Left a question on the MR, feel free to self-RTBC after answering

I'm triaging RTBC issues.

I gather from #58 that this was reverted because the test-only tests did not fail for EntityAutocompleteTest ::testEntityReferenceAutocompletion. When I run the test-only version locally I get that same result as in #59,

1) Drupal\KernelTests\Core\Entity\EntityAutocompleteTest::testEntityReferenceAutocompletion
Undefined array key 0

/var/www/html/core/tests/Drupal/KernelTests/Core/Entity/EntityAutocompleteTest.php:139

That makes sense because it is testing an empty array. Changing the assertion to test against NULL doesn't make sense to me because that is not the real result from getAutocompleteResult. A simple assertion that the return value is an array would probably work. But before that I want to revert the last change and rebase.

After running test-only changes I see the two expected errors, https://git.drupalcode.org/project/drupal/-/jobs/552187. So, that is working now.

Do we think there is a need to assert that getAutocompleteResult() returned an array with two elements?

    // Try to autocomplete an entity label with the '0' character.
    $input = '0';
    $data = $this->getAutocompleteResult($input);
    $this->assertSame(Html::escape($entity_1->name->value), $data[0]['label'], 'Autocomplete returned the first matching entity');
    $this->assertSame(Html::escape($entity_2->name->value), $data[1]['label'], 'Autocomplete returned the second matching entity');

Believe thats what is being tested here. Unless I'm misunderstanding.

Committed to 11.x and backported to 10.2.x

Thanks all

Believe so as it appears to have been committed to both 11.c and 10.2.x