Entity autocomplete form element ignores entities with label "0"

Created on 24 August 2023, 10 months ago
Updated 14 February 2024, 4 months ago

Problem/Motivation

It is possible to create an entity with label "0". However, this entity will be ignored by the entity autocomplete form element.

Steps to reproduce

Create a new term with name "0". Take any node type and configure a new field that refers to terms. Set any autocomplete widget for this field. Take any node of this type and open the edit form. Input "0" in the field - there are no autocomplete suggestions. After saving the node, the field will remain empty. However, if you input "0 " (trailing space), then the field value will be saved correctly.

Proposed resolution

Do not use empty() to check the form element value. Do not use the (bool) cast to check the typed string from the URL.

🐛 Bug report
Status

Fixed

Version

10.2

Component
Entity 

Last updated about 2 hours ago

Created by

🇷🇺Russia WalkingDexter

Live updates comments and jobs are added and updated live.
  • Novice

    It would make a good project for someone who is new to the Drupal contribution process. It's preferred over Newbie.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @WalkingDexter
  • Status changed to Needs review 10 months ago
  • last update 10 months ago
    30,058 pass
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update 10 months ago
    2,160 pass
  • 🇷🇺Russia WalkingDexter

    Drupal 7 also has this problem with taxonomy autocomplete.

  • 🇷🇺Russia WalkingDexter

    Fix for terms without core patch (Drupal 7).

    /**
     * Implements hook_field_widget_WIDGET_TYPE_form_alter().
     */
    function custom_field_widget_taxonomy_autocomplete_form_alter(&$element, &$form_state, $context) {
      array_unshift($element['#element_validate'], 'custom_taxonomy_autocomplete_validate');
    }
    
    /**
     * Form element validate handler for taxonomy term autocomplete element.
     *
     * @param array $element
     *   The element structure.
     */
    function custom_taxonomy_autocomplete_validate(array &$element) {
      // Fix for single term with name "0".
      if ($element['#value'] === '0') {
        $element['#value'] .= ' ';
      }
    }
    
  • Status changed to Needs work 10 months ago
  • 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.

  • Status changed to Needs review 10 months ago
  • 🇫🇷France nod_ Lille

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

  • Status changed to Needs work 10 months ago
  • 🇺🇸United States smustgrave

    Thank you for reporting.

    Can we get a test case showing the issue please

  • Status changed to Needs review 10 months ago
  • last update 10 months ago
    30,134 pass
  • 🇷🇺Russia WalkingDexter

    Patch with tests.

  • Status changed to RTBC 10 months ago
  • 🇺🇸United States smustgrave

    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.

  • last update 10 months ago
    30,136 pass
  • last update 10 months ago
    30,136 pass
  • last update 9 months ago
    30,146 pass
  • last update 9 months ago
    30,146 pass
  • 15:33
    8:27
    Running
  • 🇺🇸United States smustgrave

    Seems unrelated, retesting

  • last update 9 months ago
    30,150 pass
  • Status changed to Needs work 9 months ago
  • 🇺🇸United States xjm

    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 (<code>isset($element['#value']) && ($element['#value'] === '0' || !empty($element['#value'[))
      

      or perhaps:

      if (<code>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!

  • Status changed to Needs review 9 months ago
  • last update 9 months ago
    30,168 pass
  • 🇷🇺Russia WalkingDexter

    Good points! I updated the patch with minor changes - empty() is not necessary when the variable is defined.

  • 🇩🇪Germany marcoliver Neuss, NRW, Germany

    The updated patch in #14 🐛 Entity autocomplete form element ignores entities with label "0" Needs review 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);
        }
    
  • Status changed to Needs work 9 months ago
  • 🇺🇸United States smustgrave

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

    @marcoliver think that would be good!

  • 🇷🇺Russia WalkingDexter

    @smustgrave Here it is.

  • 🇺🇸United States xjm
  • 🇩🇪Germany marcoliver Neuss, NRW, Germany

    @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.

  • 🇺🇸United States xjm

    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.

  • 🇺🇸United States allisonherodevs

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

  • 🇺🇸United States allisonherodevs

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

  • 🇺🇸United States allisonherodevs

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

  • 🇺🇸United States allisonherodevs

    Updating existing method with test

  • Status changed to Needs review 9 months ago
  • last update 9 months ago
    Custom Commands Failed
  • last update 9 months ago
    Custom Commands Failed
  • Status changed to Needs work 9 months ago
  • 🇺🇸United States smustgrave

    CC failure.

  • Status changed to Needs review 9 months ago
  • last update 9 months ago
    30,361 pass
  • 🇷🇺Russia WalkingDexter

    Improvements for tests.

    I removed [] from the list of empty values because it causes an error:

    1) Drupal\KernelTests\Core\Entity\EntityAutocompleteTest::testEntityReferenceAutocompletion
    Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value "q" contains a non-scalar value.
    
    /var/www/web/vendor/symfony/http-foundation/InputBag.php:37
    /var/www/web/core/modules/system/src/Controller/EntityAutocompleteController.php:82
    /var/www/web/core/tests/Drupal/KernelTests/Core/Entity/EntityAutocompleteTest.php:209
    /var/www/web/core/tests/Drupal/KernelTests/Core/Entity/EntityAutocompleteTest.php:142
    /var/www/web/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
  • last update 9 months ago
    30,354 pass, 3 fail
  • 🇧🇷Brazil ashley_herodev

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

  • last update 9 months ago
    30,360 pass
  • 🇺🇸United States xjm

    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. :)

  • Status changed to RTBC 9 months ago
  • 🇺🇸United States smustgrave

    Confirmed additional test in #15 has been added.

  • last update 9 months ago
    30,360 pass
  • last update 9 months ago
    30,371 pass
  • Status changed to Needs review 9 months ago
  • 🇫🇮Finland lauriii Finland
    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).

  • Status changed to Needs work 9 months ago
  • 🇺🇸United States smustgrave

    Think this would be a good task for a novice user

  • Status changed to Needs review 8 months ago
  • last update 8 months ago
    30,382 pass
  • 🇮🇳India roshni27

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

  • Status changed to RTBC 8 months ago
  • 🇺🇸United States smustgrave

    @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!

  • last update 8 months ago
    30,392 pass
  • 🇮🇳India roshni27

    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.

  • last update 8 months ago
    30,397 pass
  • last update 8 months ago
    30,397 pass
  • last update 8 months ago
    30,414 pass
  • last update 8 months ago
    30,417 pass
  • last update 8 months ago
    30,426 pass
  • last update 8 months ago
    30,427 pass
  • 15:34
    20:10
    Running
  • last update 8 months ago
    Build Successful
  • last update 8 months ago
    30,464 pass
  • last update 8 months ago
    30,483 pass
  • last update 8 months ago
    30,485 pass
  • last update 8 months ago
    30,486 pass
  • last update 7 months ago
    30,488 pass
  • last update 7 months ago
    30,511 pass
  • Status changed to Needs work 7 months ago
  • 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.)

  • Status changed to RTBC 7 months ago
  • 🇺🇸United States smustgrave

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

  • last update 7 months ago
    30,519 pass
  • Status changed to Needs work 7 months ago
  • 🇺🇸United States xjm

    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.

  • Status changed to Needs review 7 months ago
  • 🇮🇳India pradhumanjainOSL

    Made changes as per #39

  • Status changed to Needs work 7 months ago
  • 🇺🇸United States smustgrave

    Please include any patches with an interdiff,

    Also encouraged patches be converted to MRs.

  • First commit to issue fork.
  • Status changed to Needs review 7 months ago
  • 🇮🇳India ankithashetty Karnataka, India

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

    Thanks!

  • Status changed to RTBC 7 months ago
  • 🇺🇸United States smustgrave

    Feedback appears to be have been addressed

  • last update 7 months ago
    30,552 pass
  • 🇺🇸United States xjm

    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.

  • 🇺🇸United States xjm

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

  • 🇺🇸United States xjm

    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.

  • Status changed to Needs review 7 months ago
  • 🇺🇸United States xjm

    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?

  • Status changed to RTBC 7 months ago
  • 🇺🇸United States smustgrave

    Ran EntityAutocompleteTest ::testEntityReferenceAutocompletion and got

    Undefined array key 0

  • 🇺🇸United States xjm

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

  • 🇺🇸United States smustgrave

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

  • 🇺🇸United States xjm

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

  • 🇺🇸United States xjm

    Saving credits.

    • xjm committed 68dfec3c on 11.x
      Issue #3383131 by WalkingDexter, xjm, allisonherodevs, ashley_herodev,...
    • xjm committed eab4bfa9 on 10.2.x
      Issue #3383131 by WalkingDexter, xjm, allisonherodevs, ashley_herodev,...
    • xjm committed 18732679 on 10.1.x
      Issue #3383131 by WalkingDexter, xjm, allisonherodevs, ashley_herodev,...
  • 🇺🇸United States xjm

    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!

  • Status changed to Fixed 7 months ago
    • xjm committed b78c8746 on 10.1.x
      Revert "Issue #3383131 by WalkingDexter, xjm, allisonherodevs,...
    • xjm committed 899166e3 on 10.2.x
      Revert "Issue #3383131 by WalkingDexter, xjm, allisonherodevs,...
    • xjm committed 7ddc1b8e on 11.x
      Revert "Issue #3383131 by WalkingDexter, xjm, allisonherodevs,...
  • Status changed to Needs work 7 months ago
  • 🇺🇸United States xjm

    Sorry, I realized this morning why the test failure probably wasn't being surfaced correctly. 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!

  • Status changed to Needs review 7 months ago
  • Status changed to Needs work 7 months ago
  • 🇺🇸United States smustgrave

    Seems to have a unit test failure now.

  • Status changed to Needs review 7 months ago
  • 🇷🇺Russia WalkingDexter

    Rebased the source branch, now all tests are passed. The test-only results now show the failure for core/tests/Drupal/KernelTests/Core/Entity/EntityAutocompleteTest.php.

  • Status changed to RTBC 7 months ago
  • 🇺🇸United States smustgrave

    LGTM

  • Status changed to Needs work 6 months ago
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

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

  • Status changed to RTBC 6 months ago
  • First commit to issue fork.
  • Status changed to Needs work 6 months ago
  • 🇳🇿New Zealand quietone New Zealand

    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.

  • Status changed to Needs review 6 months ago
  • 🇳🇿New Zealand quietone New Zealand

    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?

  • Status changed to RTBC 6 months ago
  • 🇺🇸United States smustgrave
        // 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.

    • larowlan committed 91c760c5 on 10.2.x
      Issue #3383131 by WalkingDexter, xjm, allisonherodevs, ashley_herodev,...
    • larowlan committed d7537753 on 11.x
      Issue #3383131 by WalkingDexter, xjm, allisonherodevs, ashley_herodev,...
  • Status changed to Postponed 5 months ago
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Committed to 11.x and backported to 10.2.x

    Thanks all

  • 🇷🇺Russia WalkingDexter

    Should the status be "Fixed" instead of "Postponed"?

  • Status changed to Fixed 5 months ago
  • 🇺🇸United States smustgrave

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

  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.69.0 2024