Follow up from #765860-18: drupal_alter() fails to order modules correctly in some cases. Although a feature request, I'm hoping it's accepted for D7, because it's cool, and I don't see any down side. Although hook_page_alter() is being used by core modules (which is neat) and will likely be used by contrib modules (which is also neat), I think its biggest awesomeness is what it will enable site builders to accomplish in site-specific modules and themes. And for these, I think the extra targeted hooks will be very useful.

One could claim that something similar could be useful with hook_(node|comment|user|*)_view_alter(), but that needs to be explored in a separate issue, because performance implications need to be evaluated (drupal_alter() static caching incurs more cache misses when passing additional targets). But the hook_page_alter() sequence runs just once per page request, so it's a cache miss anyway, and the extra calls to module_implements() are insignificant when only done once per page request, so it seems like a no-brainer.

CommentFileSizeAuthor
hook_page_alter-advanced.patch1.58 KBeffulgentsia
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

moshe weitzman’s picture

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

Terriific 1 line patch. Its great that we can reuse theme_get_suggestions(). Consistency++

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

subscribe

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Guys, at exactly what point does this stop? This is about the 3rd of 4th one of these patches you've tried to sneak in post-API freeze.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

IMO, this low-risk sort of patch stops at about RC1 or RC2. Alpha and Beta are ripe for patches like this (improve consistency, coherency, ...).

The real problem and anxiety is that we are not making good progress anymore on the criticals queue. Thats an entirely different problem from this sort of patch. Postponing low-risk improvements during Alpha is wrong IMO. If postponing could magically transfer developer attention to the criticals queue then sure. But alas attention doesn't work like that. We've already shut down HEAD for new features for about as long as its ever been shut down.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

See, to me this reads like one of those non-API breaking enhancements that would get committed to Drupal 8 and then back-ported to Drupal 7, which is something new we're doing this release. Of course, in order to get it committed to Drupal 8, Drupal 7 would need to first get released... ;)

Anyway, we'll see what Dries thinks.

Dries’s picture

Dries CreditAttribution: Dries commented
Version: 7.x-dev » 8.x-dev

I think this needs to go in Drupal 8 first. We can consider back-porting it later on.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

Um, if something is eligible for d7 after it is released then why would it be ineligible during alpha? that does not compute for me.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented

If the answer is to set (or continue) a precedent of increased strictness to (at least try to) focus both committer and developer attention on the criticals, then I can see the logic in that.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Lovely patch. Truly RTBC.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented

@sun: thx! in case you didn't notice, it got bumped to D8 (possibly to be ported to D7 after D7 is released). If you'd like it in D7 prior to D7 release, please help pitch why.

Lars Toomre’s picture

Lars Toomre CreditAttribution: Lars Toomre commented

Bump... Can this now be applied to D8 and D7 before D7.1 release?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue tags: +API addition, +Needs backport to D7

Tagging for backport, it's only an API addition.

catch’s picture

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

hook_page_alter-advanced.patch queued for re-testing.

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

The last submitted patch, hook_page_alter-advanced.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented 
+++ includes/common.inc	13 May 2010 05:31:15 -0000
@@ -4902,8 +4902,21 @@ function drupal_render_page($page) {
+  // - hook_page_node_%_alter()

mmm, apparently the most interesting case blows up...

> php -r "function foo_%_bar() { echo 'helo'; } foo_%_bar();"

Parse error: syntax error, unexpected '%', expecting '(' in Command line code on line 1

9 days to next Drupal core point release.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Issue summary: View changes
Status: Needs work » Fixed

We don't provide those global level alter hooks anylonger. yes!

Status: Fixed » Closed (fixed)

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