Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The EntityReferenceAutocompleteWidget
will always return a maximum of 10 results, which is caused by the call $entity_labels = $handler->getReferenceableEntities($string, $match_operator, 10);
in Drupal\Core\Entity\EntityAutocompleteMatcher::getMatches()
.
Proposed resolution
Make the match size limit configurable as is the case with the operator.
Remaining tasks
None.
User interface changes
New "Number of results" setting on entity reference autocomplete widgets. Defaults to 10.
Before:
After:
API changes
N/A
Data model changes
match_limit setting added to entity reference autocomplete/tags field widget config schema.
Comment | File | Size | Author |
---|---|---|---|
#180 | Screenshot 2019-12-14 at 12.04.04.png | 38.91 KB | markconroy |
#180 | Screenshot 2019-12-14 at 12.03.52.png | 48.1 KB | markconroy |
#167 | interdiff-2863188-151-167.txt | 1.14 KB | acbramley |
#167 | 2863188-167.patch | 31.59 KB | acbramley |
Comments
Comment #2
selvira CreditAttribution: selvira commentedI'm going to work on this!.
Greetings.
Comment #3
Erik Frèrejean@Maxfire,
Missed your remark about working on it (thought I assigned the ticket).
I was just packaging one of our internal patches for publication as attached. However if you know a better solution I'd like to hear :).
Comment #4
Erik FrèrejeanComment #5
selvira CreditAttribution: selvira commentedOk, don't worry, i'm going to unassign the issue...
regards.
Comment #7
Erik FrèrejeanSeems I forgot to add the schema change to the patch.
Comment #9
_Archy_ CreditAttribution: _Archy_ at PitechPlus commentedComment #11
Erik FrèrejeanI noticed that I missed a change in the schema.
Comment #12
Erik FrèrejeanFix coding style issue.
Comment #13
cilefen CreditAttribution: cilefen as a volunteer commentedComment #14
sathish.redcrackle CreditAttribution: sathish.redcrackle at Red Crackle commentedRerolled patch for Drupal 8.5.x
Tested:
Comment #15
Erik FrèrejeanI just realised that the description with the settings field says
, however the widget itself would reset to the default 10, in case of "empty". Updated patch attached.
Comment #16
sathish.redcrackle CreditAttribution: sathish.redcrackle at Red Crackle commentedComment #18
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedComment #20
borisson_I think this needs an upgrade path?
Comment #21
hchonovand when the update path is ready this fallback will not be needed anymore.
Comment #22
hchonovNeeds work for adding an update.
Also when preparing the update path, you should consider field widgets extending from the
EntityReferenceAutocompleteWidget
. This means all the widgets have to be loaded and checked if they are instance of that class. Based on the found widgets then we have to search for all entity form displays, where the widgets are used and add the new setting to each component using such a widget. A similar update but for fields isfield_update_8003()
, where we check withinstanceof
.Comment #23
Maouna CreditAttribution: Maouna at bio.logis Genetic Information Management GmbH commentedComment #24
Maouna CreditAttribution: Maouna at bio.logis Genetic Information Management GmbH commentedAs suggested in #20 and #22, I added the update.
I disagree with #21, as getMatches is public, the fallback is still needed.
Comment #26
Maouna CreditAttribution: Maouna at bio.logis Genetic Information Management GmbH commentedComment #27
Maouna CreditAttribution: Maouna at bio.logis Genetic Information Management GmbH commentedComment #29
Maouna CreditAttribution: Maouna at bio.logis Genetic Information Management GmbH commentedComment #31
Maouna CreditAttribution: Maouna at bio.logis Genetic Information Management GmbH commentedComment #32
kfritscheReviewed and tested and locks pretty good.
Just a small code style issue
better and shorter would be
Also where I'm unsure is the location of the update. Its definitly okay in entity_reference, but isn't it a post_update? Second opinion here would be nice.
Comment #33
hchonovAs the EntityReferenceAutocompleteWidget is defined in the core system, the update should be placed in the system module. The update saves config entities and therefore it should be a post update instead a hook_update_N.
Additionally we should use the new helper class for updating config entities as described in https://www.drupal.org/node/2949630 .
Comment #34
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedActually, updates for field types, widgets and formatters provided by core should be placed in the Field module :)
Comment #35
hchonovExcuse me for questioning this, but isn't it possible to have a system where the field module isn't enabled, but only base fields are used?
Comment #36
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedSure, it's possible, but highly unlikely. We're already putting too much stuff in the system module so handling field-related updates in the field module was a "good enough" compromise.
Comment #37
tstoecklerEven if it's highly unlikely, I don't think I agree that's it's "good enough" to potentially leave sites in a broken state (because the updates won't run). Maybe we can get some more opinions on this.
Comment #38
hchonovWell in this case I am fine with this decision. Thank you for the clarification, @amateescu!
Comment #39
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@tstoeckler, this is how we've handled field type / widget / formatter related updates since Drupal 8.0.0, see
field.install
andfield.post_update.php
, and there's no bug report about it.Comment #40
tstoecklerWell, I guess that means that noone was aware that it could cause broken sites. I for one was convinced that field.module was in fact a required module, which would make the whole point moot, but apparently that's not the case. So while I'm all for consistency, I think lack-of-brokenness trumps that, so I still disagree with putting in field.module. I'm fine with punting that to a generic policy follow-up to no longer put field-updates in field.module, if you feel strongly that the update should be in field.module for now.
Comment #41
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedOk, system module it is then :)
Comment #42
vasi1186 CreditAttribution: vasi1186 at Amazee Labs commentedComment #43
vasi1186 CreditAttribution: vasi1186 at Amazee Labs commentedI updated the patch so that the update hook is actually a post update of the system module.
Comment #44
vasi1186 CreditAttribution: vasi1186 at Amazee Labs commentedComment #45
tstoecklerThanks for pushing this forward! I think this is close, but marking needs work for the second point below.
Super-nitpick: This can just be
if ($match_size)
I think this should now use the batch update mechanism that was recently introduced. See https://www.drupal.org/node/2949630
Comment #46
Erik FrèrejeanRerolled #43, against 8.6.x-dev as
\Drupal\Tests\rest\Functional\EntityResource\EntityFormDisplay\EntityFormDisplayResourceTestBase
has been deprecated and the actual test now resides in\Drupal\FunctionalTests\Rest\EntityFormDisplayResourceTestBase
, https://www.drupal.org/node/2971931. And added the changes suggested in #45.Comment #47
Erik FrèrejeanNoticed that I had the interdiff the wrong way around...
Comment #50
tstoecklerPatch looks great now, but unfortunately some tests fail now.
Comment #52
chr.fritschThe test fail came from
EntityReferenceAutocompleteWidget::defaultSettings()
. There was the size parameter specified as a string.That change is a bit out of scope here, but it will at least fix the tests.
I also added the match_size parameter to more displays.
Comment #53
Erik FrèrejeanThanks for fixing the tests.
Comment #54
alexpottmatch_size
is a confusing configuration key. Something likematch_limit
would be better. Perhaps someone has a better suggestion. But without changing this I would be confused aboutsize
andmatch_size
and wonder how they are related.I think we can use
is_a($plugin_definition['class'], EntityReferenceAutocompleteWidget::class, TRUE)
to simplify the if.
Comment #55
Erik FrèrejeanComment #56
Erik Frèrejeanmatch_size
tomatch_limit
, which seem a proper name to me.Comment #57
hchonov=> Maximum number of ... / Number of maximum ...
Comment #58
Erik FrèrejeanYou're right.
Comment #59
hchonovNow that the review from @alexpott has been addressed, I think that the patch is ready.
Comment #60
alexpottI think this can be written as
The nice thing about doing it like that is that we prove the any entity display in uninstalled modules would be have the default value added when they are saved too.
Comment #61
jibranIS needs an update there is a UI change, API addition maybe and config data model change. We also need a change notice.
We should use
insatanceof
so that all the widgets inherited from this class should also get an update.Here are some of the widgets inherited from this class.
Comment #62
alexpott@jibran is_a() does exactly what we want - see http://php.net/manual/en/function.is-a.php - also you can't use
instanceof
with strings on the left hand side. Withis_a()
you can.Comment #63
alexpott@jibran to quote the docs
Comment #64
hchonov@alexpott, could you please elaborate on this?
Comment #65
Erik FrèrejeanIf I read @alexpott correctly he seems to mean that it isn't needed to explicitly update/set the component but just letting it save should be sufficient.
I just ran a quick test but that doesn't seem the case, using a clean "standard" D8.7 install and the tags field on the default article node type. Comparing the display options before and after the update, the
match_limit
wasn't saved when using the simplified version.Now it might be possible that the default values should be saved, but then there is obviously a different bug at work here.
Comment #66
tstoecklerHere's a re-roll, did not address #60 / #65 yet.
Re @alexpott: Are you suggesting that we add some runtime code in
EntityFormDisplay::preSave()
that adds the default value to widgets similar toView::fixTableNames()
?Comment #67
larowlanthis will always be set because of the default?
Coming here from #3024404: Support configuring the number of matches returned by Entity Reference Autocomplete widget where we had basically the same approach (which is nice)
Comment #68
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedElsewhere, saving the config entity was enough to update all plugins to use their default configuration if it's not already present.
I think this case
\Drupal\Core\Plugin\DefaultLazyPluginCollection::getConfiguration
attempts to callgetConfiguration
on all widget instances. It looks like widgets use the deprecated\Drupal\Core\Field\PluginSettingsInterface
instead of\Drupal\Component\Plugin\ConfigurablePluginInterface
, so this might be a reason (aside from entity displays being a bit special), that this wouldn't happen automatically.Comment #69
acbramley CreditAttribution: acbramley at PreviousNext commentedWe don't need this check anymore since the default value is being set.
It won't be if it's set to 0, maybe we should add an else in there and tell the user that all results will be shown.
Comment #70
acbramley CreditAttribution: acbramley at PreviousNext commentedWorking on fixes for #69
Comment #71
acbramley CreditAttribution: acbramley at PreviousNext commentedFixes #69, I also tested the update hook with what @alexpott suggested in #60 and it did work as expected (I ensured that the config entity had the match limit setting set).
Comment #72
acbramley CreditAttribution: acbramley at PreviousNext commentedWeird, not sure how it worked the first time but I retested the simplified update hook and indeed it didn't add the config. Reverted back to the update hook from #66, no other changes from #71
Comment #73
tstoecklerThis change should be reverted. Because
getMatches()
is a public function, making$selection_settings['match_limit']
a required key, where it wasn't before, is an API change.Comment #74
acbramley CreditAttribution: acbramley at PreviousNext commentedAh, yes you're right. Reverted, now with a new interdiff.
Comment #75
acbramley CreditAttribution: acbramley at PreviousNext commentedwould help if I uploaded the patch.
Comment #78
Erik FrèrejeanAdded CR: https://www.drupal.org/node/3024684.
Comment #79
Erik FrèrejeanPatch in #75 looks fine, but can't test it atm so I'll leave it to needs review for now.
Comment #81
larowlannit > 80
Comment #82
acbramley CreditAttribution: acbramley at PreviousNext commentedFixes #81
Comment #83
jibranLet's update IS then we can RTBC it.
Comment #84
acbramley CreditAttribution: acbramley at PreviousNext commentedUpdated, please make any further changes required.
Comment #86
jibranLet's fix the failing test.
Comment #87
acbramley CreditAttribution: acbramley at PreviousNext commentedFixes the failure.
Comment #88
jibranGreat work thanks. I think we are done here.
Comment #89
sathish.redcrackle CreditAttribution: sathish.redcrackle at Red Crackle commentedComment #90
larowlanThis is a feature request, not a bug fix - unless someone can convince me otherwise
Comment #91
larowlanDoes this mean the 'unlimited' string here cannot be translated? If so, I think we'll need to put the ternary outside the t call, with two different strings.
Should we be using $this->t() here instead of t() (even though the code around it uses t())
Comment #92
acbramley CreditAttribution: acbramley at PreviousNext commentedFixes #91 in both instances where t() was being used.
Comment #93
hchonovThis however leads to two different translation strings. Could we probably pass
$this->t("unlimited")
as the@size
argument instead?Comment #94
hchonoventity_get_form_display()
is deprecated and instead we should useEntityDisplayRepositoryInterface::getFormDisplay()
.Please take a look at the corresponding change record - https://www.drupal.org/node/2835616
Comment #95
acbramley CreditAttribution: acbramley at PreviousNext commentedFixes #93 and #94
Comment #96
acbramley CreditAttribution: acbramley at PreviousNext commentedMessed up the interdiff.
Comment #97
hchonovThank you. However in #91 @larowlan wanted to have the ternary operator outside the t call:
Could we please do this last change? I am sorry if not mentioning this explicitly in my previous comment caused the confusion.
Comment #98
larowlan@hchonov @acbramley and I checked, there is already a translation string for 'Unlimited' so I think this is approach is the best outcome, as we only have one new string for translators.
Although, the existing one was Unlimited (capital U), so not sure if this picks that up.
Comment #99
hchonovI totally agree with this :).
I am surprised that we don't have a translation string for the lower case word yet, but then it is the time to introduce it :).
P.S. we only need to move out the ternary and save the result to a dedicated variable. Then I will consider the issue RTBC.
Comment #100
acbramley CreditAttribution: acbramley at PreviousNext commentedMade changes from #99.
Comment #101
pandaski CreditAttribution: pandaski at govCMS (Australian Government Department of Finance) commentedNot sure whether we need a hard limit such as '#maxlength' => 1024 instead of 'unlimited'
Comment #102
chr.fritschNice patch.
Just one nitpick:
We should use $this->t() here
Comment #103
phjouI just rerolled the patch because of that issue Rename "File" media type to "Document". It wasn't applying anymore.
@chr.fritsch There are lots of them not using $this->t() even some not added by this patch. Should we change them all?
Comment #104
hchonovIt is not required to update existing ones, as those changes wouldn't fall into the scope of the current issue and would only make the patch bigger.
New string translations inside a class should be done through the class method
t()
by calling$this->t()
.Comment #105
acbramley CreditAttribution: acbramley at PreviousNext commentedFixes #102
Comment #106
hchonovComment #107
hchonovThank you. This looks good now.
Comment #108
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThanks everyone for working on this, I think it's a very useful addition!
There is a separate issue for fixing this string -> integer problem: #2885441: EntityReferenceAutocompleteWidget should define its size setting as an integer, so we shouldn't do it here.
Super nit: let's put
match_limit
undermatch_operator
to match the order with all the other places.Marking as NW for the first point.
Comment #109
acbramley CreditAttribution: acbramley at PreviousNext commentedFixes #108
Comment #111
acbramley CreditAttribution: acbramley at PreviousNext commentedSo switching that back is causing update tests to fail due to the default config no longer adhering to the schema. This is now blocked on #2885441: EntityReferenceAutocompleteWidget should define its size setting as an integer
Comment #112
jibranComment #113
acbramley CreditAttribution: acbramley at PreviousNext commentedRerolled against 8.8.x
Comment #114
chr.fritschAll done. Nice 👍
Comment #115
catchThe logic here should happen in a hook_ENTITY_TYPE_presave() (as we've done for multiple views changes), this way exported and default configuration will also be changed when it's saved.
Then the update can just resave everything.
Also tagging for 'needs screenshots' and 'needs UX review' since there are new configuration options in the UI here. I'm wondering a bit why this is a field-level setting and note a global setting site-wide - what is the use-case for increasing or reducing the number?
Comment #116
hchonovCould you please point to some example?
I wasn't aware, that we support old configuration? Why not execute the update, re-export the configuration and then import it? This way the exported and the imported configuration will always be up-to-date.
It is a widget setting, not a field setting. The match_operator is defined on the same level. Therefore it is much more natural, that both settings are defined at the same place. Please note that also both the match_operator and the match_limit are passed to the same method:
\Drupal\Core\Entity\EntityReferenceSelection\SelectionInterface::getReferenceableEntities($match = NULL, $match_operator = 'CONTAINS', $limit = 0)
.Widget level setting makes the configuration of the match limit much more flexible than having only one global setting. It makes it possible to design different forms with differently set match limit. While this is not directly an use case as you requested, I still think that the forms themselves should be responsible for controlling how many results should be shown to the user. Having the setting somewhere else would also be more confusing.
Comment #117
acbramley CreditAttribution: acbramley at PreviousNext commented100% agree with this, I think a global setting would be almost as limiting as a hardcoded limit of 10.
Comment #118
larowlanreroll against 8.7.8 for those who need it
Comment #119
larowlanor perhaps this one
Comment #120
acbramley CreditAttribution: acbramley at PreviousNext commentedScreenshots:
Before:
After:
Looking at porting the update hook to a presave now.
Comment #121
catch@hchonov
Views was the example but here's a direct link:
https://api.drupal.org/api/drupal/core%21modules%21views%21views.module/...
It works for configuration syncs (as long as site admins do it correctly), but there's also default configuration provided with modules, which no hook_update_N() can touch.
Comment #122
init90Added changes according to @catch suggestion. I hope it's correct implementation.
I have a few questions about it.
1. I correctly understood that, in fact, the code only needed in case when configuration management is improper and the main purpose of it is a guaranty that 'match_limit' always available in configs property list?
2. Should we add todo about remove the code in the future?
Also, I've found - Define and document best practices for configuration entity updates/bc layers
In general, should we rename the 'Autocomplete suggestion list size' to 'Autocomplete suggestion list limit'?
Comment #123
init90Comment #125
init90Fixed tests errors.
Comment #126
init90Comment #127
init90Added the 'match_size' parameter to the next displays:
Comment #128
acbramley CreditAttribution: acbramley at PreviousNext commentedWe can now remove setting the match_limit in the post update hook since it's being done in presave.
Comment #129
hchonovI've just added the screenshots from #120 to the issue summary.
From #115 :
This is now done.
The screenshots have been provided. I am not really sure if we need an UX review for configuration options, as this is not UX but DX IMHO, but still leaving the tag.
We've commented on this and there we no objections yet.
I hope that it is okay to set it to RTBC again while waiting for the UX review?
Comment #130
catchCrosspost, leaving RTBC since I'm not 100% sure on the deprecation part of this:
@init90:
It's primarily needed to support contrib modules (or install profiles etc.) that provide default configuration.
This should probably trigger a deprecation error to prompt contrib modules to update. The update test will need to account for the deprecation error being triggered in order to still pass. And yes a @todo to remove the hook implementation in 9.x
The presave/update changes look exactly right to me, thanks! Also fine with it being RTBC for the UX review. I agree the widget settings seems like the right place, just concerned about UI clutter adding configuration for something that's barely perceptible.
Comment #131
hchonovBut the example you've referenced to in #121 do not have all that?
And if we do this, then the deprecation error will be triggered during the update as well.
Comment #132
alexpottIt's fine for deprecations being triggered during the update. All update tests expect legacy code to be triggered.
I agree with @catch in #130
The latest patch seems to be missing an explicit update test.
There's no test of this being set in the UI.
Comment #133
catchIt was added during the 8.5.x cycle, we've got better at deprecations since then, these kinds of bc layers are the least-properly-implemented deprecations in core and the worst documented.
I've opened #3086261: Add proper deprecation notices in config entity presave bc layers for views_view_presave() et al.
Comment #134
acbramley CreditAttribution: acbramley at PreviousNext commentedHere's the deprecation notice, @todo, and Update test addition. Uploading before working on the admin UI test as I know the deprecation notice will probably need text changes. Also getting some weirdness locally with the @expectedDeprecation showing a huge number of notices rather than just mine so want to see if we get the same in CI.
Comment #135
acbramley CreditAttribution: acbramley at PreviousNext commentedForgot to fix the issue number after copy pasting
Comment #136
andypostI bet it needs manual testing to make sure that hook is useless because BC is provided by default settings
I bet this hook implementation could be removed because default 10 is provided by default settings
this todo needs a link to fix according standards https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...
Comment #137
acbramley CreditAttribution: acbramley at PreviousNext commentedCreated #3086388: Deprecate system_entity_form_display_presave(), will add the link along with the UI tests.
Comment #138
acbramley CreditAttribution: acbramley at PreviousNext commentedI think this is explained back in #68 but it doesn't seem like defaultSettings are merged in on save. I've manually tested this by removing the presave hook and running db updates, clearing cache, and inspecting config and indeed they aren't populated.
I also tested removing the match_limit setting from
core.entity_form_display.node.article.default.yml
in the standard profile and running a site install and again the setting isn't populated via defaultSettings.Comment #139
acbramley CreditAttribution: acbramley at PreviousNext commentedComment #140
andypost- used to apply patch to fresh install & use interdiff to remove hook (did not run update db)
- visited
/admin/structure/types/manage/article/form-display
- default value is provided but config export shows it is not stored
I think this RTBC because we have
- test for default value (extra is
EntityFormDisplayResourceTestBase.php
)- upgrade test
Comment #141
andypostbit more clean-up
Comment #142
andypostand one more
Comment #145
acbramley CreditAttribution: acbramley at PreviousNext commentedHere's an update to #135 including updating the settings via the UI.
Re 140-142, the presave hook was explicitly asked for, and without it we'd only be testing that default config is applied in the UpdateTest so it'd basically be redundant at that point. With that in mind, I'm happy to remove that test if we come to a consensus that the hook is not needed.
Comment #146
andypost@acbramley btw I realized that if I save display before running updates then post update overrides it to default(
Looks for proper updates it should check if the property already configured somehow...
Comment #147
acbramley CreditAttribution: acbramley at PreviousNext commentedMessed that last patch up....interdiff still applies.
It was doing that in the presave ;)
Comment #149
andypostNice test improvement, suppose it ready
Comment #150
larowlanThis is looking really good.
Should we provide some more context here? This message will be shown for every display but the user is none-the-wiser about which display it impacts? Should we add the ID for the display to the message?
we could just return TRUE here right? at least one of the fields is an ER field, no need to check the rest.
Then the final return $needs_save would be just
return FALSE
and the local variable $needs_save goes away.can we assert that the value isn't set before the updates are run, to prevent from future updates to the DB dumps from giving us a false positive?
Comment #151
acbramley CreditAttribution: acbramley at PreviousNext commentedFixes feedback in #150 thanks for the review @larowlan!
Comment #153
acbramley CreditAttribution: acbramley at PreviousNext commentedFailure looks unrelated.
Comment #154
andypostback to rtbc
Comment #155
larowlanComment #157
larowlanCommitted 438f383 and pushed to 8.8.x. Thanks!
Published changed record
Comment #159
xjmThis patch broke postgres.
Comment #160
xjmQueued a test against Postgres to expose the fails.
Comment #161
Krzysztof Domański#159 looks like a common random test failure. Previously, there was a similar failure test. Similar to #3055648: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest.
Comment #162
Krzysztof DomańskiI confirm there are several test failures for Postgres related to the new patch...
https://www.drupal.org/pift-ci-job/1429975
https://www.drupal.org/pift-ci-job/1429970
https://www.drupal.org/pift-ci-job/1429944
https://www.drupal.org/pift-ci-job/1429866
Comment #163
Krzysztof Domański#160 as we can see, three tests failed: https://www.drupal.org/pift-ci-job/1430011.
Two similar to #2982755: Random failure in SchemaTest::testSchemaChangePrimaryKey with order of composite primary key.
and one that breaks Postgres tests #162.
Comment #164
Krzysztof DomańskiVerifying that
EntityReferenceAutocompleteWidgetTest
failed each time in PostgreSQL.Result: 10/10 failed https://www.drupal.org/pift-ci-job/1430212.
Comment #165
hchonovI've tried finding out what is the problem with
EntityReferenceAutocompleteWidgetTest
.In this test we create two entries named
Test page
andPage test
. After that we enterTest
in the autocomplete field and assert that onlyTest page
is returned.Results:
MariaDB:
Test page
PostgreSQL 11.4:
Test page
PostgreSQL 9.4:
Page test
See the screenshots for the PostgreSQL results:
PostgreSQL 11.4:
PostgreSQL 9.4:
One might think that it is because of sensitivity, but it is not. In mysql LIKE is case insensitive, but in PostgreSQL it is not which is why in our DB Driver for PostgreSQL we map LIKE to ILIKE which is the case insensitive operator in PostgreSQL.
As it looks like the order might break and in this case both
Test page
andPage test
are correct results I think that we shouldn't assert that specificallyTest page
is returned, but instead ensure that only one of them is returned. I've updated the patch accordingly.Comment #166
larowlan@hchonov does this random-ness go away if we pass ['sort' => 'title'] as part of the call to
createEntityReferenceField
in the test setup?if so, I think that's all we need to change to remove the randomness
Comment #167
acbramley CreditAttribution: acbramley at PreviousNext commentedThis patch implements #166 by sorting the results by title to ensure consistent result lists.
Comment #168
kim.pepperFixes postgres issue so back to RTBC
Comment #169
larowlanCommitted 1108909 and pushed to 9.0.x. Thanks!
c/p to 8.8 and 8.9
Comment #172
xjmBelated apologies for the earlier confusion; I accidentally pasted a link to the wrong postgres QA result when I reverted the patch earlier. But y'all figured it out pretty quickly anyway. :)
Comment #173
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedDue to the
@trigger_error
deprecation message which is created in contrib tests, I addedmatch_limit: 10
to my contrib module test content type intests/modules/scheduler_api_test/config/install/core.entity_form_display.node.scheduler_api_test.default.yml
. Running the tests at core 8.8 is now fine, the deprecation messages are gone, but the tests now fail at core 8.7 and 8.6 withIs there anything that contrib maintainers can do to get all passing tests at 8.8, 8.7 and 8.6? I know I could mark the tests with @group legacy and delay the code fix until we no longer need to support testing at 8.6 and 8.7. But is there a better way?
Comment #174
BerdirNot really with the current core code. I'd recommend in general to ignore 8.8 deprecation messages and stick to 8.7 or 8.6 until your module depends on that core version. It's just a deprecation message saying it will be required in Drupal 9 (although even then, plugins have default configuration and I'd expect it to be merged in. But didn't review the patch and selection plugins might be special). You have plenty of time to fix that, especially trivial stuff like this.
Comment #175
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThanks @Berdir. Actually, I have now realised that the uid field in the test entity type does not need to be an entity_reference_autocomplete, it can be a simple number. So now we have passing tests at 8.6, 8.7 and 8.8+
Comment #177
BerdirFWIW, while #174 is IMHO correct in general, I'm confused about the deprecation message as well.
Widgets/Formatters support default settings out of the box. There is no reason to force-update form displays, the widget falls back to the default in \Drupal\Core\Field\PluginSettingsBase::getSetting() automatically. Widgets/Formatters can just add new new settings, it only gets more complicated if they're nested arrays, we don't merge recursively.
The one place where that doesn't apply is EntityAutocompleteMatcher but the BC layer can't handle that anyway, as our own widget will always pass a value while other widgets and hardcoded usages of entity_autocomplete aren't going to do that and we also don't trigger a deprecation message there (nor should we imho, nothing wrong with having a default).
Another thing the ·@trigger_error() doesn't cover is base fields, but again, I don't see why we would need to force updates of any of that.
Comment #178
jimafisk CreditAttribution: jimafisk at Jantcu commentedOn drupal 8.7.10, #119 worked for me. Thanks!
Comment #179
oknateI believe I found a work around where you can have passing tests on < 8.8 and add the match limit to your test configs for 8.8.
Add the following to your tests:
And then when you no longer test against versions less than 8.8, you can remove it.
That being said, I agree with Berdir that there shouldn't be a trigger_error there in 8.8. Should we create a follow-up?
Comment #180
markconroy CreditAttribution: markconroy at Annertech commentedHi Folks,
This looks like a great improvement to the UI. Thanks for all the work you have done on it.
One issue that has cropped up for me is how to set the amount to show when creating menu items? It still seems to default to 10, and we don't have a UI to set the amount.
On this simplytest.me page - https://stm5df4cd7fd0302-qsmdn47i0hfklj0cn0idtflyeefzfjhh.tugboat.qa - I have created 11 nodes:
But when I go to the menu UI to add a link, I can (still) only see 10:
Comment #181
oknateI don't think menu links use EntityReferenceAutocompleteWidget. So perhaps add a separate follow-up issue for menu links.
Comment #182
andypostComment #183
lucasantunes CreditAttribution: lucasantunes commentedI second that, @markconroy. This would be extremely useful for automated test tools.
As an example, I have a site with way over 10 pages for a match I need my tool to make, that may be partially a word.
I was unfortunate enough for the site to not show the exact match I was searching for.
Maybe show closest matches to what I'm searching for first would be a great improvement! That would allow for an exact match to be shown first.
Comment #184
liquidcms CreditAttribution: liquidcms commenteddeleted