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'));
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tsphethean’s picture

Status: Active » Needs review
FileSize
5.4 KB
tsphethean’s picture

Added newlines to end of new files.

Status: Needs review » Needs work

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

tsphethean’s picture

Status: Needs work » Needs review
FileSize
5.34 KB

Re-rolled.

Status: Needs review » Needs work

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

tsphethean’s picture

Status: Needs work » Needs review
FileSize
5.23 KB

and again...

Status: Needs review » Needs work

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

tsphethean’s picture

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.

tsphethean’s picture

Issue summary: View changes

Updated issue summary.

tsphethean’s picture

Issue summary: View changes

Updated issue summary.

fubhy’s picture

+++ 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.

fubhy’s picture

+++ 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.

dawehner’s picture

+++ 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?

tsphethean’s picture

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

tsphethean’s picture

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.

tsphethean’s picture

Status: Needs work » Needs review
FileSize
5.03 KB

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

tsphethean’s picture

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()

tayzlor’s picture

Status: Needs review » Reviewed & tested by the community

This looks pretty ok to me

chx’s picture

chx’s picture

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.

tsphethean’s picture

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

chx’s picture

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.

tsphethean’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
18.78 KB

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.

tayzlor’s picture

+++ 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) ?

tim.plunkett’s picture

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.

tsphethean’s picture

Status: Needs work » Needs review
FileSize
10.74 KB

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.

tsphethean’s picture

Status: Needs work » Needs review
FileSize
14.16 KB

#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.

tim.plunkett’s picture

Status: Needs review » Needs work

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

ParisLiakos’s picture

Issue tags: +PHPUnit Blocker

tagging

damiankloip’s picture

Status: Needs work » Needs review
FileSize
14.15 KB

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

dawehner’s picture

Status: Needs review » Needs work

We should mark all these methods as deprecated.

tsphethean’s picture

Status: Needs work » Needs review
FileSize
14.72 KB

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

dawehner’s picture

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.

tsphethean’s picture

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?

damiankloip’s picture

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 :)

chx’s picture

Issue tags: +sad chx

No answer to #21 so tagging and unfollowing

dawehner’s picture

Status: Needs review » Needs work

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

Needs work based on #37

tsphethean’s picture

Status: Needs work » Needs review
FileSize
16.03 KB

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?

damiankloip’s picture

FileSize
3.19 KB
15.33 KB

@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.

damiankloip’s picture

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'.

tsphethean’s picture

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.

tsphethean’s picture

Status: Needs work » Needs review
FileSize
15.37 KB

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.

tsphethean’s picture

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.

tsphethean’s picture

Status: Needs work » Needs review
FileSize
15.38 KB

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.

dawehner’s picture

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".

xjm’s picture

@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?

dawehner’s picture

FileSize
557 bytes
15.34 KB

Yeah totally.

tim.plunkett’s picture

+1

tsphethean’s picture

Seems sensible to me, thanks @dawehner.

tsphethean’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

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

xjm’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

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.

sun’s picture

+++ 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).

sun’s picture

Issue summary: View changes

Updated issue summary.