Deprecate module_config_sort()

Moved comment

probably it could be added to @param description

+++ b/core/lib/Drupal/Component/Utility/SortArray.php
@@ -128,8 +128,7 @@ public static function sortByKeyInt($a, $b, $key) {
    * Sorts a keyed array by weight and keys.
...
-   * The list is always sorted on write to avoid the overhead on read.
+   * Array values are supposed to be casted to integer as weights for keys.
    *
    * @param array $array
    *   A keyed array of names with weight as value.

I think it makes sense to point about cast to in for array values

@anypost,
Made changes as per comment #26.
Needs review.

Believe #27 addresses #26

+++ b/core/lib/Drupal/Component/Utility/SortArray.php
@@ -125,4 +125,38 @@ public static function sortByKeyInt($a, $b, $key) {
+  /**
+   * Sorts a keyed array by weight and keys.
+   *
+   * @param array $array
+   *   A keyed array of names with weight as value.
+   *   Array values are supposed to be casted to integer as weights for keys.
+   *
+   * @return array
+   *   An array sorted by weight and name.
+   *
+   * @internal
+   */
+  public static function sortByWeightAndKeys(array $array): array {

If we're moving this functionality to the utility class we should be adding explicit and extensive test coverage of this to \Drupal\Tests\Component\Utility\SortArrayTest.

Also the name of this method is interesting I think it should be something else. The other usage of the word weight in this class refers to a weight property. Here we're sorting an array of integers by the value and then by the key. I think the method name should reflect that. To me this is really sortByIntValueAndKey(). I think the typehint should be @param int[] $array.

Changed method name but still needs test.

Meanwhile I think we can remove this method at all in case of related 📌 Deprecate the trusted data concept in configuration Active
The new config class can care about sorting module list before saving the config if I got it right

Added a test including one that I think should pass, but fails.

For some reason the diff was whacky. Changing the order of methods seems to make it behave.

The last submitted patch, 31: 3262807-31.patch, failed testing. View results →

+++ b/core/tests/Drupal/Tests/Component/Utility/SortArrayTest.php
@@ -304,6 +304,77 @@ public function providerSortByTitleProperty() {
+    $tests['negative_weights'] = [
+      [
+        'aaa' => -2,
+        'bbb' => -1,
+        'ccc' => -3,
+      ],
+      [
+        'bbb' => -3,
+        'aaa' => -2,
+        'ccc' => -1,
+      ],
+    ];

This should sort asc right?

The last submitted patch, 32: 3262807-32.patch, failed testing. View results →

I think this can be greatly simplified to just an array multisort like:

array_multisort(array_values($array), SORT_ASC, array_keys($array), SORT_ASC, $array);

This modifies the original array that gets passed in by reference so calling code will need to change too. I think this is more in line with how usort etc. work though.

Seems to have some failures.

Have not re-reviewed yet.

Fixes test fails due to dodgy extension config setting logic.

Fix more extension config setting issues.

The last submitted patch, 38: 3262807-38.patch, failed testing. View results →

The last submitted patch, 39: 3262807-39.patch, failed testing. View results →

Fixes test fails.

I wonder if this should just be sortByWeightAndKey() since that's what we use it for?

Just re-read the comments and @alexpott suggested sortByIntValueAndKey() back in #29.

+++ b/core/lib/Drupal/Component/Utility/SortArray.php
@@ -125,4 +125,15 @@ public static function sortByKeyInt($a, $b, $key) {
+    array_multisort(array_values($array), SORT_ASC, array_keys($array), SORT_ASC, $array);

Looking at https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/modules/mig...

I think it should add SORT_NUMERIC to value and SORT_STRING to keys

+++ b/core/lib/Drupal/Component/Utility/SortArray.php
@@ -125,4 +125,15 @@ public static function sortByKeyInt($a, $b, $key) {
+   * @param array $array
+   *   key: (string) A key name.
+   *   value: (int) A weight integer.

Missing "A Keyed array..."

Fixes for #45 and #46.

I also bumped the deprecation message to 10.2.x in anticipation of it opening soon.

More docblock feedback from @andypost in slack.

I find it ready, let's wait for 10.2 opening

The last submitted patch, 48: 3262807-48.patch, failed testing. View results →

Looks like a random fail. Happening in HEAD too apparently.

Yes, back to RTBC

Ref Drupal\FunctionalTests\Installer\InstallerExistingConfigMultilingualTest

1. +++ b/core/tests/Drupal/Tests/Component/Utility/SortArrayTest.php
   @@ -304,6 +304,89 @@ public function providerSortByTitleProperty() {
   +    $tests['negative_weights_with_equal'] = [
   +      [
   +        'bbb' => -1,
   +        'aaa' => -1,
   +        'ccc' => -3,
   +      ],
   +      [
   +        'ccc' => -3,
   

   Can we add a test case that uses a string value that is numeric e.g. '3'

2. +++ b/core/lib/Drupal/Component/Utility/SortArray.php
   @@ -125,4 +125,16 @@ public static function sortByKeyInt($a, $b, $key) {
   +  public static function sortByIntValueAndKey(array &$array): void {
   +    array_multisort(array_values($array), SORT_ASC, SORT_NUMERIC, array_keys($array), SORT_ASC, SORT_NATURAL, $array);
   

   Or alternatively can we add a check that each value is an integer?

Changing this to a bug as the current implementation doesn't correctly order negative weights.

#53.1

Added a test for passing string values (of numbers) that can still be sorted numerically.

Reviewed this patch with @kim.pepper at DrupalSouth. I think going with #53.1 makes sense. The patch looks good now so setting it back to RTBC.

The last submitted patch, 56: 3262807-56.patch, failed testing. View results →

random failure

The last submitted patch, 56: 3262807-56.patch, failed testing. View results →

random failure

The last submitted patch, 56: 3262807-56.patch, failed testing. View results →

Seems like the random failure popping up today.

The last submitted patch, 56: 3262807-56.patch, failed testing. View results →

failed Drupal\Tests\views\Functional\Plugin\DisplayFeedTranslationTest::testFeedFieldOutput(), re-queued

Tests are ok now

The last submitted patch, 56: 3262807-56.patch, failed testing. View results →

Failures after 🐛 Strengthen TransactionManager Needs work

I'm triaging RTBC issues → . I read the IS and the comments. I didn't find any unanswered questions or other work to do.

I noted that #53 suggesting either testing for an integer or testing for numeric. I read the patch and it adds a test for numeric values proving that the method works for integer and numeric values. However, the method name is still, sortByIntValueAndKey, which seems misleading. Shouldn't it be sortByNumericValueAndKey? Although, I guess one could also argue that is should 'Natural' should be in the title as well.

1. +++ b/core/lib/Drupal/Component/Utility/SortArray.php
   @@ -125,4 +125,16 @@ public static function sortByKeyInt($a, $b, $key) {
   +   * Sorts an associative array by int value then key.
   

   "int" is not a word. But this also accepts numeric values.

2. +++ b/core/lib/Drupal/Component/Utility/SortArray.php
   @@ -125,4 +125,16 @@ public static function sortByKeyInt($a, $b, $key) {
   +   *   - value: An integer.
   

   The tests include testing numeric values, so is this correct?

3. +++ b/core/tests/Drupal/Tests/Component/Utility/SortArrayTest.php
   @@ -304,6 +304,101 @@ public function providerSortByTitleProperty() {
   +   *   An array of tests, matching the parameter inputs for
   +   *   sortByIntValueAndKey.
   

   This is rather generic, merely describing what any data provider does. I suppose the test should have a case for alphanumeric keys.

Instead of setting back to NW I opted to make a patch changing the method name and updating the comments. Is this better?

Good point about numeric, the only question is a test for floats (as they allowed)

doc-block is fine to keep with int

+++ b/core/lib/Drupal/Component/Utility/SortArray.php
@@ -125,4 +125,16 @@ public static function sortByKeyInt($a, $b, $key) {
+   * Sorts an associative array by numeric value then key.
+   *
+   * @param int[] $array
+   *   An associative array containing:
+   *   - key: A string.
+   *   - value: A numeric value.
+   */
+  public static function sortByNumericValueAndKey(array &$array): void {
+    array_multisort(array_values($array), SORT_ASC, SORT_NUMERIC, array_keys($array), SORT_ASC, SORT_NATURAL, $array);

There's no ability to point (int|float)[]

There an example of floats being used?

technically that's allowed but not sure in a wild

Is it something that should be allowed? Would it be worth a follow up to look into disallowing or throwing an error if floats are passed?

Going to mark but do wonder if we should prevent floats.

I think it's fine to document that the values should be integers; if it coincidentally works with floats that is not something we officially support as we have not documented it, but also something that I don't think we should spend time checking for or throwing errors for.

The updated method name has not been correctly replaced in all places, such as comments and the deprecation notice, back to NW for that.

Assigned and saved issue credits.

1. +++ b/core/includes/module.inc
   @@ -104,8 +109,14 @@ function module_set_weight($module, $weight) {
   + *   \Drupal\Component\Utility\SortArray::sortByIntValueAndKey() instead.
   

   Method name needs updating.

2. +++ b/core/includes/module.inc
   @@ -104,8 +109,14 @@ function module_set_weight($module, $weight) {
   +  @trigger_error(__FUNCTION__ . '() is deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. Use \Drupal\Component\Utility\SortArray::sortByIntValueAndKey() instead. See https://www.drupal.org/node/3262814', E_USER_DEPRECATED);
   

   Method name needs updating.

3. +++ b/core/tests/Drupal/Tests/Component/Utility/SortArrayTest.php
   @@ -304,6 +304,114 @@ public function providerSortByTitleProperty() {
   +   * @dataProvider providerSortByIntValueAndKey
   ...
   +  public function testSortByIntValueAndKey(array $a, array $expected): void {
   ...
   +   * Data provider for SortArray::sortByIntValueAndKey().
   ...
   +   * @see \Drupal\Tests\Component\Utility\SortArrayTest::testSortByIntValueAndKey()
   ...
   +  public function providerSortByIntValueAndKey(): array {
   

   Method names need updating.

Converted to MR and fixed #76

I bet it will be easier to improve docs via MR for #75

Could it be as simple as

value: A numeric value. Recommended to use integer values, floats will work but not supported

?

Changed https://git.drupalcode.org/project/drupal/-/merge_requests/5106/diffs?di...

Comment looks good. Gitlab is all green.

Please remember to hide patch files from the IS when an issue is converted to MR. Thanks!

Posted a number of points on the MR. I have several concerns about the data typing and integrity as well as possible behavior changes from this deprecation.

This will need config subsystem maintainer signoff before it is committed. It is also a significant change that may impact the data integrity of the config sort if done incorrectly, so it should have careful committer review in its final state.