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.

CommentFileSizeAuthor
#180 Screenshot 2019-12-14 at 12.04.04.png38.91 KBmarkconroy
#180 Screenshot 2019-12-14 at 12.03.52.png48.1 KBmarkconroy
#167 interdiff-2863188-151-167.txt1.14 KBacbramley
#167 2863188-167.patch31.59 KBacbramley
#165 2863188-165.patch31.09 KBhchonov
#165 interdiff-151-165.txt1.05 KBhchonov
#165 2863188-151_on_pgsql_9.4.png727.65 KBhchonov
#165 2863188-151_on_pgsql_11.4.png679.04 KBhchonov
#164 2863188-test-only-repeat-10.patch33.1 KBKrzysztof Domański
#151 interdiff-2863188-147-151.txt3.91 KBacbramley
#151 2863188-151.patch30.9 KBacbramley
#147 2863188-146.patch30.25 KBacbramley
#145 interdiff-2863188-135-145.txt3.54 KBacbramley
#145 2863188-145-with-presave.patch89.17 KBacbramley
#142 2863188-140.patch26.38 KBandypost
#142 interdiff.txt648 bytesandypost
#141 interdiff.txt621 bytesandypost
#141 2863188-139.patch26.39 KBandypost
#140 2863188-138.patch27 KBandypost
#140 interdiff.txt1.45 KBandypost
#140 before_update_after_patch.png39.6 KBandypost
#135 interdiff-2863188-134-135.txt2.03 KBacbramley
#135 2863188-135.patch28.13 KBacbramley
#134 2863188-134.patch28.13 KBacbramley
#134 interdiff-2863188-128-134.txt2.8 KBacbramley
#128 interdiff-2863188-127-128.txt678 bytesacbramley
#128 2863188-128.patch26.16 KBacbramley
#127 2863188-127.patch26.39 KBinit90
#127 interdiff_125-127.txt5.13 KBinit90
#125 interdiff_122-125.txt784 bytesinit90
#125 2863188-125.patch18.35 KBinit90
#122 interdiff_113-122.txt1.5 KBinit90
#122 2863188-122.patch17.22 KBinit90
#120 Screen Shot 2019-10-04 at 2.41.31 pm.png130.66 KBacbramley
#120 Screen Shot 2019-10-04 at 2.38.14 pm.png115.18 KBacbramley
#119 2863188-8.7.8.do-not-test.patch15.76 KBlarowlan
#118 2863188-8.7.8.do-not-test.patch16.08 KBlarowlan
#113 2863188-113.patch15.63 KBacbramley
#109 2863188-109.patch15.65 KBacbramley
#109 interdiff-2863188-105-109.txt1.37 KBacbramley
#105 2863188-105.patch15.71 KBacbramley
#105 interdiff-2863188-103-105.txt881 bytesacbramley
#103 2863188-103.patch15.71 KBphjou
#100 2863188-100.patch15.69 KBacbramley
#100 interdiff-2863188-95-100.txt1.12 KBacbramley
#96 interdiff-2863188-92-95-2.txt2.04 KBacbramley
#95 interdiff-2863188-92-95.txt2.04 KBacbramley
#95 2863188-95.patch15.67 KBacbramley
#92 2863188-92.patch15.75 KBacbramley
#92 interdiff-2863188-87-92.txt1.67 KBacbramley
#87 2863188-87.patch15.65 KBacbramley
#87 interdiff-2863188-82-87.txt673 bytesacbramley
#82 2863188-82.patch14.99 KBacbramley
#82 interdiff-2863188-75-82.txt650 bytesacbramley
#75 2863188-74.patch14.99 KBacbramley
#74 interdiff-2863188-66-74.txt1.12 KBacbramley
#72 2863188-72.patch14.91 KBacbramley
#71 interdiff-2863188-66-71.txt3.39 KBacbramley
#71 2863188-71.patch14.76 KBacbramley
#66 2863188-66.patch15.03 KBtstoeckler
#65 2863188.png79.89 KBErik Frèrejean
#61 Screenshot from 2018-10-07 23-19-00.png42.48 KBjibran
#58 limit_autocomplete_suggestions-2863188-58.patch15.1 KBErik Frèrejean
#56 interdiff-2863188-52-56.txt14.42 KBErik Frèrejean
#56 limit_autocomplete_suggestions-2863188-56.patch15.1 KBErik Frèrejean
#52 interdiff-2863188-46-52.txt4.74 KBchr.fritsch
#52 2863188-52.patch15.14 KBchr.fritsch
#47 interdiff-42-46.txt3.57 KBErik Frèrejean
#3 hardcoded_result_size-2863188-3.patch5.06 KBErik Frèrejean
#7 hardcoded_result_size-2863188-7.patch5.59 KBErik Frèrejean
#11 hardcoded_result_size-2863188-11.patch7 KBErik Frèrejean
#12 hardcoded_result_size-2863188-12.patch7 KBErik Frèrejean
#14 rerolled_patch_hardcoded_result_size-2863188.patch7.07 KBsathish.redcrackle
#14 Screen Shot for setting size limit pm.png137.18 KBsathish.redcrackle
#14 Screen Shot with values pm.png48.83 KBsathish.redcrackle
#15 hardcoded_result_size-2863188-15.patch7.07 KBErik Frèrejean
#24 hardcoded_result_size-2863188-24.patch8.86 KBMaouna
#24 15-24-interdiff.txt1.58 KBMaouna
#26 hardcoded_result_size-2863188-26.patch8.93 KBMaouna
#26 24-26-interdiff.txt1.58 KBMaouna
#27 8.4.x_hardcoded_result_size-2863188-26.patch8.93 KBMaouna
#29 hardcoded_result_size-2863188-29.patch9.69 KBMaouna
#29 26-29-interdiff.txt780 bytesMaouna
#43 hardcoded_result_size-2863188-43.patch10.07 KBvasi1186
#43 29-43-interdiff.txt4.07 KBvasi1186
#46 interdiff-42-46.txt3.57 KBErik Frèrejean
#46 hardcoded_result_size-2863188-46.patch9.72 KBErik Frèrejean
#46 hardcoded_result_size-2863188-43-reroll.patch9.94 KBErik Frèrejean
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Erik Frèrejean created an issue. See original summary.

selvira’s picture

Assigned: Unassigned » selvira

I'm going to work on this!.
Greetings.

Erik Frèrejean’s picture

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

Erik Frèrejean’s picture

Status: Active » Needs review
selvira’s picture

Assigned: selvira » Unassigned

Ok, don't worry, i'm going to unassign the issue...
regards.

Status: Needs review » Needs work

The last submitted patch, 3: hardcoded_result_size-2863188-3.patch, failed testing.

Erik Frèrejean’s picture

Status: Needs work » Needs review
FileSize
5.59 KB

Seems I forgot to add the schema change to the patch.

Status: Needs review » Needs work

The last submitted patch, 7: hardcoded_result_size-2863188-7.patch, failed testing.

_Archy_’s picture

Issue tags: +DCTransylvania

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

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

Erik Frèrejean’s picture

Status: Needs work » Needs review
FileSize
7 KB

I noticed that I missed a change in the schema.

Erik Frèrejean’s picture

Fix coding style issue.

cilefen’s picture

sathish.redcrackle’s picture

Erik Frèrejean’s picture

I just realised that the description with the settings field says

Use 0 to remove the limit.

, however the widget itself would reset to the default 10, in case of "empty". Updated patch attached.

sathish.redcrackle’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: hardcoded_result_size-2863188-15.patch, failed testing. View results

harsha012’s picture

Status: Needs work » Needs review

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

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

borisson_’s picture

hchonov’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityAutocompleteMatcher.php
@@ -61,7 +61,8 @@ public function getMatches($target_type, $selection_handler, $selection_settings
+      $match_size = isset($selection_settings['match_size']) ? (int) $selection_settings['match_size'] : 10;

and when the update path is ready this fallback will not be needed anymore.

hchonov’s picture

Status: Needs review » Needs work

Needs 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 is field_update_8003(), where we check with instanceof.

Maouna’s picture

Assigned: Unassigned » Maouna
Status: Needs work » Active
Maouna’s picture

Status: Active » Needs review
FileSize
8.86 KB
1.58 KB

As suggested in #20 and #22, I added the update.

I disagree with #21, as getMatches is public, the fallback is still needed.

Status: Needs review » Needs work

The last submitted patch, 24: hardcoded_result_size-2863188-24.patch, failed testing. View results

Maouna’s picture

Status: Needs work » Needs review
FileSize
8.93 KB
1.58 KB
Maouna’s picture

Version: 8.5.x-dev » 8.6.x-dev
FileSize
8.93 KB

The last submitted patch, 26: hardcoded_result_size-2863188-26.patch, failed testing. View results

Maouna’s picture

Status: Needs review » Needs work

The last submitted patch, 29: hardcoded_result_size-2863188-29.patch, failed testing. View results

Maouna’s picture

Assigned: Maouna » Unassigned
Status: Needs work » Needs review
kfritsche’s picture

Reviewed and tested and locks pretty good.

Just a small code style issue

+++ b/core/modules/entity_reference/entity_reference.install
@@ -0,0 +1,42 @@
+          if ($plugin_definition['class'] == EntityReferenceAutocompleteWidget::class || is_subclass_of($plugin_definition['class'], EntityReferenceAutocompleteWidget::class)) {

better and shorter would be

$class = 'Drupal\Core\Field\Plugin\Field\FieldWidget\EntityReferenceAutocompleteWidget';
if ($plugin_definition['class'] === $class || is_subclass_of($plugin_definition['class'], $class)) {

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.

hchonov’s picture

Status: Needs review » Needs work
+++ b/core/modules/entity_reference/entity_reference.install
@@ -0,0 +1,42 @@
+function entity_reference_update_8001() {
...
+          if ($plugin_definition['class'] == EntityReferenceAutocompleteWidget::class || is_subclass_of($plugin_definition['class'], EntityReferenceAutocompleteWidget::class)) {
...
+      $display_config->save(TRUE);

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

amateescu’s picture

As the EntityReferenceAutocompleteWidget is defined in the core system, the update should be placed in the system module.

Actually, updates for field types, widgets and formatters provided by core should be placed in the Field module :)

hchonov’s picture

Actually, updates for field types, widgets and formatters provided by core should be placed in the Field module :)

Excuse 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?

amateescu’s picture

Sure, 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.

tstoeckler’s picture

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

hchonov’s picture

Well in this case I am fine with this decision. Thank you for the clarification, @amateescu!

amateescu’s picture

@tstoeckler, this is how we've handled field type / widget / formatter related updates since Drupal 8.0.0, see field.install and field.post_update.php, and there's no bug report about it.

tstoeckler’s picture

Well, 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.

amateescu’s picture

Ok, system module it is then :)

vasi1186’s picture

Assigned: Unassigned » vasi1186
vasi1186’s picture

Status: Needs work » Needs review
FileSize
10.07 KB
4.07 KB

I updated the patch so that the update hook is actually a post update of the system module.

vasi1186’s picture

Assigned: vasi1186 » Unassigned
tstoeckler’s picture

Status: Needs review » Needs work

Thanks for pushing this forward! I think this is close, but marking needs work for the second point below.

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php
    @@ -68,6 +76,10 @@ class EntityReferenceAutocompleteWidget extends WidgetBase {
    +    if ($match_size != 0) {
    

    Super-nitpick: This can just be if ($match_size)

  2. +++ b/core/modules/system/system.post_update.php
    @@ -170,3 +171,37 @@ function system_post_update_extra_fields(&$sandbox = NULL) {
    +  foreach ($config->listAll('core.entity_form_display.') as $display_id) {
    

    I think this should now use the batch update mechanism that was recently introduced. See https://www.drupal.org/node/2949630

Erik Frèrejean’s picture

Rerolled #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.

Erik Frèrejean’s picture

FileSize
3.57 KB

Noticed that I had the interdiff the wrong way around...

The last submitted patch, 46: hardcoded_result_size-2863188-46.patch, failed testing. View results

The last submitted patch, 46: hardcoded_result_size-2863188-46.patch, failed testing. View results

tstoeckler’s picture

Status: Needs review » Needs work

Patch looks great now, but unfortunately some tests fail now.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
15.14 KB
4.74 KB

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

Erik Frèrejean’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for fixing the tests.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/config/schema/core.entity.schema.yml
    @@ -234,6 +234,9 @@ field.widget.settings.entity_reference_autocomplete_tags:
    +    match_size:
    +      type: integer
    +      label: 'Number of autocomplete suggestions.'
         size:
           type: integer
           label: 'Size of textfield'
    
    +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php
    @@ -44,6 +45,13 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
    +    $element['match_size'] = [
    +      '#type' => 'number',
    +      '#title' => t('Number of results'),
    +      '#default_value' => $this->getSetting('match_size'),
    +      '#min' => 0,
    +      '#description' => t('The number of suggestions that will be listed. Use <em>0</em> to remove the limit.'),
    +    ];
    

    match_size is a confusing configuration key. Something like match_limit would be better. Perhaps someone has a better suggestion. But without changing this I would be confused about size and match_size and wonder how they are related.

  2. +++ b/core/modules/system/system.post_update.php
    @@ -169,3 +170,32 @@ function system_post_update_extra_fields(&$sandbox = NULL) {
    +      if ($plugin_definition['class'] === EntityReferenceAutocompleteWidget::class || is_subclass_of($plugin_definition['class'], EntityReferenceAutocompleteWidget::class)) {
    

    I think we can use is_a($plugin_definition['class'], EntityReferenceAutocompleteWidget::class, TRUE)
    to simplify the if.

Erik Frèrejean’s picture

Assigned: Unassigned » Erik Frèrejean
Erik Frèrejean’s picture

Assigned: Erik Frèrejean » Unassigned
Status: Needs work » Needs review
Issue tags: -DCTransylvania
FileSize
15.1 KB
14.42 KB
  • Renamed all occurrences of match_size to match_limit, which seem a proper name to me.
  • Update the check in the post update hook.
  • Tweaked the label in the config schema.
hchonov’s picture

+++ b/core/config/schema/core.entity.schema.yml
@@ -234,9 +234,9 @@ field.widget.settings.entity_reference_autocomplete_tags:
+      label: 'Number maximum of autocomplete suggestions.'

=> Maximum number of ... / Number of maximum ...

Erik Frèrejean’s picture

hchonov’s picture

Status: Needs review » Reviewed & tested by the community

Now that the review from @alexpott has been addressed, I think that the patch is ready.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/system.post_update.php
@@ -169,3 +170,32 @@ function system_post_update_extra_fields(&$sandbox = NULL) {
+  $callback = function (EntityDisplayInterface $display) use ($field_widget_manager) {
+    $needs_save = FALSE;
+    foreach ($display->getComponents() as $field_name => $component) {
+      if (empty($component['type'])) {
+        continue;
+      }
+
+      $plugin_definition = $field_widget_manager->getDefinition($component['type'], FALSE);
+      if (is_a($plugin_definition['class'], EntityReferenceAutocompleteWidget::class, TRUE)) {
+        $component['settings']['match_limit'] = 10;
+        $display->setComponent($field_name, $component);
+        $needs_save = TRUE;
+      }
+    }
+
+    return $needs_save;
+  };

I think this can be written as

  $callback = function (EntityDisplayInterface $display) use ($field_widget_manager) {
    foreach ($display->getComponents() as $field_name => $component) {
      if (empty($component['type'])) {
        continue;
      }

      $plugin_definition = $field_widget_manager->getDefinition($component['type'], FALSE);
      if (is_a($plugin_definition['class'], EntityReferenceAutocompleteWidget::class, TRUE)) {
        return TRUE;
      }
    }

    return FALSE;
  };

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.

jibran’s picture

IS needs an update there is a UI change, API addition maybe and config data model change. We also need a change notice.

if (is_a($plugin_definition['class'], EntityReferenceAutocompleteWidget::class, TRUE)) {
        return TRUE;
      }

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.

alexpott’s picture

@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. With is_a() you can.

>>> '\Drupal\user\UserInterface' instanceof '\Drupal\Core\Entity\ContentEntityInterface'
PHP Parse error: Syntax error, unexpected T_CONSTANT_ENCAPSED_STRING on line 1
>>> is_a('\Drupal\user\UserInterface', '\Drupal\Core\Entity\ContentEntityInterface', TRUE)
=> true
alexpott’s picture

@jibran to quote the docs

is_a — Checks if the object is of this class or has this class as one of its parents

hchonov’s picture

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.

@alexpott, could you please elaborate on this?

Erik Frèrejean’s picture

FileSize
79.89 KB

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

tstoeckler’s picture

FileSize
15.03 KB

Here'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 to View::fixTableNames() ?

larowlan’s picture

Status: Needs work » Needs review
+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php
@@ -68,6 +76,10 @@ public function settingsSummary() {
+    $match_limit = $this->getSetting('match_limit');
+    if ($match_limit) {

this 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)

Sam152’s picture

Re @alexpott: Are you suggesting that we add some runtime code in EntityFormDisplay::preSave() that adds the default value to widgets similar to View::fixTableNames() ?

Elsewhere, 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 call getConfiguration 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.

acbramley’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityAutocompleteMatcher.php
@@ -61,7 +61,8 @@ public function getMatches($target_type, $selection_handler, $selection_settings
+      $match_limit = isset($selection_settings['match_limit']) ? (int) $selection_settings['match_limit'] : 10;

We don't need this check anymore since the default value is being set.

this will always be set because of the default?

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.

acbramley’s picture

Assigned: Unassigned » acbramley
Status: Needs review » Needs work

Working on fixes for #69

acbramley’s picture

Assigned: acbramley » Unassigned
Status: Needs work » Needs review
FileSize
14.76 KB
3.39 KB

Fixes #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).

acbramley’s picture

FileSize
14.91 KB

Weird, 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

tstoeckler’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityAutocompleteMatcher.php
@@ -61,8 +61,7 @@ public function getMatches($target_type, $selection_handler, $selection_settings
-      $match_limit = isset($selection_settings['match_limit']) ? (int) $selection_settings['match_limit'] : 10;
-      $entity_labels = $handler->getReferenceableEntities($string, $match_operator, $match_limit);
+      $entity_labels = $handler->getReferenceableEntities($string, $match_operator, (int) $selection_settings['match_limit']);

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

acbramley’s picture

FileSize
1.12 KB

Ah, yes you're right. Reverted, now with a new interdiff.

acbramley’s picture

would help if I uploaded the patch.

The last submitted patch, 71: 2863188-71.patch, failed testing. View results

The last submitted patch, 72: 2863188-72.patch, failed testing. View results

Erik Frèrejean’s picture

Erik Frèrejean’s picture

Patch in #75 looks fine, but can't test it atm so I'll leave it to needs review for now.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

larowlan’s picture

+++ b/core/modules/system/system.post_update.php
@@ -190,3 +191,32 @@ function system_post_update_add_expand_all_items_key_in_system_menu_block(&$sand
+ * Populate the new 'match_limit' setting for entity reference autocomplete widget.

nit > 80

acbramley’s picture

FileSize
650 bytes
14.99 KB

Fixes #81

jibran’s picture

Let's update IS then we can RTBC it.

acbramley’s picture

Issue summary: View changes

Updated, please make any further changes required.

Status: Needs review » Needs work

The last submitted patch, 82: 2863188-82.patch, failed testing. View results

jibran’s picture

Let's fix the failing test.

acbramley’s picture

Status: Needs work » Needs review
FileSize
673 bytes
15.65 KB

Fixes the failure.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Great work thanks. I think we are done here.

sathish.redcrackle’s picture

larowlan’s picture

Category: Bug report » Feature request

This is a feature request, not a bug fix - unless someone can convince me otherwise

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php
@@ -68,6 +76,7 @@ public function settingsSummary() {
+    $summary[] = t('Autocomplete suggestion list size: @size', ['@size' => $this->getSetting('match_limit') ?: 'unlimited']);

Does 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())

acbramley’s picture

Status: Needs work » Needs review
FileSize
1.67 KB
15.75 KB

Fixes #91 in both instances where t() was being used.

hchonov’s picture

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php
@@ -76,7 +76,8 @@ public function settingsSummary() {
+    $summary[] = $match_limit ? $this->t('Autocomplete suggestion list size: @size', ['@size' => $match_limit]) : $this->t('Autocomplete suggestion list size: unlimited');

This however leads to two different translation strings. Could we probably pass $this->t("unlimited") as the @size argument instead?

hchonov’s picture

Status: Needs review » Needs work
+++ b/core/tests/Drupal/FunctionalJavascriptTests/EntityReference/EntityReferenceAutocompleteWidgetTest.php
@@ -97,6 +97,30 @@ public function testEntityReferenceAutocompleteWidget() {
+    entity_get_form_display('node', 'page', 'default')

entity_get_form_display() is deprecated and instead we should use EntityDisplayRepositoryInterface::getFormDisplay().

Please take a look at the corresponding change record - https://www.drupal.org/node/2835616

acbramley’s picture

Status: Needs work » Needs review
FileSize
15.67 KB
2.04 KB

Fixes #93 and #94

acbramley’s picture

FileSize
2.04 KB

Messed up the interdiff.

hchonov’s picture

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php
@@ -76,8 +76,7 @@ public function settingsSummary() {
+    $summary[] = $this->t('Autocomplete suggestion list size: @size', ['@size' => $this->getSetting('match_limit') ?: $this->t('unlimited')]);

Thank you. However in #91 @larowlan wanted to have the ternary operator outside the t call:

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

Could we please do this last change? I am sorry if not mentioning this explicitly in my previous comment caused the confusion.

larowlan’s picture

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

hchonov’s picture

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

I totally agree with this :).

Although, the existing one was Unlimited (capital U), so not sure if this picks that up.

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.

acbramley’s picture

FileSize
1.12 KB
15.69 KB

Made changes from #99.

pandaski’s picture

Not sure whether we need a hard limit such as '#maxlength' => 1024 instead of 'unlimited'

chr.fritsch’s picture

Status: Needs review » Needs work

Nice patch.

Just one nitpick:

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php
@@ -44,6 +45,13 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
+      '#title' => t('Number of results'),

We should use $this->t() here

phjou’s picture

I 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?

hchonov’s picture

It 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().

acbramley’s picture

Status: Needs work » Needs review
FileSize
881 bytes
15.71 KB

Fixes #102

hchonov’s picture

hchonov’s picture

Status: Needs review » Reviewed & tested by the community

Thank you. This looks good now.

amateescu’s picture

Status: Reviewed & tested by the community » Needs work

Thanks everyone for working on this, I think it's a very useful addition!

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php
    @@ -28,7 +28,8 @@ class EntityReferenceAutocompleteWidget extends WidgetBase {
    -      'size' => '60',
    ...
    +      'size' => 60,
    

    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.

  2. +++ b/core/modules/jsonapi/tests/src/Functional/EntityFormDisplayTest.php
    @@ -138,6 +138,7 @@ protected function getExpectedDocument() {
    +                'match_limit' => 10,
                     'match_operator' => 'CONTAINS',
    

    Super nit: let's put match_limit under match_operator to match the order with all the other places.

Marking as NW for the first point.

acbramley’s picture

Status: Needs work » Needs review
FileSize
1.37 KB
15.65 KB

Fixes #108

Status: Needs review » Needs work

The last submitted patch, 109: 2863188-109.patch, failed testing. View results

acbramley’s picture

Title: Hardcoded result size limit in the entity reference autocomplete widget. » [PP-1] Hardcoded result size limit in the entity reference autocomplete widget.
Related issues: +#2885441: EntityReferenceAutocompleteWidget should define its size setting as an integer

So 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

jibran’s picture

Title: [PP-1] Hardcoded result size limit in the entity reference autocomplete widget. » Hardcoded result size limit in the entity reference autocomplete widget.
acbramley’s picture

Status: Needs work » Needs review
FileSize
15.63 KB

Rerolled against 8.8.x

chr.fritsch’s picture

Status: Needs review » Reviewed & tested by the community

All done. Nice  👍

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs screenshots, +Needs UX review
+++ b/core/modules/system/system.post_update.php
@@ -213,3 +214,32 @@ function system_post_update_clear_menu_cache() {
+ */
+function system_post_update_entity_reference_autocomplete_match_limit(&$sandbox = NULL) {
+  $config_entity_updater = \Drupal::classResolver(ConfigEntityUpdater::class);
+  /** @var \Drupal\Core\Field\WidgetPluginManager $field_widget_manager */
+  $field_widget_manager = \Drupal::service('plugin.manager.field.widget');
+
+  $callback = function (EntityDisplayInterface $display) use ($field_widget_manager) {
+    $needs_save = FALSE;
+    foreach ($display->getComponents() as $field_name => $component) {
+      if (empty($component['type'])) {
+        continue;
+      }
+
+      $plugin_definition = $field_widget_manager->getDefinition($component['type'], FALSE);
+      if (is_a($plugin_definition['class'], EntityReferenceAutocompleteWidget::class, TRUE)) {
+        $component['settings']['match_limit'] = 10;
+        $display->setComponent($field_name, $component);
+        $needs_save = TRUE;
+      }
+    }
+
+    return $needs_save;
+  };

The 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?

hchonov’s picture

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

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

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?

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.

acbramley’s picture

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.

100% agree with this, I think a global setting would be almost as limiting as a hardcoded limit of 10.

larowlan’s picture

reroll against 8.7.8 for those who need it

larowlan’s picture

or perhaps this one

acbramley’s picture

Screenshots:

Before:
Before applying patch.

After:
After applying

Looking at porting the update hook to a presave now.

catch’s picture

@hchonov

Could you please point to some example?

Views was the example but here's a direct link:
https://api.drupal.org/api/drupal/core%21modules%21views%21views.module/...

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

init90’s picture

Added 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'?

init90’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 122: 2863188-122.patch, failed testing. View results

init90’s picture

Fixed tests errors.

init90’s picture

Status: Needs work » Needs review
init90’s picture

Added the 'match_size' parameter to the next displays:

core/profiles/standard/config/optional/core.entity_form_display.media.audio.default.yml
core/profiles/standard/config/optional/core.entity_form_display.media.document.default.yml
core/profiles/standard/config/optional/core.entity_form_display.media.image.default.yml
core/profiles/standard/config/optional/core.entity_form_display.media.remote_video.default.yml
core/profiles/standard/config/optional/core.entity_form_display.media.video.default.yml

core/modules/media_library/tests/modules/media_library_test/config/install/core.entity_form_display.media.type_five.default.yml
core/modules/media_library/tests/modules/media_library_test/config/install/core.entity_form_display.media.type_four.default.yml
core/modules/media_library/tests/modules/media_library_test/config/install/core.entity_form_display.media.type_one.default.yml
core/modules/media_library/tests/modules/media_library_test/config/install/core.entity_form_display.media.type_three.default.yml
core/modules/media_library/tests/modules/media_library_test/config/install/core.entity_form_display.media.type_two.default.yml
core/modules/media_library/tests/modules/media_library_test/config/install/core.entity_form_display.node.basic_page.default.yml
core/modules/options/tests/options_config_install_test/config/install/core.entity_form_display.node.options_install_test.default.yml
acbramley’s picture

We can now remove setting the match_limit in the post update hook since it's being done in presave.

hchonov’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

I've just added the screenshots from #120 to the issue summary.

From #115 :

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

This is now done.

Also tagging for 'needs screenshots' and 'needs UX review' since there are new configuration options in the UI here.

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.

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?

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?

catch’s picture

Crosspost, leaving RTBC since I'm not 100% sure on the deprecation part of this:

@init90:

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?

It's primarily needed to support contrib modules (or install profiles etc.) that provide default configuration.

+++ b/core/modules/system/system.module
@@ -1459,3 +1461,29 @@ function system_modules_uninstalled($modules) {
+
+    if (!isset($component['settings']['match_limit'])) {
+      $component['settings']['match_limit'] = 10;
+      $display->setComponent($field_name, $component);
+    }

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.

hchonov’s picture

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

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

And if we do this, then the deprecation error will be triggered during the update as well.

It's fine for deprecations being triggered during the update. All update tests expect legacy code to be triggered.

I agree with @catch in #130

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 latest patch seems to be missing an explicit update test.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php
@@ -44,6 +45,13 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
+    $element['match_limit'] = [
+      '#type' => 'number',
+      '#title' => $this->t('Number of results'),
+      '#default_value' => $this->getSetting('match_limit'),
+      '#min' => 0,
+      '#description' => $this->t('The number of suggestions that will be listed. Use <em>0</em> to remove the limit.'),
+    ];

There's no test of this being set in the UI.

catch’s picture

But the example you've referenced to in #121 do not have all that?

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

acbramley’s picture

Status: Needs work » Needs review
FileSize
2.8 KB
28.13 KB

Here'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.

acbramley’s picture

Forgot to fix the issue number after copy pasting

andypost’s picture

Status: Needs review » Needs work
Issue tags: +Needs manual testing

I bet it needs manual testing to make sure that hook is useless because BC is provided by default settings

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php
    @@ -28,6 +28,7 @@ class EntityReferenceAutocompleteWidget extends WidgetBase {
       public static function defaultSettings() {
    ...
    +      'match_limit' => 10,
    
    +++ b/core/modules/system/system.module
    @@ -1466,3 +1468,32 @@ function system_modules_uninstalled($modules) {
    +function system_entity_form_display_presave(EntityFormDisplayInterface $display) {
    
    +++ b/core/tests/Drupal/FunctionalTests/Rest/EntityFormDisplayResourceTestBase.php
    @@ -109,6 +109,7 @@ protected function getExpectedNormalizedEntity() {
    +            'match_limit' => 10,
    

    I bet this hook implementation could be removed because default 10 is provided by default settings

  2. +++ b/core/modules/system/system.module
    @@ -1466,3 +1468,32 @@ function system_modules_uninstalled($modules) {
    + * @todo Remove this hook in Drupal 9.0.x.
    

    this todo needs a link to fix according standards https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...

acbramley’s picture

Created #3086388: Deprecate system_entity_form_display_presave(), will add the link along with the UI tests.

acbramley’s picture

I bet it needs manual testing to make sure that hook is useless because BC is provided by default settings

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

acbramley’s picture

Issue tags: -Needs manual testing
andypost’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
39.6 KB
1.45 KB
27 KB

- 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

before update

I think this RTBC because we have
- test for default value (extra is EntityFormDisplayResourceTestBase.php)
- upgrade test

andypost’s picture

FileSize
26.39 KB
621 bytes

bit more clean-up

andypost’s picture

FileSize
648 bytes
26.38 KB

and one more

The last submitted patch, 140: 2863188-138.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 141: 2863188-139.patch, failed testing. View results

acbramley’s picture

Here'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.

andypost’s picture

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

acbramley’s picture

Messed that last patch up....interdiff still applies.

Looks for proper updates it should check if the property already configured somehow...

It was doing that in the presave ;)

The last submitted patch, 142: 2863188-140.patch, failed testing. View results

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Nice test improvement, suppose it ready

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

This is looking really good.

  1. +++ b/core/modules/system/system.module
    @@ -1466,3 +1468,32 @@ function system_modules_uninstalled($modules) {
    +      @trigger_error('Any entity_reference_autocomplete component of an entity_form_display must have a match_limit setting. This BC layer will be removed before 9.0.0. See https://www.drupal.org/node/2863188', E_USER_DEPRECATED);
    

    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?

  2. +++ b/core/modules/system/system.post_update.php
    @@ -213,3 +214,30 @@ function system_post_update_clear_menu_cache() {
    +        $needs_save = TRUE;
    

    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.

  3. +++ b/core/modules/system/tests/src/Functional/Update/EntityReferenceAutocompleteWidgetMatchLimitUpdateTest.php
    @@ -0,0 +1,39 @@
    +    $this->runUpdates();
    +
    +    $display = EntityFormDisplay::load('node.article.default');
    +    $this->assertEquals(10, $display->getComponent('field_tags')['settings']['match_limit']);
    +    $this->assertEquals(10, $display->getComponent('uid')['settings']['match_limit']);
    

    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?

Status: Needs review » Needs work

The last submitted patch, 151: 2863188-151.patch, failed testing. View results

acbramley’s picture

Status: Needs work » Needs review

Failure looks unrelated.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc

larowlan’s picture

  • larowlan committed 438f383 on 8.8.x
    Issue #2863188 by acbramley, Erik Frèrejean, Maouna, andypost, init90,...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +8.8.0 highlights

Committed 438f383 and pushed to 8.8.x. Thanks!

Published changed record

  • xjm committed bd5908a on 8.8.x
    Revert "Issue #2863188 by acbramley, Erik Frèrejean, Maouna, andypost,...
xjm’s picture

Status: Fixed » Needs work

This patch broke postgres.

xjm’s picture

Queued a test against Postgres to expose the fails.

Krzysztof Domański’s picture

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

Testing Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest
.....E.                                                             7 / 7 (100%)

Time: 2.63 minutes, Memory: 4.00MB

There was 1 error:

1) Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest::testWidgetUpload
Behat\Mink\Exception\ResponseTextException: The text "image-1.png" appears in the text of this page, but it should not.
Krzysztof Domański’s picture

I 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

1) Drupal\FunctionalJavascriptTests\EntityReference\EntityReferenceAutocompleteWidgetTest::testEntityReferenceAutocompleteWidget
Behat\Mink\Exception\ResponseTextException: The text "Page test" appears in the text of this page, but it should not.

/var/www/html/vendor/behat/mink/src/WebAssert.php:785
/var/www/html/vendor/behat/mink/src/WebAssert.php:279
/var/www/html/core/tests/Drupal/FunctionalJavascriptTests/EntityReference/EntityReferenceAutocompleteWidgetTest.php:121
Krzysztof Domański’s picture

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

1) Drupal\KernelTests\Core\Database\SchemaTest::testSchemaChangePrimaryKey with data set "composite_primary_key" (array('test_field', 'other_test_field'), array('test_field_renamed', 'other_test_field'))
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 'test_field_renamed'
-    1 => 'other_test_field'
+    0 => 'other_test_field'
+    1 => 'test_field_renamed'

/var/www/html/core/tests/Drupal/KernelTests/KernelTestBase.php:1108
/var/www/html/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php:858

and one that breaks Postgres tests #162.

1) Drupal\FunctionalJavascriptTests\EntityReference\EntityReferenceAutocompleteWidgetTest::testEntityReferenceAutocompleteWidget
Behat\Mink\Exception\ResponseTextException: The text "Page test" appears in the text of this page, but it should not.

/var/www/html/vendor/behat/mink/src/WebAssert.php:785
/var/www/html/vendor/behat/mink/src/WebAssert.php:279
/var/www/html/core/tests/Drupal/FunctionalJavascriptTests/EntityReference/EntityReferenceAutocompleteWidgetTest.php:121
Krzysztof Domański’s picture

Verifying that EntityReferenceAutocompleteWidgetTest failed each time in PostgreSQL.

build:
  assessment:
    testing:
      run_tests.javascript:
        concurrency: 15
        types: 'PHPUnit-FunctionalJavascript'
        testgroups: '--class "Drupal\FunctionalJavascriptTests\EntityReference\EntityReferenceAutocompleteWidgetTest"'
        suppress-deprecations: false
        halt-on-fail: false
        repeat: 10

Result: 10/10 failed https://www.drupal.org/pift-ci-job/1430212.

hchonov’s picture

I've tried finding out what is the problem with EntityReferenceAutocompleteWidgetTest.

In this test we create two entries named Test page and Page test. After that we enter Test in the autocomplete field and assert that only Test 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 and Page test are correct results I think that we shouldn't assert that specifically Test page is returned, but instead ensure that only one of them is returned. I've updated the patch accordingly.

larowlan’s picture

@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

acbramley’s picture

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

Fixes postgres issue so back to RTBC

larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1108909 and pushed to 9.0.x. Thanks!

c/p to 8.8 and 8.9

  • larowlan committed 1108909 on 9.0.x
    Issue #2863188 by acbramley, Erik Frèrejean, Maouna, andypost, init90,...

  • larowlan committed ddc21a1 on 8.8.x
    Issue #2863188 by acbramley, Erik Frèrejean, Maouna, andypost, init90,...
  • larowlan committed e847676 on 8.9.x
    Issue #2863188 by acbramley, Erik Frèrejean, Maouna, andypost, init90,...
xjm’s picture

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

jonathan1055’s picture

Due to the @trigger_error deprecation message which is created in contrib tests, I added match_limit: 10 to my contrib module test content type in tests/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 with

Drupal\Core\Config\Schema\SchemaIncompleteException:
Schema errors for core.entity_form_display.node.scheduler_api_test.default with the following errors:
core.entity_form_display.node.scheduler_api_test.default:content.uid.settings.match_limit missing schema

Is 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?

Berdir’s picture

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

jonathan1055’s picture

Thanks @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+

Status: Fixed » Closed (fixed)

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

Berdir’s picture

FWIW, 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.

jimafisk’s picture

On drupal 8.7.10, #119 worked for me. Thanks!

oknate’s picture

I 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:

  /**
   * {@inheritdoc}
   */
  protected function prepareSettings() {
    $drupal_version = (float) substr(\Drupal::VERSION, 0, 3);
    if ($drupal_version < 8.8) {
      // Fix entity_reference_autocomplete match_limit schema errors.
      $this->strictConfigSchema = FALSE;
    }
    parent::prepareSettings();
  }

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?

markconroy’s picture

Hi 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:
11 nodes

But when I go to the menu UI to add a link, I can (still) only see 10:
10 nodes

oknate’s picture

I don't think menu links use EntityReferenceAutocompleteWidget. So perhaps add a separate follow-up issue for menu links.

andypost’s picture

lucasantunes’s picture

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

liquidcms’s picture

deleted