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 :
- Install Drupal
- Go to the tag vocabulary admin (admin/structure/taxonomy/manage/tags)
- Add term "aaazzz"
- Add term "aaabbb"
- Go to the article creation form (node/add/article)
- Type "aaa" in the "Tags" field
- 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
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
Comment | File | Size | Author |
---|---|---|---|
#45 | drupal-taxonomy-cannot_predict_term_autocomplete_order-tests_only-2374301-45.patch | 6.97 KB | DuaelFr |
#45 | interdiff.2374301.38.45.txt | 1.12 KB | DuaelFr |
#40 | after.png | 106.29 KB | lomasr |
#40 | before.png | 99.49 KB | lomasr |
#38 | drupal-taxonomy-cannot_predict_term_autocomplete_order-tests_only-2374301-38.patch | 6.88 KB | DuaelFr |
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedgreat summary.
Drupal.org is running drupal 7, so this wont effect d.o.
Comment #2
DuaelFrI am working on the tests.
Comment #3
DuaelFrComment #4
DuaelFrHere is the real test-only patch. The previous one contained the fix...
Comment #6
YesCT CreditAttribution: YesCT commentedthis is
Contains \Drupal...
https://www.drupal.org/node/1354#file
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.
properties in a class should use lowerCamel
https://www.drupal.org/node/608152#naming
all methods should have a doc block. This one can have @inheritdoc
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)
english is weird.
this is "a user"
clases have a blank line at the end.
https://www.drupal.org/node/608152#indenting
Fix coming for these nits.
Comment #7
YesCT CreditAttribution: YesCT commentedjust fixing the nits.
Comment #8
YesCT CreditAttribution: YesCT commentedthe 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
Comment #9
YesCT CreditAttribution: YesCT commented@timplunkett helped me with the array syntax in irc.
let's see what the bot says
Comment #10
YesCT CreditAttribution: YesCT commentedthis 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]
Comment #15
YesCT CreditAttribution: YesCT commentedoh 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'")
Comment #16
YesCT CreditAttribution: YesCT commented[edit: wow. I just noticed an extra tests-only crept into my file names. forgive that. will correct on a future try]
Comment #18
dawehnerMaybe we should think about expanding the index
Comment #19
YesCT CreditAttribution: YesCT commentedComment #20
DuaelFr@dawehner: could you explain what you mean in #18? Would it be a solution for this issue or is it a side thought?
Comment #23
DuaelFrThe 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 :
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.
Comment #24
nlisgo CreditAttribution: nlisgo commentedComment #25
nlisgo CreditAttribution: nlisgo commentedRemoving 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.
Comment #26
willzyx CreditAttribution: willzyx commentedIn Drupal\taxonomy\Plugin\EntityReferenceSelection\TermSelection::buildConfigurationForm() you can see:
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
Comment #27
DuaelFrThanks @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.
Comment #28
DuaelFrHere is a new patch that forces the sort on name field for term autocomplete.
Comment #29
DuaelFrComment #33
DuaelFrRerolled the patches and updated the issues queue.
Comment #35
dawehnerThis looks great for me. Personally I cannot be bothered with this bit
It is a test, so what ...
Comment #36
DuaelFrThank you for the review Daniel!
I don't get your comment. Do you want me to remove that property declaration?
Comment #37
dawehnerWell, you could have used camelcasing. Let's just do that so the commmitter don't have to care about it.
Comment #38
DuaelFrDone (with a minor change to avoid mixing old and new syntax for arrays) :)
Can I find the documentation for these "new" coding standards somewhere?
Comment #40
lomasr CreditAttribution: lomasr at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commentedReproduced the issue & applied the patch in #38 . Patch worked cleanly . Please see before and after screenshots.
Comment #41
DuaelFrThank you :)
Comment #42
DuaelFrTestbot is happy and the only changes since @dawehner's review are obvious.
Let's got back to RTBC.
Comment #43
catchentity_get_form_display() and entity_get_display() are both deprecated, let's not introduce new usages
Otherwise looks good to me though.
Comment #44
DuaelFrThese patches you reroll a year later ;)
Comment #45
DuaelFrHere 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
Comment #46
DuaelFrTestbot still happy.
As we just made some minor changes since #35, let's go back to RTBC!
Comment #49
catchI checked the indexes on taxonomy_term_field_data, we should be fine here:
Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!