Problem/Motivation

I noticed this problem while working on #3089525: Sort options should correspond to bundles selected for entity reference field settings.

Using the Standard profile,

  1. Visit /admin/structure/types/manage/article/fields
  2. Click the "Add field" button.
  3. Select Content under the Reference group.
  4. Enter a label, then "Save and continue".
  5. On the next screen, click the "Save field settings" button.
  6. In the "Reference type" section, select at least one content type. This will expose the "Sort by" select list.
  7. Select a "Sort by" field. The "Sort direction" select list should appear.
  8. De-select all content types.
  9. The "Sort by" option is hidden, but the "Sort direction" option is still there.

We have to be careful that the fix for this problem does not break reference fields for entity types, like User, that do not have bundles.

Proposed resolution

TBD

Remaining tasks

User interface changes

When no bundles are selected, the "Sort direction" option is hidden as well as the "Sort by" option.

Current behavior:

screenshot showing no "Sort by" option but the "Sort direction" option is visible

API changes

None

Data model changes

None

CommentFileSizeAuthor
#57 After applying patch #54 .png21.82 KBjanmejaig
#55 before patch #54 .png29.32 KBjanmejaig
#54 3158651-54.patch5.3 KBsarvjeetsingh
#2 3158651-2-test-only.patch1.4 KBbenjifisher
sort-direction-visible.png19.74 KBbenjifisher
#7 3158651-7.patch2.31 KBhimanshu_sindhwani
#9 3158651-9.patch2.3 KBhimanshu_sindhwani
#9 interdiff_7-9.txt840 byteshimanshu_sindhwani
#13 3158651-13.patch2.95 KBpaulocs
#14 3158651-14.patch2.96 KBpaulocs
#15 3158651-15.patch2.95 KBpaulocs
#15 interdiff.txt583 bytespaulocs
#18 sort_direction_is_not_hidden-3158651-18.patch2.59 KBmayurjadhav
#18 interdiff-7-18.txt2.07 KBmayurjadhav
#24 3158651-23-test-only.patch2.46 KBbenjifisher
#25 interdiff-3158651-2-23.txt2.07 KBbenjifisher
#26 3158651-26.patch4.03 KBpaulocs
#29 3158651-28.patch5.9 KBpaulocs
#31 3158651-31.patch5.9 KBpaulocs
#31 interdiff.txt3.55 KBpaulocs
#33 3158651-33.patch5.12 KBpaulocs
#33 interdiff-32-33.txt4.07 KBpaulocs
#34 Before-patch.png25 KBthalles
#34 after-patch.png18.9 KBthalles
#37 3158651-37.patch5.17 KBabhisekmazumdar
#37 interdiff-33-37.txt2.53 KBabhisekmazumdar
#39 3158651-39.patch5.21 KBabhisekmazumdar
#39 interdiff.37-39.txt1.24 KBabhisekmazumdar
#41 3158651-41.patch5.21 KBVidushi Mehta
#41 interdiff-39-41.txt920 bytesVidushi Mehta
#51 3158651-51.patch5.21 KBsarvjeetsingh
#51 interdiff_41-51.txt604 bytessarvjeetsingh

Comments

benjifisher created an issue. See original summary.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Assigned: benjifisher » Unassigned
Status: Active » Needs review
StatusFileSize
new 3158651-2-test-only.patch1.4 KB

Here is a failing test. I will set the status to NR, and the testbot should set it back to NW.

Status: Needs review » Needs work

The last submitted patch, 2: 3158651-2-test-only.patch, failed testing. View results

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Status: Needs work » Needs review

The patch in #2 failed in the expected way. So far, so good! now we just have to fix the bug. ;)

In core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/DefaultSelection.php, I see the following:

    $configuration = $this->getConfiguration();
// ...
      if ($configuration['sort']['field'] != '_none') {
        $form['sort']['settings']['direction'] = [
          '#type' => 'select',
          '#title' => $this->t('Sort direction'),
          '#required' => TRUE,
          '#options' => [
            'ASC' => $this->t('Ascending'),
            'DESC' => $this->t('Descending'),
          ],
          '#default_value' => $configuration['sort']['direction'],
        ];
      }

I am not sure where the configuration is set, but presumably from $form_state when the form is submitted. (Also, there is an AJAX submission when the Content type checkboxes are changed.)

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Status: Needs review » Needs work

Oops, I did not mean to set the status to NR. It is still NW.

mayurjadhav’s picture

mayurjadhav commented
Assigned: Unassigned » mayurjadhav
himanshu_sindhwani’s picture

himanshu_sindhwani commented
Assigned: mayurjadhav » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3158651-7.patch2.31 KB

I tried to fix the issue with a small change in conditions. Please review.

Status: Needs review » Needs work

The last submitted patch, 7: 3158651-7.patch, failed testing. View results

himanshu_sindhwani’s picture

himanshu_sindhwani commented
Status: Needs work » Needs review
StatusFileSize
new 3158651-9.patch2.3 KB
new interdiff_7-9.txt840 bytes

Changing the test case according to the fix in patch.

Status: Needs review » Needs work

The last submitted patch, 9: 3158651-9.patch, failed testing. View results

Kumar Kundan’s picture

Kumar Kundan commented
Assigned: Unassigned » Kumar Kundan
paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented

Also if we apply patch #9, it will not be able to choose "Sort direction" if it is used a User field reference.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
StatusFileSize
new 3158651-13.patch2.95 KB

Created a new patch but it needs to re-write the tests.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Status: Needs work » Needs review
StatusFileSize
new 3158651-14.patch2.96 KB

Patch with the test.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
StatusFileSize
new 3158651-15.patch2.95 KB
new interdiff.txt583 bytes

New patch with code standard fix.

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

Status: Needs review » Needs work

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

mayurjadhav’s picture

mayurjadhav commented
Status: Needs work » Needs review
StatusFileSize
new sort_direction_is_not_hidden-3158651-18.patch2.59 KB
new interdiff-7-18.txt2.07 KB

Updated the patch for failed test for #7, Kindly review.

Kumar Kundan’s picture

Kumar Kundan commented

#11 assigned by me to work on this issue. @mayurjadhav & @paulocs you hijacked the issue. I request you to follow the drupal issue etiquette & show your respect to other contributers.

Kumar Kundan’s picture

Kumar Kundan commented
Assigned: Kumar Kundan » Unassigned
paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Status: Needs review » Needs work

Sorry @Kumar Kundan, I thought it would be no problem to work in the issue.

Now about the issue, as I said in comment #12, if we apply patch #18, it is not possible to choose a "Sort direction" if the field is a user reference.

Try to fix the tests from patch #15 @mayurjadhav.

Cheers, Paulo.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Assigned: Unassigned » benjifisher

@Kumar Kundan:

Thanks for the link to the page on issue etiquette. Item 8 on that page is

Don't: Assign yourself to an issue unless you intend to work on it in a fairly short timeframe. State clearly in the comment where you assign yourself what you intend to do, and when a progress report can be expected. Assigning yourself to issues block [sic] others from working on them, so you need to be very careful when doing this.

If you want others to follow the guidance on that page, then please set a good example and leave such a comment when assigning an issue to yourself.

When I created this issue, I intended to work on it myself, but I respected the assignments in #6 and #11.

In the issue summary, I wrote,

We have to be careful that the fix for this problem does not break reference fields for entity types, like User, that do not have bundles.

and @paulocs points out that the patches so far did not pay attention to that.

I am assigning this issue to myself:

  • I will add test coverage to a test-only patch to make sure that we do not break the sort order for User references.
  • I will attempt to fix the bug.
  • If this issue is still assigned to me on Monday, August 3, then feel free to assign it to yourself or to make it unassigned.
benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented

Here is a new test-only patch, along with an interdiff comparing it to the one in #2. It makes two changes to the tests:

After de-selecting all bundles and checking that the sort order is not visible, re-select them so that the rest of the test will continue as before.

Test that User references can be sorted, and that the sort direction can be selected, as discussed in my previous comment.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
StatusFileSize
new 3158651-23-test-only.patch2.46 KB
benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
StatusFileSize
new interdiff-3158651-2-23.txt2.07 KB
paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Assigned: benjifisher » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3158651-26.patch4.03 KB

As @benjifisher allowed, I unassigned the issue and I attached a new patch with a possible solution for the issue.
Lets see what the tests bots will say.

Cheers, Paulo.

Status: Needs review » Needs work

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

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented

If you want to use #states to control visibility, then

  1. You have to change tests like $this->assertNoFieldByName('settings[handler_settings][sort][direction]'); to test visibility.
  2. You have to make sure that, when hidden and required, the sort direction has a value.

Maybe (2) is not a problem. It might default to ascending.

Are you set up to run functional javascript tests locally? The feedback loop is painfully slow if you have to wait for the testbot to check your work.

Since $form['target_bundles'] has '#ajax' => TRUE, the form is rebuilt whenever you change the selected bundles. I spent some time on Saturday trying to figure out how to get $configuration['sort']['field'] to update properly. It seems that validateConfigurationForm() is called, and I tried updating $this->configuration or $form_state there, but I could not get it to work. I know that $this->configuration is set from the constructor, but I am not sure what values are passed to the constructor.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Status: Needs work » Needs review
StatusFileSize
new 3158651-28.patch5.9 KB

Hey @benjifisher!

I did a new patch for it and I change also the tests. As I'm still learning how to create tests, I don't if I wright the best test code but I could manage it.

Please review it.

Cheers, Paulo.

Status: Needs review » Needs work

The last submitted patch, 29: 3158651-28.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Status: Needs work » Needs review
StatusFileSize
new 3158651-31.patch5.9 KB
new interdiff.txt3.55 KB

New patch with the right tests.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Status: Needs review » Needs work

@paulocs:

  1. Is there a reason you use $page->find() instead of $page->findField() here?

     +++ b/core/modules/field/tests/src/FunctionalJavascript/EntityReference/EntityReferenceAdminTest.php
 @@ -126,10 +126,16 @@ public function testFieldAdminHandler() {
      // Option 0: no sort.
      $this->assertFieldByName('settings[handler_settings][sort][field]', '_none');
      $sort_by = $page->findField('settings[handler_settings][sort][field]');
 -    $this->assertNoFieldByName('settings[handler_settings][sort][direction]');
 +    $sort_direction = $page->find('css' , '.form-item-settings-handler-settings-sort-direction');
 +    $style = $sort_direction->getAttribute('style');
 +    $isDisplayed = $style == 'display: none;' ? FALSE : TRUE;
 +    $this->assertFalse($isDisplayed, 'Sort direction is not displayed');

    I think my patches defined $sort_direction using findField(). It makes the test easier to read if we do similar tasks the same way. Also, CSS classes are more likely to change than field names, so your version is more likely to need updates in the future.

  2. Same snippet: use the isVisible() method. The same argument for consistency applies. It is also more robust: the style attribute might end up being something like display: none; font-size: larger;. It would still be hidden, but your $isDisplayed would be set to TRUE.

  3. Same snippet: under Functions and variables in the Drupal coding standards, it says, "Be consistent; do not mix camelCase and snake_case variable naming inside a file.".

  4. Use findField() and isVisible() elsewhere as well. I will not list all the places they can be used.

  5.   -    $assert_session->waitForField('settings[handler_settings][sort][direction]');
  +    $assert_session->assertWaitOnAjaxRequest();

    Why make this change? It makes your patch harder to review, so only change it if there is a reason to.

  6. I am nervous about the general approach, adding an input field to the form but hiding it in some cases. Does that mean some values will get processed that should not be there? Maybe it is the right thing to do: the form has probably been around for a long time, and the #states property was not always available. The fact that it passes tests is a good sign. Sometimes it is right to be bold rather than cautious.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Status: Needs work » Needs review
StatusFileSize
new 3158651-33.patch5.12 KB
new interdiff-32-33.txt4.07 KB

Hello @benjifisher,
I really appreciate your feedback! It makes me know more about Drupal and tests :)

As you pointed I made a new patch with some fixings:

1) I used $page->find() because I was searching the select field directly. I didn't know I could get the div field by name and use the isVisible() function. I fixed it in my new patch.

2) Now the patch is using the isVisible() function to verify the field visibility so even if the field style changes, the tests will not fail.

3) I fixed the drupal code standards!

4) New patch uses findField() and isVisible().

5) About what you pointed in topic 5, I'm still using $assert_session->assertWaitOnAjaxRequest(); because the tests will fail if I use $assert_session->waitForField('settings[handler_settings][sort][direction]');. I suppose that is because of the #states property.

6) I think it is no problem to use the #states property as you said and the tests didn't fail.

The interdiff file name is interdiff-32-33 but suppose to be interdiff-31-33.

Thanks!
Paulo.

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Before-patch.png25 KB
new after-patch.png18.9 KB

#33 work fine to me!
Before:

After patch in #33

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work

@thalles, thank you for manually testing the patch and making the screen shots. It will help reviews (and comitters) if you add the screens shots to the issues summary and noting the number of the patch you tested. Thanks.

The issue summary shows that the proposed resolution is TBD - that needs to be completed.

I applied the patch and found that a deprecated method is being used. Since it will be NW for that I also have a query about long lines.

  1. +++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/DefaultSelection.php
@@ -246,18 +246,25 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
+            ':input[name="settings[handler_settings][sort][field]"]' => ['!value' => '_none'],
...
+        $form['sort']['settings']['direction']['#states']['visible'][] = [':input[name^="settings[handler_settings][target_bundles]["]' => ['checked' => TRUE]];

    These are hard to read on one line, can we make them multi-line? 

  2. +++ b/core/modules/field/tests/src/FunctionalJavascript/EntityReference/EntityReferenceAdminTest.php
@@ -154,7 +156,24 @@ public function testFieldAdminHandler() {
+      $this->assertFieldByName('settings[handler_settings][target_bundles][' . $bundle_name . ']');
...
+      $this->assertFieldByName('settings[handler_settings][target_bundles][' . $bundle_name . ']');

    assertFieldByName is deprecated.

abhisekmazumdar’s picture

abhisekmazumdar
he/him
Primary language English
Location India
commented
Assigned: Unassigned » abhisekmazumdar

Updating the last patch.

abhisekmazumdar’s picture

abhisekmazumdar
he/him
Primary language English
Location India
commented
Assigned: abhisekmazumdar » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3158651-37.patch5.17 KB
new interdiff-33-37.txt2.53 KB

Hi @quietone
Thanks for you feedback.
I see the use of assertFieldByName in many places In the file core/modules/field/tests/src/FunctionalJavascript/EntityReference/EntityReferenceAdminTest.php and many other places too, so it will better if the clean up work is done other issue. As of now I have just replaced the code which are realated to the #33 patch.

himanshu_sindhwani’s picture

himanshu_sindhwani commented
Status: Needs review » Needs work

Few observations:

+++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/DefaultSelection.php
@@ -246,18 +246,29 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
+        $form['sort']['settings']['direction']['#states']['visible'][] = ¶

remove trailing space here.

+++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/DefaultSelection.php
@@ -246,18 +246,29 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
+            ':input[name="settings[handler_settings][sort][field]"]' => ['!value' => '_none'],

This is not broken to multi lines.

abhisekmazumdar’s picture

abhisekmazumdar
he/him
Primary language English
Location India
commented
Status: Needs work » Needs review
StatusFileSize
new 3158651-39.patch5.21 KB
new interdiff.37-39.txt1.24 KB

Oops My bad. Update the patch correctly. Thanks @himanshu_sindhwani

himanshu_sindhwani’s picture

himanshu_sindhwani commented
Status: Needs review » Needs work 
  1. +++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/DefaultSelection.php
@@ -246,18 +246,31 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
+              '!value' => '_none'

    Its good to add a comma after value '_none'. 

  2. +++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/DefaultSelection.php
@@ -246,18 +246,31 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
+          'checked' => TRUE

    Please correct the indentation here and also add comma after the value TRUE.

Vidushi Mehta’s picture

Vidushi Mehta commented
Status: Needs work » Needs review
StatusFileSize
new 3158651-41.patch5.21 KB
new interdiff-39-41.txt920 bytes

Added above mentioned correction.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Status: Needs review » Reviewed & tested by the community

I think now it is everything okay. Patch #41 looks good.
Set to RTBC.

Cheers, Paulo.

The last submitted patch, 24: 3158651-23-test-only.patch, failed testing. View results

The last submitted patch, 24: 3158651-23-test-only.patch, failed testing. View results

The last submitted patch, 24: 3158651-23-test-only.patch, failed testing. View results

The last submitted patch, 24: 3158651-23-test-only.patch, failed testing. View results

The last submitted patch, 24: 3158651-23-test-only.patch, failed testing. View results

The last submitted patch, 24: 3158651-23-test-only.patch, failed testing. View results

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented

I don't know why the tests are running on patch #24 if it not the last patch attached.

I will hide patch #24.

Status: Reviewed & tested by the community » Needs work

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

sarvjeetsingh’s picture

sarvjeetsingh commented
Status: Needs work » Needs review
StatusFileSize
new interdiff_41-51.txt604 bytes
new 3158651-51.patch5.21 KB

Added a comma after the last array element.

The last submitted patch, 51: 3158651-51.patch, failed testing. View results

tanubansal’s picture

tanubansal commented

#51 is not working on 9.1
Please check

sarvjeetsingh’s picture

sarvjeetsingh commented
StatusFileSize
new 3158651-54.patch5.3 KB

I have re-rolled the patch in #51.
core/modules/field/tests/src/FunctionalJavascript/EntityReference/EntityReferenceAdminTest.php had conflicts, see https://www.drupal.org/project/drupal/issues/3139406
please review

janmejaig’s picture

janmejaig commented
StatusFileSize
new before patch #54 .png29.32 KB

I have verified the above ticket and found that after applying patch #54 on Drupal 9.1.x it is working as expected.Please find the attachment for the same.

Please find the steps below to test the issue :

  1. Visit /admin/structure/types/manage/article/fields
  2. Click the "Add field" button.
  3. Select Content under the Reference group.
  4. Enter a label, then "Save and continue".
  5. On the next screen, click the "Save field settings" button.
  6. In the "Reference type" section, select at least one content type. This will expose the "Sort by" select list.
  7. Select a "Sort by" field. The "Sort direction" select list should appear.
  8. De-select all content types.
  9. The "Sort by" option is hidden, but the "Sort direction" option is still there.

Expected : When no bundles are selected, the "Sort direction" option is hidden as well as the "Sort by" option.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Status: Needs review » Reviewed & tested by the community
janmejaig’s picture

janmejaig commented
StatusFileSize
new After applying patch #54 .png21.82 KB

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

  • catch committed 4118ac6 on 9.2.x 
    Issue #3158651 by paulocs, benjifisher, abhisekmazumdar,...

  • catch committed fab2dc6 on 9.1.x 
    Issue #3158651 by paulocs, benjifisher, abhisekmazumdar,...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

#3132676: Use progressive disclosure on sort direction element of entity reference fields seems like a duplicate of this.

jibran credited Bunty Badgujar.

jibran credited Rkumar.

jibran credited mrinalini9.

jibran credited sharma.amitt16.

jibran credited solotandem.

jibran credited xjm.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Issue tags: +Bug Smash Initiative, +DER issue
Related issues: +#3132676: Use progressive disclosure on sort direction element of entity reference fields

Bringing the tags and credits over from duplicate issue #3132676: Use progressive disclosure on sort direction element of entity reference fields.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented

I guess I am the one who created the duplicate issue. Thanks for connecting them, @jibran!

xjm’s picture

xjm
she/her
Primary language English
commented
Version: 9.2.x-dev » 9.1.x-dev

Status: Fixed » Closed (fixed)

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