Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
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.
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;
}
Comment | File | Size | Author |
---|---|---|---|
#32 | 3097765-32.patch | 2.69 KB | Krzysztof Domański |
#32 | interdiff-26-32.txt | 1.01 KB | Krzysztof Domański |
#26 | interdiff-24-26.txt | 644 bytes | Krzysztof Domański |
#26 | 3097765-26.patch | 2.42 KB | Krzysztof Domański |
#26 | 3097765-test-only.patch | 1.89 KB | Krzysztof Domański |
Comments
Comment #2
LendudeHa! 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.
Comment #3
apadernoComment #4
2phaSeems 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/...
Comment #5
apadernoComment #6
LendudeThe 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:
This adds a test for this. No fix yet, but somehow we need to reset the numeric keys after ordering.
Comment #8
2pha@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...
Comment #9
2phaComment #10
LendudeHmm 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 :)
Comment #11
MaskOta CreditAttribution: MaskOta commentedIs this a BC break because of a different sort order?
Comment #12
Lendude@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)
Comment #13
2phaAhh, 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.
Comment #14
2phaWoops, 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.
Comment #15
2phaComment #16
LendudeThe 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.
Comment #17
LendudeAnd before I forget, thanks @2pha for looking into this issue!
Comment #19
2phaNo worries, I need this in to move forward with a D7 to D8 upgrade.
Comment #21
2phaBack to asort
Comment #22
Lendude@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
Comment #24
Krzysztof DomańskiComment #26
Krzysztof DomańskiMy mistake. This change was intended. Restored.
Comment #27
Krzysztof DomańskiChanged in commit c0f3934 due to problem with PHP 5. We can revert back now.
Only a small change so I think I can RTBC. See interdiff. I tested locally.
Comment #28
Krzysztof DomańskiComment #29
LendudeEven with little changes, you shouldn't RTBC your own patches :) But RTBC +1 if this comes back green.
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 ¯\_(ツ)_/¯
Comment #31
alexpottSo 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 toNodeType::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...
Comment #32
Krzysztof DomańskiComment #33
LendudeFeedback from @alexpott has been addressed, thanks @Krzysztof Domański. Back to RTBC.
Comment #34
alexpottCommitted 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.
Comment #37
alexpottCatch +1'd the backport.