Related to #2372225: Add sort setting for taxonomy term autocomplete results

Problem/Motivation

As explained in #2372225-10: Add sort setting for taxonomy term autocomplete results the term autocompletion have no explicit orderBy clause for now.
The problem is that, in this case, we cannot predict the order of the results as it will depend on the database engine, its version, the type of the storage and the usage of database indexes.

Doing some tests, drupal.org seems to sort the results ordered by name while a personal D8 install on mySQL 5.6 order them by tid.

As an user, I want to be sure that my terms will be ordered they way I expect they are, probably by term name by default.

Steps to reproduce :

  1. Install Drupal
  2. Go to the tag vocabulary admin (admin/structure/taxonomy/manage/tags)
  3. Add term "aaazzz"
  4. Add term "aaabbb"
  5. Go to the article creation form (node/add/article)
  6. Type "aaa" in the "Tags" field
  7. See that the results are not ordered by name

Proposed resolution

Explicitely add a default orderBy clause on the request retrieving the terms for the autocomple engine.

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch Instructions done
Update the issue summary Instructions done
Add automated tests Instructions Done in #3
Embed before and after screenshots in the issue summary Novice Instructions Done in #40
Review patch approach of fix and tests (and to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards) Instructions

User interface changes

Terms retreived by an autocomplete field will always be filtered by name, no mater what are the specificities of the server on which the site is installed.

API changes

None

CommentFileSizeAuthor
#45 drupal-taxonomy-cannot_predict_term_autocomplete_order-tests_only-2374301-45.patch6.97 KBDuaelFr
#45 interdiff.2374301.38.45.txt1.12 KBDuaelFr
#40 after.png106.29 KBlomasr
#40 before.png99.49 KBlomasr
#38 drupal-taxonomy-cannot_predict_term_autocomplete_order-tests_only-2374301-38.patch6.88 KBDuaelFr
#38 drupal-taxonomy-cannot_predict_term_autocomplete_order-tests_only-2374301-38-tests-only.patch6.11 KBDuaelFr
#38 interdiff.2374301.33.38.txt2.83 KBDuaelFr
#33 drupal-taxonomy-cannot_predict_term_autocomplete_order-tests_only-2374301-33.patch6.89 KBDuaelFr
#33 drupal-taxonomy-cannot_predict_term_autocomplete_order-tests_only-2374301-33-tests-only.patch6.12 KBDuaelFr
#28 interdiff.2374301.15.28-tests-only.txt7.56 KBDuaelFr
#28 drupal-taxonomy-cannot_predict_term_autocomplete_order-tests_only-2374301-28.patch6.89 KBDuaelFr
#28 drupal-taxonomy-cannot_predict_term_autocomplete_order-tests_only-2374301-28-tests-only.patch6.12 KBDuaelFr
#15 drupal-taxonomy-cannot_predict_term_autocomplete_order-tests_only-2374301-14.patch6.58 KBYesCT
#15 drupal-taxonomy-cannot_predict_term_autocomplete_order-tests_only-2374301-14-testsonly.patch5.94 KBYesCT
#15 interdiff.2374301.10.14.txt1.29 KBYesCT
#10 drupal-taxonomy-cannot_predict_term_autocomplete_order-tests_only-2374301-10.patch6.36 KBYesCT
#10 interdiff.2374301.9.10.txt3.45 KBYesCT
#10 drupal-taxonomy-cannot_predict_term_autocomplete_order-tests_only-2374301-10.patch6.36 KBYesCT
#10 interdiff.2374301.9.10.txt3.45 KBYesCT
#9 drupal-taxonomy-cannot_predict_term_autocomplete_order-tests_only-2374301-9.patch6.17 KBYesCT
#9 drupal-taxonomy-cannot_predict_term_autocomplete_order-tests_only-2374301-9-testsonly.patch5.53 KBYesCT
#9 interdiff.2374301.7.9.txt1.53 KBYesCT
#7 drupal-taxonomy-cannot_predict_term_autocomplete_order-tests_only-2374301-7.patch6.33 KBYesCT
#7 interdiff.2374301.3.7.txt5.65 KBYesCT
#4 drupal-taxonomy-cannot_predict_term_autocomplete_order-tests_only-2374301-4.patch5.24 KBDuaelFr
#3 drupal-taxonomy-cannot_predict_term_autocomplete_order-tests_only-2374301-3.patch5.88 KBDuaelFr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

great summary.

Drupal.org is running drupal 7, so this wont effect d.o.

DuaelFr’s picture

Assigned: Unassigned » DuaelFr

I am working on the tests.

DuaelFr’s picture

Assigned: DuaelFr » Unassigned
Issue summary: View changes
Status: Active » Needs review
Issue tags: -Needs tests
FileSize
5.88 KB
DuaelFr’s picture

Here is the real test-only patch. The previous one contained the fix...

Status: Needs review » Needs work
YesCT’s picture

  1. +++ b/core/modules/taxonomy/src/Tests/TermAutocompleteTest.php
    @@ -0,0 +1,149 @@
    + * Definition of Drupal\taxonomy\Tests\TermAutocompleteTest.
    

    this is
    Contains \Drupal...
    https://www.drupal.org/node/1354#file

  2. +++ b/core/modules/taxonomy/src/Tests/TermAutocompleteTest.php
    @@ -0,0 +1,149 @@
    +  /**
    +   * @var \Drupal\taxonomy\Entity\Vocabulary
    +   */
    +  protected $vocabulary;
    

    doc blocks need to have one line summaries.
    https://www.drupal.org/node/1354#var
    ... at least I thought 1354 said they need it. but re-reading it now, I'm not sure.

  3. +++ b/core/modules/taxonomy/src/Tests/TermAutocompleteTest.php
    @@ -0,0 +1,149 @@
    +  protected $field_name;
    

    properties in a class should use lowerCamel
    https://www.drupal.org/node/608152#naming

  4. +++ b/core/modules/taxonomy/src/Tests/TermAutocompleteTest.php
    @@ -0,0 +1,149 @@
    +  protected function setUp() {
    

    all methods should have a doc block. This one can have @inheritdoc

  5. +++ b/core/modules/taxonomy/src/Tests/TermAutocompleteTest.php
    @@ -0,0 +1,149 @@
    +    // Create 11 terms, starting with the same prefix, in a non-alphabetical
    +    // order.
    

    the reason we need 11, is so that there is more than 10 that will match something. (they dont actually need to start with the same prefix, we just later need more than 10 that match something)

  6. +++ b/core/modules/taxonomy/src/Tests/TermAutocompleteTest.php
    @@ -0,0 +1,149 @@
    +    // Create an user then login.
    

    english is weird.
    this is "a user"

  7. +++ b/core/modules/taxonomy/src/Tests/TermAutocompleteTest.php
    @@ -0,0 +1,149 @@
    +  }
    +}
    

    clases have a blank line at the end.
    https://www.drupal.org/node/608152#indenting

Fix coming for these nits.

YesCT’s picture

just fixing the nits.

YesCT’s picture

+++ b/core/modules/taxonomy/src/Tests/TermAutocompleteTest.php
@@ -0,0 +1,162 @@
+    $this->assertEqual('aaa 10', $data[0]['label']);
+    $this->assertEqual('aaa 11', $data[1]['label']);

the test fails,
https://qa.drupal.org/pifr/test/907213
as can nicely be seen from the test only patch in #4

.. are not very semantic.

"Value 'aaa 10' is equal to value 'aaa 20'"

really we want to test if the array of results is equal to the expected results..

I'm going to try and do that. Using another sort order test as a pattern.

I remember #2304651: Core Language sort() should not work, needs to use name instead of default title

YesCT’s picture

YesCT’s picture

+++ b/core/modules/taxonomy/src/Tests/TermAutocompleteTest.php
@@ -0,0 +1,167 @@
+  public function testAutocompleteOrderedResults() {

this one is depending on the count being correct for the sort test to also work. I think we can do a little better to test the sort separate from the count. We just have to match not more than the max that will be returned (10 things).

other nice thing is this also makes sure that it's not only matching beginning of strings. (#1120144: Term Reference Autocomplete widget - add option to match only start of term and expose settings (size, match) related to that idea)

[edit: oops. added the files twice. I'll cancel on of the tests]

Status: Needs review » Needs work
YesCT’s picture

oh no. 9.patch was not supposed to fail.

Value array ( 0 => array ( 'label' => 'aaa 10', ), ... 9 => array ( 'label' => 'aaa 80', ), ) is identical to value array ( 0 => array ( 'value' => 'aaa 10', 'label' => 'aaa 10', ), ... 9 => array ( 'value' => 'aaa 80', 'label' => 'aaa 80', ), ).

oh, there is a value and a label. (heh, and neither are "'name'")

YesCT’s picture

Status: Needs work » Needs review

[edit: wow. I just noticed an extra tests-only crept into my file names. forgive that. will correct on a future try]

dawehner’s picture

+++ b/core/modules/taxonomy/src/Controller/TermAutocompleteController.php
+++ b/core/modules/taxonomy/src/Controller/TermAutocompleteController.php
@@ -180,6 +180,7 @@ protected function getMatchingTerms($tags_typed, array $vids, $tag_last) {

@@ -180,6 +180,7 @@ protected function getMatchingTerms($tags_typed, array $vids, $tag_last) {
       ->condition('vid', $vids, 'IN')
       ->condition('name', $tag_last, 'CONTAINS')
       ->range(0, 10)
+      ->sort('name')
       ->execute();
 

Maybe we should think about expanding the index

YesCT’s picture

Issue summary: View changes
DuaelFr’s picture

@dawehner: could you explain what you mean in #18? Would it be a solution for this issue or is it a side thought?

Status: Needs review » Needs work
DuaelFr’s picture

The patch in #15 cannot be applied anymore as the taxonomy fields have been replaced by entity reference fields in #1847596: Remove Taxonomy term reference field in favor of Entity reference.

I just tried to reproduce this issue on HEAD and it's still there.

Steps to reproduce :

  1. Install Drupal
  2. Go to the tag vocabulary admin (admin/structure/taxonomy/manage/tags)
  3. Add term "aaazzz"
  4. Add term "aaabbb"
  5. Go to the article creation form (node/add/article)
  6. Type "aaa" in the "Tags" field
  7. See that the results are not ordered by name

Next steps :

Find out how entity reference builds it autocomplete requests.
Figure out why taxonomy term reference autocomplete fields does not provide a "sort by" setting like "content" does.

nlisgo’s picture

Issue tags: +Needs reroll
nlisgo’s picture

Issue tags: -Needs reroll

Removing the 'Needs reroll' as comment #23 correctly points out that the taxonomy fields have been replaced by entity reference fields. We need a new test and fix for the entity reference autocomplete.

willzyx’s picture

In Drupal\taxonomy\Plugin\EntityReferenceSelection\TermSelection::buildConfigurationForm() you can see:

// Sorting is not possible for taxonomy terms because we use
// \Drupal\taxonomy\TermStorageInterface::loadTree() to retrieve matches.
$form['sort']['#access'] = FALSE;

sort settings field was intentionally hidden in #2412553: Taxonomy terms in an Entity Reference field are not sorted because sorting does not work with checkbox/radio/select widgets

DuaelFr’s picture

Thanks @willzyx for your help finding out where that's been done (#2412553: Taxonomy terms in an Entity Reference field are not sorted)

The problem is that in TermSelection::getReferenceableEntities(), there is a condition that bypass the TermStorageInterface::loadTree() call in favor of the SelectionBase::getReferenceableEntities() results. These results are not sorted because that "sort by" setting is hidden.

I found a way to fix that for autocomplete widgets. I'm currently rewriting the tests from #15 to be sure it's working fine.

DuaelFr’s picture

DuaelFr’s picture

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

DuaelFr’s picture

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This looks great for me. Personally I cannot be bothered with this bit

+++ b/core/modules/taxonomy/src/Tests/TermAutocompleteTest.php
@@ -0,0 +1,207 @@
+  /**
+   * The autocomplete URL to call.
+   *
+   * @var string
+   */
+  protected $autocomplete_url;

It is a test, so what ...

DuaelFr’s picture

Thank you for the review Daniel!
I don't get your comment. Do you want me to remove that property declaration?

dawehner’s picture

Status: Reviewed & tested by the community » Needs work

Well, you could have used camelcasing. Let's just do that so the commmitter don't have to care about it.

DuaelFr’s picture

Done (with a minor change to avoid mixing old and new syntax for arrays) :)
Can I find the documentation for these "new" coding standards somewhere?

lomasr’s picture

FileSize
99.49 KB
106.29 KB

Reproduced the issue & applied the patch in #38 . Patch worked cleanly . Please see before and after screenshots.

DuaelFr’s picture

Issue summary: View changes

Thank you :)

DuaelFr’s picture

Status: Needs review » Reviewed & tested by the community

Testbot is happy and the only changes since @dawehner's review are obvious.
Let's got back to RTBC.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/taxonomy/src/Tests/TermAutocompleteTest.php
@@ -0,0 +1,207 @@
+    entity_get_form_display('node', 'article', 'default')

entity_get_form_display() and entity_get_display() are both deprecated, let's not introduce new usages

Otherwise looks good to me though.

DuaelFr’s picture

Assigned: Unassigned » DuaelFr

These patches you reroll a year later ;)

DuaelFr’s picture

Here is the patch with the interdiff. I hope this was the usage you had in mind @catch.

Aside, I opened a "follow-up" for a minor docblock issue I discovered while making this patch : #2822730: \Drupal\taxonomy\Tests\TaxonomyTestTrait::createVocabulary() docblock missing @return

DuaelFr’s picture

Status: Needs review » Reviewed & tested by the community

Testbot still happy.
As we just made some minor changes since #35, let's go back to RTBC!

  • catch committed 27bedf8 on 8.3.x
    Issue #2374301 by DuaelFr, YesCT, lomasr: Cannot predict the order of...

  • catch committed 6ee5eb8 on 8.2.x
    Issue #2374301 by DuaelFr, YesCT, lomasr: Cannot predict the order of...
catch’s picture

Status: Reviewed & tested by the community » Fixed

I checked the indexes on taxonomy_term_field_data, we should be fine here:

| taxonomy_term_field_data | 1 | taxonomy_term__vid_name | 1 | vid | A | 2 | NULL | NULL | | BTREE | | |
| taxonomy_term_field_data | 1 | taxonomy_term__vid_name | 2 | name | A | 2 | 191 | NULL | | BTREE | | |
+--------------------------+------------+-----------------------------------------------+--------------+------------------+-----------+-------------+----------+--------+------+------------+---------+---------------+

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

Status: Fixed » Closed (fixed)

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