Problem/Motivation

There seems to be a problem with the filters in views when the options are content types and the content types have numerical machine names.
I have a content type with a machine name of "1200" and want to filter a view by this content type, but the value of the checkbox is 0 instead of 1200.
I would take a look at this myself, but am not sure where the code for these options is generated.

views query

Proposed resolution

Sorting changed in commit c0f3934 due to problem with PHP 5. We can revert back now.

"Revert" commit c0f3934.

--- a/core/modules/views/src/Plugin/views/filter/Bundle.php
+++ b/core/modules/views/src/Plugin/views/filter/Bundle.php
@@ -110,7 +110,7 @@ public function getValueOptions() {
         $options[$type] = $info['label'];
       }
 
-      array_multisort($options, SORT_ASC, SORT_REGULAR, array_keys($options));
+      asort($options);
       $this->valueOptions = $options;
     }
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

2pha created an issue. See original summary.

Lendude’s picture

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

Ha! Had not seen this one before, I can confirm this happens on a clean D8 Umami install.

This is probably going to have something to do with having mixed array keys and PHP type casting fun.

apaderno’s picture

Version: 8.8.x-dev » 8.9.x-dev
2pha’s picture

Seems the offending function is `getAllBundleInfo` in the `EntityTypeBundleInfo` Class.
Using the entity id as an array key.
https://git.drupalcode.org/project/drupal/blob/8.8.x/core/lib/Drupal/Cor...

It is being called, in my case, the bundle filter.
https://git.drupalcode.org/project/drupal/blob/8.8.x/core/modules/views/...

apaderno’s picture

Title: numerical machine name problems in views filters » Numerical machine names create problems in view filters
Lendude’s picture

Status: Active » Needs review
FileSize
1.9 KB

The one that breaks it is actually:
https://git.drupalcode.org/project/drupal/blob/8.8.x/core/modules/views/...

Per https://www.php.net/manual/en/function.array-multisort.php:

array_multisort() can be used to sort several arrays at once, or a multi-dimensional array by one or more dimensions.

Associative (string) keys will be maintained, but numeric keys will be re-indexed.

This adds a test for this. No fix yet, but somehow we need to reset the numeric keys after ordering.

Status: Needs review » Needs work

The last submitted patch, 6: 3097765-6._TEST_ONLYpatch.patch, failed testing. View results

2pha’s picture

@Lendude, ahh yes,...
I am wondering why array_multisort is even used, This array is never going to be multi dimensional as far as I can see, seems from the issue below it was changed to make the test pas ???.
It was changed from asort() in this issue / commit.
Issue: https://www.drupal.org/node/3041778
commit: https://git.drupalcode.org/project/drupal/commit/c0f3934ce329c5200cfed2f...

2pha’s picture

Lendude’s picture

+++ b/core/modules/views/src/Plugin/views/filter/Bundle.php
@@ -110,7 +110,7 @@ public function getValueOptions() {
+      ksort($options, SORT_NATURAL);

Hmm we really want to use asort because we want to order by label not by machinename, but since the revert was done for PHP5 and we don't need to worry about that anymore, I don't see why we can't just go back to asort here.

Also, can you roll the test-only into the fix patch too? That way we can always see that the fix actually fixes the problem :)

MaskOta’s picture

Is this a BC break because of a different sort order?

Lendude’s picture

Component: views_ui.module » views.module

@MaskOta no, the sort order should stay the same if we order by title, it just won't destroy the keys (even is the order would change, I don't think we should mark this as a BC break)

2pha’s picture

Ahh, I should have used asort($options, SORT_NATURAL).
I was making these changes after running the quick-start comments, and could not seem to get the testing environment working, so did run the tests.

2pha’s picture

Woops, sorry, patch is not good... Am trying to fit this in between doing other things..
And for some reason my wife decided she wanted to tint my eyebrows while I am sitting at my computer....
I also just noticed that ksort (SORT_NATURAL) will get the numerals to always be above the strings, which asort wont, which is probably why I chose to use it in the first place.

2pha’s picture

Lendude’s picture

Status: Needs work » Needs review

The problem with asort vs ksort is that if I have a NodeType with machinename '123' but with a label 'OneTwoThree' we want it to be sorted for display in the UI based on the label, not the numeric machinename. Hence we need to asort, not ksort.

We should maybe expand the test to include a NodeType that shows this.

Lendude’s picture

And before I forget, thanks @2pha for looking into this issue!

The last submitted patch, 14: Fix-bundle-filter-sort-3097765-14.patch, failed testing. View results

2pha’s picture

No worries, I need this in to move forward with a D7 to D8 upgrade.

Status: Needs review » Needs work

The last submitted patch, 15: Fix-bundle-filter-sort-3097765-15.patch, failed testing. View results

2pha’s picture

Lendude’s picture

Status: Needs work » Needs review

@2pha when you upload a new patch, set the Status to 'needs review', this will trigger the test bot. Also try to always include an interdiff, makes reviewing much easier https://www.drupal.org/documentation/git/interdiff

Status: Needs review » Needs work

The last submitted patch, 21: Fix-bundle-filter-sort-3097765-20.patch, failed testing. View results

Krzysztof Domański’s picture

Status: Needs review » Needs work

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

Krzysztof Domański’s picture

My mistake. This change was intended. Restored.

Krzysztof Domański’s picture

Status: Needs review » Reviewed & tested by the community

Changed in commit c0f3934 due to problem with PHP 5. We can revert back now.

--- a/core/modules/views/src/Plugin/views/filter/Bundle.php
+++ b/core/modules/views/src/Plugin/views/filter/Bundle.php
@@ -103,7 +103,7 @@ public function getValueOptions() {
         $options[$type] = $info['label'];
       }
 
-      asort($options);
+      array_multisort($options, SORT_ASC, SORT_REGULAR, array_keys($options));
       $this->valueOptions = $options;

Only a small change so I think I can RTBC. See interdiff. I tested locally.

Krzysztof Domański’s picture

Issue summary: View changes
Lendude’s picture

Even with little changes, you shouldn't RTBC your own patches :) But RTBC +1 if this comes back green.

+++ b/core/modules/views/tests/src/Kernel/Entity/FilterEntityBundleTest.php
@@ -32,13 +32,14 @@ public function testFilterEntity() {
-      for ($i = 0; $i < 5; $i++) {
+      for ($i = 0; $i < 3; $i++) {

For reference, this change is needed because the view falls back to it's default of 10 items, and this is easier then adding a pager to the test view to bump the limit ¯\_(ツ)_/¯

The last submitted patch, 26: 3097765-test-only.patch, failed testing. View results

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

So this change is correct. And the move to array_multisort() was wrong. What happened was in the move to a KernelTestBase test $this->drupalCreateContentType(['type' => 'test_bundle']); was changed to NodeType::create(['type' => 'test_bundle'])->save(); - thereby causing all the node types this test is creating to have a NULL value for a label.

Let's not continue making that mistake here. We should add labels ie...

    NodeType::create(['type' => 'test_bundle', 'name' => 'Test 1'])->save();
    NodeType::create(['type' => 'test_bundle_2', 'name' => 'Test 2'])->save();
    NodeType::create(['type' => '180575', 'name' => '180575'])->save();
Krzysztof Domański’s picture

Status: Needs work » Needs review
FileSize
1.01 KB
2.69 KB
Lendude’s picture

Status: Needs review » Reviewed & tested by the community

Feedback from @alexpott has been addressed, thanks @Krzysztof Domański. Back to RTBC.

alexpott’s picture

Version: 8.9.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed 001fdae1f8 to 9.0.x and 584f1574de to 8.9.x. Thanks!

Going to ask other committers about backporting to 8.8.x.

  • alexpott committed 001fdae on 9.0.x
    Issue #3097765 by Krzysztof Domański, 2pha, Lendude, alexpott: Numerical...

  • alexpott committed 584f157 on 8.9.x
    Issue #3097765 by Krzysztof Domański, 2pha, Lendude, alexpott: Numerical...
alexpott’s picture

Status: Patch (to be ported) » Fixed

Catch +1'd the backport.

  • alexpott committed b4a5ffc on 8.8.x
    Issue #3097765 by Krzysztof Domański, 2pha, Lendude, alexpott: Numerical...

Status: Fixed » Closed (fixed)

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