module_implements($hook, $sort = FALSE, $reset = FALSE) uses an internal cache (based on drupal_static_fast as well as cache_get()). The $sort argument is not used in the cache key, so when a cached result is returned, the sorting does not reflect $sort in the current call but its value when the cache entry was generated.

This patch makes module_implements() sort the module list itself rather than relying on the $sort argument for module_list(). I don't expect this to have a big performance impact. In core, module_implements() is only called with $sort == TRUE by help module (and even there this seems redundant, because the help module sorts the modules again). In D8 we may consider removing the $sort argument altogether.

CommentFileSizeAuthor
#20 module-implements-sort-9.patch8.37 KBc960657
#17 module-implements-sort-8.patch8.35 KBc960657
#16 module-implements-sort-7.patch8.13 KBc960657
#12 module-implements-sort-6.patch8.22 KBc960657
#6 module-implements-sort-5.patch11.88 KBc960657
#4 module-implements-sort-4.patch963 bytesc960657
#3 module-implements-sort-3.patch2.2 KBc960657
#2 module-implements-sort-2.patch1.96 KBc960657
module-implements-sort-1.patch1.89 KBc960657
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

c960657’s picture

c960657 CreditAttribution: c960657 commented

This patch also fixed the bug/oddity that hook_module_implements_alter() is invoked even if when $sort == TRUE, i.e. when the caller requests the modules to be returned in alphabetical order.

c960657’s picture

c960657 CreditAttribution: c960657 commented
FileSize
module-implements-sort-2.patch1.96 KB

Reroll.

c960657’s picture

c960657 CreditAttribution: c960657 commented
FileSize
module-implements-sort-3.patch2.2 KB

Reroll.

c960657’s picture

c960657 CreditAttribution: c960657 commented
Version: 7.x-dev » 8.x-dev
FileSize
module-implements-sort-4.patch963 bytes
mikl’s picture

mikl CreditAttribution: mikl commented

Looks good to me. I think this should be merged for D7 (as it fixes a bug in the API).

For D8, however, I would be more in favour of removing the $sort option. It's apparently not widely used (as this bug has gone unnoticed), and there's no advantage to doing sort() in module_implements() rather than the calling code.

c960657’s picture

c960657 CreditAttribution: c960657 commented
FileSize
module-implements-sort-5.patch11.88 KB

Good point. I agree with mikl that $sort is redundant. The function was called with $sort == TRUE twice in core (in the Help module), but in both cases the sorting was irrelevant.

This patch removed the $sort argument from module_implements() as well as module_list().

mikl’s picture

mikl CreditAttribution: mikl commented
Status: Needs review » Reviewed & tested by the community

+1 for cleaner APIs. Works as advertised.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue tags: +Framework Initiative, +Needs backport to D7

Good catch. Looks ready to fly for me.

c960657’s picture

c960657 CreditAttribution: c960657 commented

The patch in #4 applies to D7.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue tags: -Framework Initiative, -Needs backport to D7

#6: module-implements-sort-5.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Framework Initiative, +Needs backport to D7

The last submitted patch, module-implements-sort-5.patch, failed testing.

c960657’s picture

c960657 CreditAttribution: c960657 commented
Status: Needs work » Needs review
FileSize
module-implements-sort-6.patch8.22 KB

Reroll (due to #577950: API-cleanup: Factor module_implements $refresh parameter into it's own function).

Status: Needs review » Needs work

The last submitted patch, module-implements-sort-6.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Based on the patch size, looks like you missed a hunk?

c960657’s picture

c960657 CreditAttribution: c960657 commented

I hope not. But some of the changes were made redundant by the introduction of the new module_implements_reset() in #577950: API-cleanup: Factor module_implements $refresh parameter into it's own function.

c960657’s picture

c960657 CreditAttribution: c960657 commented
Status: Needs work » Needs review
FileSize
module-implements-sort-7.patch8.13 KB

Updated D8 patch. The patch in #4 applies to D7.

c960657’s picture

c960657 CreditAttribution: c960657 commented
FileSize
module-implements-sort-8.patch8.35 KB

Reroll after #22336: Move all core Drupal files under a /core folder to improve usability and upgrades.

c960657’s picture

c960657 CreditAttribution: c960657 commented
Issue tags: -Framework Initiative, -Needs backport to D7

#17: module-implements-sort-8.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +Needs backport to D7

The last submitted patch, module-implements-sort-8.patch, failed testing.

c960657’s picture

c960657 CreditAttribution: c960657 commented
Status: Needs work » Needs review
FileSize
module-implements-sort-9.patch8.37 KB

Reroll.

bojanz’s picture

bojanz CreditAttribution: bojanz commented

Sorry, it seems I've just duplicated some of your work (I blame the airplane non-connectivity).
I'm trying to fix the disabled modules situation and ran into the absolute mess that module_list() is.
Provided #1503224: Cleanup module_list() that tries to do a more complete cleanup, removes the $sort parameter like your patch does, and addresses some additional points.
So if the approach I took looks fine, maybe we can continue there? We'll add a note to the maintainers that you should get commit credit as well.

c960657’s picture

c960657 CreditAttribution: c960657 commented
Status: Needs review » Closed (duplicate)

Sure, let's continue in #1503224: Cleanup module_list().