Problem/Motivation

drupal_sort_weight() is used in many of our classes, which cannot be unit tested whilst it lives in common.inc

Proposed resolution

Create a SortArray class and move drupal_sort_weight into it.

API changes

Non-BC-breaking API addition only

Calls to drupal_sort_weight() will be replaced by SortArray::sortWeight(). drupal_sort_weight() is preserved as a BC wrapper.

From within classes, calls to

    uasort($operations, 'drupal_sort_weight');

will now be:
    uasort($operations, array('Drupal\Component\Utility\SortArray', 'sortWeight'));
Files: 
CommentFileSizeAuthor
#52 drupal-2028499-52.patch15.34 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,863 pass(es).
[ View ]
#52 interdiff.txt557 bytesdawehner
#49 drupal_sort_array-2028499-49.patch15.38 KBtsphethean
PASSED: [[SimpleTest]]: [MySQL] 57,860 pass(es).
[ View ]
#45 drupal_sort_array-2028499-45.patch15.37 KBtsphethean
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#41 2028499-40.patch15.33 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 56,784 pass(es), 349 fail(s), and 9,901 exception(s).
[ View ]
#41 interdiff-2028499-40.txt3.19 KBdamiankloip
#40 drupal_sort_array-2028499-40.patch16.03 KBtsphethean
FAILED: [[SimpleTest]]: [MySQL] 57,173 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#36 drupal_sort_array-2028499-36.patch14.77 KBtsphethean
PASSED: [[SimpleTest]]: [MySQL] 56,821 pass(es).
[ View ]
#34 drupal_sort_array-2028499-34.patch14.72 KBtsphethean
PASSED: [[SimpleTest]]: [MySQL] 57,221 pass(es).
[ View ]
#32 2028499-32.patch14.15 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 56,791 pass(es).
[ View ]
#28 drupal_sort_weight-2028499-28.patch14.16 KBtsphethean
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_sort_weight-2028499-28_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#26 drupal_sort_weight-2028499-24.patch10.74 KBtsphethean
FAILED: [[SimpleTest]]: [MySQL] 57,655 pass(es), 347 fail(s), and 20,526 exception(s).
[ View ]
#22 drupal_sort_weight-2028499-22.patch18.78 KBtsphethean
FAILED: [[SimpleTest]]: [MySQL] 56,736 pass(es), 452 fail(s), and 23,560 exception(s).
[ View ]
#20 drupal_sort_weight-2028499-20.patch13.34 KBtsphethean
PASSED: [[SimpleTest]]: [MySQL] 58,324 pass(es).
[ View ]
#15 drupal_sort_weight-2028499-14.patch5.03 KBtsphethean
PASSED: [[SimpleTest]]: [MySQL] 58,122 pass(es).
[ View ]
#13 drupal_sort_weight-2028499-13.patch4.98 KBtsphethean
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#12 drupal_sort_weight-2028499-10.patch4.97 KBtsphethean
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#6 drupal_sort_weight-2028499-6.patch5.23 KBtsphethean
PASSED: [[SimpleTest]]: [MySQL] 57,935 pass(es).
[ View ]
#4 drupal_sort_weight-2028499-4.patch5.34 KBtsphethean
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_sort_weight-2028499-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#2 drupal_sort_weight-2028499-2.patch5.34 KBtsphethean
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_sort_weight-2028499-2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 drupal_sort_weight-2028499-1.patch5.4 KBtsphethean
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new5.4 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

StatusFileSize
new5.34 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_sort_weight-2028499-2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Added newlines to end of new files.

Status:Needs review» Needs work

The last submitted patch, drupal_sort_weight-2028499-2.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new5.34 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_sort_weight-2028499-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-rolled.

Status:Needs review» Needs work

The last submitted patch, drupal_sort_weight-2028499-4.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new5.23 KB
PASSED: [[SimpleTest]]: [MySQL] 57,935 pass(es).
[ View ]

and again...

Status:Needs review» Needs work

The last submitted patch, drupal_sort_weight-2028499-6.patch, failed testing.

Status:Needs work» Needs review

#6: drupal_sort_weight-2028499-6.patch queued for re-testing.

The failing test is passing on my local, so re-queueing.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

+++ b/core/includes/common.incundefined
@@ -5180,14 +5181,22 @@ function element_info_property($type, $property_name, $default = NULL) {
+  // Ensure we're passing an array to SortArray::sortWeight. If we didn't get an array in the
+  // first place then passing an empty array in will have the same effect as $a['weight'] will
+  // not be set.
+++ b/core/lib/Drupal/Component/Utility/SortArray.phpundefined
@@ -0,0 +1,41 @@
+   *   First item for comparison. The compared items should be associative arrays

Each line should be a maximum of 80 characters wide.

+++ b/core/includes/common.incundefined
@@ -5180,14 +5181,22 @@ function element_info_property($type, $property_name, $default = NULL) {
+  if (!is_array($a)) {
+    $a = array();
   }
...
+  if (!is_array($b)) {
+    $b = array();
+  }

We gotta fix that in the places where we are using it. Otherwise we can never deprecate drupal_sort_weight().

+++ b/core/lib/Drupal/Component/Utility/SortArray.phpundefined
@@ -0,0 +1,41 @@
+   *   First item for comparison. The compared items should be associative arrays

Same here (80 characters max.)

+++ b/core/lib/Drupal/Component/Utility/SortArray.phpundefined
@@ -0,0 +1,41 @@
+   * @return
+   *  The comparison result for uasort().

Missing whitespace in front of "The". Also, the return type is "int".

+++ b/core/tests/Drupal/Tests/Component/Utility/SortArrayTest.phpundefined
@@ -0,0 +1,105 @@
+   * @see          Drupal\Component\Utility\SortArray::sortWeight()
+   * @see          Drupal\Tests\Component\Utility\SortArrayTest::providerSortArray()

Remove these whitspaces after the @see's.

+++ b/core/tests/Drupal/Tests/Component/Utility/SortArrayTest.phpundefined
@@ -0,0 +1,105 @@
+   * @see          Drupal\Component\Utility\SortArray::sortWeight()
+   * @see          Drupal\Tests\Component\Utility\SortArrayTest::providerSortArray()
...
+   * @see Drupal\Component\Utility\SortArray::sortWeight()
+   * @see Drupal\Tests\Component\Utility\SortArrayTest::testSortArray()

Needs leading backslashes.

+++ b/core/includes/common.incundefined
@@ -5180,14 +5181,22 @@ function element_info_property($type, $property_name, $default = NULL) {
+ * @see \Drupal\Component\Utility\String::checkPlain()
+ *

The @see should be more linking to the sort array not check plain.

+++ b/core/lib/Drupal/Component/Utility/SortArray.phpundefined
@@ -0,0 +1,41 @@
+    if ($a_weight == $b_weight) {

It feels like having === would make more sense here?

StatusFileSize
new4.97 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

@fubhy - thanks for the comments. New patch attached to address the issues.

StatusFileSize
new4.98 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

And another patch to address the comments I missed!

Status:Needs review» Needs work

The last submitted patch, drupal_sort_weight-2028499-13.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new5.03 KB
PASSED: [[SimpleTest]]: [MySQL] 58,122 pass(es).
[ View ]

Revised @deprecated line as I heard another issue get rejected because version was missing.

Also created follow up to replace occurrences of drupal_sort_weight in core

#2029537: Replace drupal_sort_weight() with SortArray::sortByWeightElement() and remove drupal_sort_weight()

Status:Needs review» Reviewed & tested by the community

This looks pretty ok to me

This is not the first issue where I think the tail wags the dog -- that we have irrational dislike to include_once common.inc in phpunit tests should be no reason to move every line of code to a separate class hidden somewhere deep in the psr-0 maze.

StatusFileSize
new13.34 KB
PASSED: [[SimpleTest]]: [MySQL] 58,324 pass(es).
[ View ]

Following discussion on IRC, drupal_sort_weight will no longer be deprecated in this patch.

msonnabaum claims that we do not include common.inc because of hidden dependencies. This is a false argument -- if we are afraid of code calling random functions in common.inc then why do we use the autoloader -- the same argument could be used that we call random methods on random classes. What's the difference? I am not talking about theoretical questions like 'polluting the test environment' I am asking about a very practical, very concrete question: if the autoloader is on and the Drupal root is loaded then practically all code under core/lib is loaded. There's no difference between that and keeping the code in common.inc and loading that. As far as I can see at least.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new18.78 KB
FAILED: [[SimpleTest]]: [MySQL] 56,736 pass(es), 452 fail(s), and 23,560 exception(s).
[ View ]

Revised patch attached.

Renamed sortWeight to sortByNumber and provided a parameter to specify which array property to sort by. This so that element_sort and drupal_sort_weight can both use this method by passing a different property. Then applied the same principle to drupal_sort_title and element_sort_by_title by creating the sortbyString method.

Unit tests added for both methods.

+++ b/core/includes/common.incundefined
@@ -3901,11 +3901,10 @@ function drupal_clear_js_cache() {
-  return Json::encode($var);
@@ -3913,11 +3912,9 @@ function drupal_json_encode($var) {
+  return json_decode($var, TRUE);

dont think the Json stuff should be in here....

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetPluginManager.phpundefined
@@ -143,7 +143,7 @@ public function getOptions($field_type = NULL) {
+      uasort($widget_types, array('Drupal\Component\Utility\SortArray', 'sortWeight'));
+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -312,7 +312,7 @@ function taxonomy_overview_terms_submit($form, &$form_state) {
+  uasort($form_state['values']['terms'], array('Drupal\Component\Utility\SortArray', 'sortWeight'));

Should this use Use at the top of the file and just reference SortArray (and in the other examples) ?

use statements have no bearing on string references to classes, the full classname is required for callbacks like uasort

Status:Needs review» Needs work

The last submitted patch, drupal_sort_weight-2028499-22.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new10.74 KB
FAILED: [[SimpleTest]]: [MySQL] 57,655 pass(es), 347 fail(s), and 20,526 exception(s).
[ View ]

Ok, bit of a rethink here. The old way failed because it didn't lend us the flexibility of being able to use a single method to replace multiple function calls whilst still satisfying uasort. This version should do.

Status:Needs review» Needs work

The last submitted patch, drupal_sort_weight-2028499-24.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new14.16 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_sort_weight-2028499-28_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

#26 was over complicated a bit, so gone back to just moving drupal_sort_weight, drupal_sort_title, element_sort, element_sort_by_title into a class, and adding tests.

#28: drupal_sort_weight-2028499-28.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, drupal_sort_weight-2028499-28.patch, failed testing.

Issue tags:+PHPUnit Blocker

tagging

Status:Needs work» Needs review
StatusFileSize
new14.15 KB
PASSED: [[SimpleTest]]: [MySQL] 56,791 pass(es).
[ View ]

just a reroll, let's get this moving again. It's been a couple of weeks.

Status:Needs review» Needs work

We should mark all these methods as deprecated.

Status:Needs work» Needs review
StatusFileSize
new14.72 KB
PASSED: [[SimpleTest]]: [MySQL] 57,221 pass(es).
[ View ]

Added deprecated statements and also added @return docblock statements for the deprecated functions for completeness.

This adds test coverage for functions, which haven't been tested before.

+++ b/core/lib/Drupal/Component/Utility/SortArray.phpundefined
@@ -0,0 +1,111 @@
+  public static function sortByWeightElement(array $a, array $b) {
...
+  public static function sortByWeightProperty($a, $b) {
...
+  public static function sortByTitleElement($a, $b) {
...
+   public static function sortByTitleProperty($a, $b) {

A potential follow up could be to introduce a method sortByKey($a, $b, $key = 'weight') for example

+++ b/core/lib/Drupal/Component/Utility/SortArray.phpundefined
@@ -0,0 +1,111 @@
+   * @param $a
...
+   * @param $b
...
+   * @param $a
...
+   * @param $b
...
+   * @param $a
...
+   * @param $b

Missings types (should be array afaik)

+++ b/core/tests/Drupal/Tests/Component/Utility/SortArrayTest.phpundefined
@@ -0,0 +1,326 @@
+    // Weights set and equal
...
+    // Weights set and $a is less (lighter) than $b
...
+    // Weights set and $a is greater (heavier) than $b
...
+    // Weights not set
...
+    // Weights for $b not set
...
+    // Weights for $a not set
...
+    // Weights set and equal
...
+    // Weights set and $a is less (lighter) than $b
...
+    // Weights set and $a is greater (heavier) than $b
...
+    // Weights not set
...
+    // Weights for $b not set
...
+    // Weights for $a not set

Impressive test coverage! Let's put a dot at the end of all the docs.

StatusFileSize
new14.77 KB
PASSED: [[SimpleTest]]: [MySQL] 56,821 pass(es).
[ View ]

Thanks @dawehner

Missings types (should be array afaik)

Done

Impressive test coverage! Let's put a dot at the end of all the docs.

Done

A potential follow up could be to introduce a method sortByKey($a, $b, $key = 'weight') for example

Yes, this was what I'd been trying to do in #26, but because the methods are purely used as a callback for uasort(), I couldn't seem to get additional arguments (other than $a and $b) passed through to the callback. If there's a way to do this that I've missed then I'd be very interested. I think we could probably do that as a follow up once this is in, and the follow up #2029537: Replace drupal_sort_weight() with SortArray::sortByWeightElement() and remove drupal_sort_weight() is completed, so that we have tests to refactor against. Is that ok?

I suggested to tspethean on IRC that we create 2 new methods: sortByKeyInt and sortByKeyString (or something). the current methods can then call those internally. He seemed to be on board so I guess a patch will be on its way soon :)

Issue tags:+sad chx

No answer to #21 so tagging and unfollowing

Status:Needs review» Needs work

Well, unfollowing so he will not care about a potential comment?

Needs work based on #37

Status:Needs work» Needs review
StatusFileSize
new16.03 KB
FAILED: [[SimpleTest]]: [MySQL] 57,173 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Slightly messy patch here - but I'd be grateful for some help thinking this through.

Proxying to sortByKeyInt() method as suggest by @damiankloip works fine, with the only downside being the public methods can no longer be static (I think! Does this matter?) For the string sorting however, the logic for element_sort_by_title and drupal_sort_title are subtly different. Part of the difference can be resolved by passing the sort method (either strcasecmp or strnatcasecmp) into sortByKeyString().

However, drupal_sort_title - which becomes sortByTitleElement in this patch - checks for existance of array keys and returns a sort value before the comparison if they are missing, whilst element_sort_by_title does not.

The attached patch will have at least 2 failures, in the phpunit tests I had written for #36, but the assertions in those tests are based on what the existing logic was. What I'm curious about is whether there is any reason for those functions to be different (other than the sort method which seems fair enough if a little inconsistent), or whether this is a good opportunity to consolidate the logic?

StatusFileSize
new3.19 KB
new15.33 KB
FAILED: [[SimpleTest]]: [MySQL] 56,784 pass(es), 349 fail(s), and 9,901 exception(s).
[ View ]

@tspethean; yes I was thinking this could be a good opportunity to consolidate if we can. It seems like alot of similar really. I think we can still use static methods, I was thinking something along the lines of this - untested... :)

Status:Needs review» Needs work

The last submitted patch, 2028499-40.patch, failed testing.

heh, but probably without the array type hints on those new methods. You get what I mean though?

+++ b/core/tests/Drupal/Tests/Component/Utility/SortArrayTest.phpundefined
@@ -0,0 +1,326 @@
+  public function SortByTitleProperty() {

We should prefix these data provider methods with provider. So 'providerSortByTitleProperty'.

Yep, I see what you mean - OK, I'll go with that and update the assertions I've created to reflect what will now be the case. Thanks for the help. I'll post a new patch later.

Status:Needs work» Needs review
StatusFileSize
new15.37 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

Hopefully this will do the trick. Thanks for the pointers @damiankloip

Status:Needs review» Needs work
Issue tags:-PHPUnit Blocker, -sad chx

The last submitted patch, drupal_sort_array-2028499-45.patch, failed testing.

Status:Needs work» Needs review

#45: drupal_sort_array-2028499-45.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+PHPUnit Blocker, +sad chx

The last submitted patch, drupal_sort_array-2028499-45.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new15.38 KB
PASSED: [[SimpleTest]]: [MySQL] 57,860 pass(es).
[ View ]

Sorry for slow follow up, I've been away - failures were due to a PHPunit exception on the dataprovider method name, fixed in this latest patch.

Status:Needs review» Reviewed & tested by the community

This is perfect now, at least from my perspective.

+++ b/core/includes/common.incundefined
@@ -4315,14 +4323,16 @@ function element_info_property($type, $property_name, $default = NULL) {
-function drupal_sort_weight($a, $b) {
...
+function drupal_sort_weight(array $a, array $b) {

This change is a bit odd, but it feels like an acceptable api "change".

@dawehner is correct in #50 in that the only BC-breaking API change is that drupal_sort_weight() no longer accepts non-array values. Let's add that information to the summary, and also clarify that drupal_sort_weight() is deprecated, not removed. It is indeed very minor, but is it required? Why not just wrap single values in an array and avoid any BC break at all, if we're already deprecating it?

StatusFileSize
new557 bytes
new15.34 KB
PASSED: [[SimpleTest]]: [MySQL] 57,863 pass(es).
[ View ]

Yeah totally.

+1

Seems sensible to me, thanks @dawehner.

Issue summary:View changes

Updated issue summary.

Cool, so we can now call this an API addition. I updated the summary accordingly.

Issue summary:View changes

Updated issue summary.

Status:Reviewed & tested by the community» Fixed

Committed 40bc8bc and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

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

+++ b/core/lib/Drupal/Component/Utility/SortArray.php
@@ -0,0 +1,137 @@
+   * Sorts a structured array by the 'weight' element.
...
+  public static function sortByWeightElement(array $a, array $b) {
...
+   * Sorts a structured array by 'title' key (no # prefix).
...
+  public static function sortByTitleElement($a, $b) {
...
+   * Sorts a string array item by an arbitrary key.
...
+  public static function sortByKeyString($a, $b, $key) {
...
+   * Sorts an integer array item by an arbitrary key.
...
+  public static function sortByKeyInt($a, $b, $key) {

Please decide whether you want to call a key a key or an element, but not both (in the same class).

Issue summary:View changes

Updated issue summary.