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
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'));
Comment | File | Size | Author |
---|---|---|---|
#52 | drupal-2028499-52.patch | 15.34 KB | dawehner |
#52 | interdiff.txt | 557 bytes | dawehner |
#49 | drupal_sort_array-2028499-49.patch | 15.38 KB | tsphethean |
#45 | drupal_sort_array-2028499-45.patch | 15.37 KB | tsphethean |
#41 | 2028499-40.patch | 15.33 KB | damiankloip |
Comments
Comment #1
tsphethean CreditAttribution: tsphethean commentedComment #2
tsphethean CreditAttribution: tsphethean commentedAdded newlines to end of new files.
Comment #4
tsphethean CreditAttribution: tsphethean commentedRe-rolled.
Comment #6
tsphethean CreditAttribution: tsphethean commentedand again...
Comment #8
tsphethean CreditAttribution: tsphethean commented#6: drupal_sort_weight-2028499-6.patch queued for re-testing.
The failing test is passing on my local, so re-queueing.
Comment #8.0
tsphethean CreditAttribution: tsphethean commentedUpdated issue summary.
Comment #8.1
tsphethean CreditAttribution: tsphethean commentedUpdated issue summary.
Comment #9
fubhy CreditAttribution: fubhy commentedEach line should be a maximum of 80 characters wide.
We gotta fix that in the places where we are using it. Otherwise we can never deprecate drupal_sort_weight().
Same here (80 characters max.)
Missing whitespace in front of "The". Also, the return type is "int".
Remove these whitspaces after the @see's.
Comment #10
fubhy CreditAttribution: fubhy commentedNeeds leading backslashes.
Comment #11
dawehnerThe @see should be more linking to the sort array not check plain.
It feels like having === would make more sense here?
Comment #12
tsphethean CreditAttribution: tsphethean commented@fubhy - thanks for the comments. New patch attached to address the issues.
Comment #13
tsphethean CreditAttribution: tsphethean commentedAnd another patch to address the comments I missed!
Comment #15
tsphethean CreditAttribution: tsphethean commentedRevised @deprecated line as I heard another issue get rejected because version was missing.
Comment #16
tsphethean CreditAttribution: tsphethean commentedAlso 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()
Comment #17
tayzlor CreditAttribution: tayzlor commentedThis looks pretty ok to me
Comment #18
chx CreditAttribution: chx commentedhttps://github.com/Herzult/SimplePHPEasyPlus
Comment #19
chx CreditAttribution: chx commentedThis 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.
Comment #20
tsphethean CreditAttribution: tsphethean commentedFollowing discussion on IRC, drupal_sort_weight will no longer be deprecated in this patch.
Comment #21
chx CreditAttribution: chx commentedmsonnabaum 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.
Comment #22
tsphethean CreditAttribution: tsphethean commentedRevised 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.
Comment #23
tayzlor CreditAttribution: tayzlor commenteddont think the Json stuff should be in here....
Should this use Use at the top of the file and just reference SortArray (and in the other examples) ?
Comment #24
tim.plunkettuse statements have no bearing on string references to classes, the full classname is required for callbacks like uasort
Comment #26
tsphethean CreditAttribution: tsphethean commentedOk, 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.
Comment #28
tsphethean CreditAttribution: tsphethean commented#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.
Comment #29
tim.plunkett#28: drupal_sort_weight-2028499-28.patch queued for re-testing.
Comment #31
ParisLiakos CreditAttribution: ParisLiakos commentedtagging
Comment #32
damiankloip CreditAttribution: damiankloip commentedjust a reroll, let's get this moving again. It's been a couple of weeks.
Comment #33
dawehnerWe should mark all these methods as deprecated.
Comment #34
tsphethean CreditAttribution: tsphethean commentedAdded deprecated statements and also added @return docblock statements for the deprecated functions for completeness.
Comment #35
dawehnerThis adds test coverage for functions, which haven't been tested before.
A potential follow up could be to introduce a method sortByKey($a, $b, $key = 'weight') for example
Missings types (should be array afaik)
Impressive test coverage! Let's put a dot at the end of all the docs.
Comment #36
tsphethean CreditAttribution: tsphethean commentedThanks @dawehner
Done
Done
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?
Comment #37
damiankloip CreditAttribution: damiankloip commentedI 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 :)
Comment #38
chx CreditAttribution: chx commentedNo answer to #21 so tagging and unfollowing
Comment #39
dawehnerWell, unfollowing so he will not care about a potential comment?
Needs work based on #37
Comment #40
tsphethean CreditAttribution: tsphethean commentedSlightly 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?
Comment #41
damiankloip CreditAttribution: damiankloip commented@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... :)
Comment #43
damiankloip CreditAttribution: damiankloip commentedheh, but probably without the array type hints on those new methods. You get what I mean though?
We should prefix these data provider methods with provider. So 'providerSortByTitleProperty'.
Comment #44
tsphethean CreditAttribution: tsphethean commentedYep, 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.
Comment #45
tsphethean CreditAttribution: tsphethean commentedHopefully this will do the trick. Thanks for the pointers @damiankloip
Comment #47
tsphethean CreditAttribution: tsphethean commented#45: drupal_sort_array-2028499-45.patch queued for re-testing.
Comment #49
tsphethean CreditAttribution: tsphethean commentedSorry 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.
Comment #50
dawehnerThis is perfect now, at least from my perspective.
This change is a bit odd, but it feels like an acceptable api "change".
Comment #51
xjm@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 thatdrupal_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?Comment #52
dawehnerYeah totally.
Comment #53
tim.plunkett+1
Comment #54
tsphethean CreditAttribution: tsphethean commentedSeems sensible to me, thanks @dawehner.
Comment #54.0
tsphethean CreditAttribution: tsphethean commentedUpdated issue summary.
Comment #55
xjmCool, so we can now call this an API addition. I updated the summary accordingly.
Comment #55.0
xjmUpdated issue summary.
Comment #56
alexpottCommitted 40bc8bc and pushed to 8.x. Thanks!
Comment #58
sunPlease decide whether you want to call a key a key or an element, but not both (in the same class).
Comment #58.0
sunUpdated issue summary.