Allow themes to alter forms and page

Pushing to critical as it's required for themes to do things like [theme]_page_alter, [theme]_css_alter and [theme]_js_alter. Hitting the subscribe button so that I can take a look at this when I'm not as tired.

The last submitted patch failed testing.

The last submitted patch failed testing.

Release blocker due to discussion in #583400: Allow jQuery UI themes to be swappable. In summary, when adding a jQuery UI element to the page, we need ui.theme.css. jQuery UI allows you to swap out different ui.theme.css files to give the elements a different look. We can switch this jQuery UI theme through hook_library_alter(), but that's only accessible from modules. Themes have to have the ability to swap out ui.theme.css, and [theme]_library_alter() will allow us to do just that.

Makes sense to me, I can't count the number of times someone showed up in #drupal asking about theming a form, and the recommendation was to make a custom module implementing hook_form_alter().

We just made module_implements() a lot less expensive, so this shouldn't introduce too much overhead either.

All critical bugs are release blockers, so removing tag though.

The previous version broke admin theme. While ths patch should pass it's not ideal as it only allows altering from theme once the theme is initialized. One solution can be initializing theme once hook_init has run - so that admin still works.

Though the code doesn't look like a hack, this *feels* like one. I don't have an alternative patch to offer but I think rather than allowing some hooks we should be stepping back and thinking about a way to keep hooks in modules as an artifact of the sudo-separate layers between modules and themes.

I don't fully grok the base theme stuff, but yes, this does allow themes access only to alters. However, there are a LOT of alters now. :-) This would include large parts of views, menus... It would give themes access to hook_query_alter(). There's a lot more here than just form alter.

Is that really wise?

As a person who spends 98% of their time in the theme, this has me really, really excited. At the same time, it scares the bejesus out of me. I can see all sorts of possibilities with a patch like this like cleanly reordering CSS and JS files for one. At the same time, this can really bork a site if a theme changes something a module depends on. (Not that this cannot be done now with Drupal's current setup.)

I would have to say that this currently shows more positive to me at this time than negative. There would certainly have to be some sort of best practice document for this to discourage use of it unless it is absolutely necessary.

We already give themes access to every possible PHP function, so the fact that we leave open the possibility to implement an _alter() hook which isn't about forms doesn't seem so bad to me - if you do something stupid in a theme, then you've done something stupid in your theme. Someone also showed me a hook_menu_alter() which took every single menu title and "Made it Look Like This" - which is a cosmetic change only, and potentially something which might need to be theme specific. That's a horrible example, but hook_page_alter() is a more valid use-case. It'd also take more code, and code in the critical path too, to exclude certain hooks here.

This version initializes theme in bootstrap we discussed with catch and there are very very few pages which have not called this before -- only if your page had zero theme calls. One t() with a % and there you go... I have moved the most important parts into module_implements so it gets cached and added a test.

The last submitted patch failed testing.

Tough call. The benefits are impressive and obvious.

The risk is big too. Drupal imposes a healthy workflow for your site. Namely, it separates your functionality from your presentation. This is a healthy separation not only for the code but also for the workflow of you team. Each side can work mostly independantly. There is a chance that this patch obliterates this separation. After this patch, themes can implement 90% of what modules want to do.

x-post

Moshe, I would like to point out that we are allowing only alters. Yes that's significant power but it's a deliberate choice to keep some of the barrier alive.

let's see whtehr this passes.

The last submitted patch failed testing.

I don't know that I'm really convinced. From a experience side, I don't think its that hard a workflow to add a module or use the following idea in a template or theme function is really all that bad.

From a conceptual side, form /building/ is a very model oriented task, if we're providing too much data consistently to the themes, I don't think its their job to manage that. If anything we've got a problem with our tools but I'm not sure of that either.

From a code perspective, a special case of module_implements where it doesn't return a module? yeah... how many negative signs can I attach to that? I do like some of the other things like having a point you can consistently assume theme initialization but... I think that's a completely different issue.

We already give themes access to every possible PHP function, so the fact that we leave open the possibility to implement an _alter() hook which isn't about forms doesn't seem so bad to me - if you do something stupid in a theme, then you've done something stupid in your theme. Someone also showed me a hook_menu_alter() which took every single menu title and "Made it Look Like This" - which is a cosmetic change only, and potentially something which might need to be theme specific. That's a horrible example, but hook_page_alter() is a more valid use-case. It'd also take more code, and code in the critical path too, to exclude certain hooks here.

I totally agree with catch on this. Why not give themes access to the tools to do small things like this the proper way? You can misuse Drupal everywhere if you want to. But that is up to the developer. Of course you should not do heavy form altering in a theme. But you shall not do SQL queries in a theme either. And right now we have full access to the database layer on the theme level.

Of course you should not do heavy form altering in a theme. But you shall not do SQL queries in a theme either.

The difference with this issue is we're explicitly saying, "you can use these hooks, but don't do too much" whereas before you *could* invoke any Drupal function available you were told you should not.

There are already some base themes that add module-like functionality. The ability to add the edit links above blocks and menus that Zen and a few others add could easily be a module.

90% of the form_alter functions I have written are to re-order fields, change their labels and help text, insert some markup, etc. That feels like a front-end task to me, but I had to keep writing site-specific modules to do it.

Encouraging best practices, or putting limitations on theme developers?

Then I see examples from the Wordpress community where a theme changes the functionality of a site (see the K2 theme), and I realize that being able to change Views and functionality-ish things from inside a theme wouldn't be so bad either, if the power was put in capable hands. K2 adds AJAX-y stuff to pagers and search results that is built in to things like Views, but may not be desired on every site. Being able to make that kind of a change in a theme would be awesome.

I've seen situations where if they couldn't alter it in the theme layer, they'd stick in some really grows SQL to get around it. If we don't give themes the ability to do things the "right" way, then they will end up doing it some way, which has bound to be the wrong way. Yes, writing another module would be the way around this, but sometimes you only want an alter on a given theme. This patch fixes that. Besides that point, we still need this for hook_page_alter, hook_css_alter, hook_js_alter and hook_library_alter.

I was asked to comment, so taking sides with catch *and* Moshe. The possibilities are exciting, but it sounds a bit too far reaching. I'm excited but not comfortable.

I really like this patch, because I stopped counting the number of sites where I had to write a totally senseless module just to be able to alter something. We shouldn't force themers to do so, especially, since we allow more and more Drupal sites to run without any module-based customizations at all.

This should run after hook_init(), please. We introduced menu_set_custom_theme(), but that is bound to the menu system, and we still want certain modules to be able to alter the theme in hook_init(). So, just move it some lines down.

Why do we limit to alter hooks? Unless there is a very, very compelling reason to do so, which doesn't start with "Because we assume that anything else...", then please remove that (fake) limitation.

It looks like we could prepare this once for all calls to increase performance.

This really should live in system.test or even module.test.

We need a corresponding hook then, and, hah, we're lucky: #593522: Upgrade drupal_alter() introduces one in system_test.module. :)

Lastly, there must be some code somewhere that allows hook_theme() in themes already -- I guess that can vanish then? :)

This review is powered by Dreditor.

uhm, yeah, "It looks like we could prepare this once for all calls to increase performance." doesn't make sense here, because we usually append only 1 element.

A few notes: you can't just remove _alter protection because there are theme functions and hooks of the same name, hook_block in forefront. So far, noone came up with theme_boo_alter and that seems inappropriate anyways. The speed difference between preparing an 1-3 size array into a static and merging in vs iterating the same with foreach is absolutely negligible. And it's cached anyways :)

I support this patch. On the whole, it does more good than harm.

Further discussion in IRC revealed that alter hooks alone are mighty enough to alter the heck out of your Drupal site. hook_system_info_alter() allows your theme to also become a module. Stuff like that. This sounds like "let's just throw some bad-ass APIs on them and hope that they won't find out"...

That's why I call it fake limitation. Actually, there is a sheer amount of possibilities here:

- Themes can implement theme-related Features.

- Themes can implement Panels layouts.

- Themes can alter JS, CSS, make the output HTML5 compliant, etc.

...

Stuff like that. Stuff that is totally wonky to code a module for, because it's all theme-related.

I resign. We will limit to alter hooks and chx will make sure to document inline why. :)

Just chiming in to support this patch.

Having the Drupal theme initialization so early on caused some white screens at irregular intervals. This patch moves the code from module_implements back into drupal_alter(), and only does it on selective theme-related alters:

• hook_page_alter
• hook_js_alter
• hook_library_alter
• hook_css_alter
• hook_form_alter

It also moves the test into common.test as drupal_alter() is in common.inc (thanks for the note, Sun). This should also pass on the testing bot front.

Catch brought up the idea of only doing the alter when the bootstrap is complete, so here it is. I was getting a bunch of exceptions during my tests, but it was passing....

The last submitted patch failed testing.

Hmm, 3 exceptions? Let's try it with the bootstrap check instead.

The last submitted patch failed testing.

Yeah, see? Exposing the alteration on selected theme-related alters is a bit safer....

The last submitted patch failed testing.

I'm still "not comfortable" as Gábor put it but I think Rob's recent patch it a much better approach.

+ case 'form_' . substr($type, 5): I presume this wants to be the form specific form_alter? Looks OK -- why you do not allow the generic form alter though?

Also, these picks are highly arbitrary. I know this looks nice and dandy but what if I want, say, hook_translated_menu_link to add a class to menu options? Heck, I even want to do hook_menu_alter or hook_menu_link_alter for the same. Who knows what contrib alter comes along? Of ocurse, if this is what the community wants then I am fine with this approach but I would rather not limit ourselves to any set.

I agree with chx. I would even skip the limitation to alter hooks, but it seems like I'm the only one who is comfortable with that. So let's go with all alter hooks.

I also disagree with a whitelist. That just cuts out all of contrib. Lets let themes participate in any alter hook.

#592008: Don't save theme registry before modules are included is required to make this work.

I'm on crack. Are you, too?

Revised implementation after drupal_alter() has been upgraded. :)

The last submitted patch failed testing.

Sorry, wrong check.

Apparently, $theme is only not initialized in one of a gazillion of alters during a single request.

Code looks good. Bots happy.

Much nicer. There's going to be minimal overhead including themes in drupal_alter() itself, and it feels happier than having the code in module_implements().

The new global $base_theme is a dupe of the existing $base_theme_info. Also, base themes need to alter things before sub-themes.

Re-rolling now…

Compared to tha_sun's patch in #47, this patch:

1. Reuses the existing global $base_theme_info variable to get the base theme keys
2. Puts sub-theme alterations after base theme alterations
3. Fixes the unintentional bug of the global $theme variable getting changed on each loop of the foreach ($themes as $theme)
4. Prevents a problem in _drupal_maintenance_theme where the maintenance theme triggers a hook_system_info_alter() on the theme .info data after global $theme was set but before $base_theme_info was; which meant an incomplete drupal_alter experience for the theme. _drupal_maintenance_theme() calls list_themes calls _system_rebuild_theme_data() calls drupal_alter().

Awesome job!

IMO, this is pretty much dependent on #371375: Never name a module and a theme the same name!

+1 for #53

The problem described in #371375: Never name a module and a theme the same name! exists regardless of whether this patch is committed now, later, or not at all. This issue will make that issue more critical since all the alter functions in a module named the same as the active theme will be run twice, but there is no "dependency" between the issues.

In the strictest sense, JohnAlbin is correct, since this patch would 'function' relatively as intended without the other. However, since this patch will cause more bugs without the other, I consider them essentially paired. (And I do believe that namespace conflicts between modules and themes is a bug.)

I've debated internally about this patch for a long time, as there are strong arguments to be made on both sides of keeping the 'purity' of not mixing a site's business logic with presentation, but on the other hand blocking theme developers from leveraging the full power of the render API unless they learn module development.

I had a look in the big-ass "alter everything" module we built for Buzzr for some guidance on this point, since this module tests the very outer limits of what Drupal 6's alterations can do. Here are common things we do in there, which are all clearly "business" logic and clearly belong to the module:

hook_menu_alter() to change who has access to a given path.
hook_menu_alter() to change the destination a path takes you to when you click on it.
hook_form_alter() to add custom validation and submission routines.
hook_form_alter() to add additional elements to the form.

However, all of this code is sitting alongside other stuff like this, which are all clearly "presentation" logic:

hook_menu_alter() to change a menu item to MENU_CALLBACK to hide it from display.
hook_menu_alter() to change menu titles here and there.
hook_form_alter() to hide form fields.
hook_form_alter() to re-weight fields.
hook_form_alter() to rename buttons.
hook_form_alter() to add wrapper divs around elements.

So in this respect, this patch would actually help make the separation between presentation and business logic cleaner, by allowing us to stick all of the first list in customtheme_XXX_alter() and the rest in custommodule_XXX_alter().

While it's true that this is open to abuse, such as hook_query_alter(), I honestly can't imagine any themer going that far. If you get to the point where you even know about hook_query_alter(), you are likely neck-deep into module development anyway.

Since this lack of separation seems to be the biggest argument not to commit this patch, and I believe there are more pros than cons in this approach, I have committed it to HEAD. May themers bask in the glory of hook_page/form/menu/..._alter(). :)

Please document this in the theme upgrade guide.

I'm still not really happy with this idea and I'd like to comment on webchick commit post.

hook_form_alter() to hide form fields.
hook_form_alter() to re-weight fields.
hook_form_alter() to rename buttons.
hook_form_alter() to add wrapper divs around elements.

As I've mentioned, these already can and probably on individual sites should be handled in the theme layer. Though, buttons maybe not if you actually meant submit because of the way html works. We all remember looking for $_POST['op'] = t('Foo') because submit buttons submit the display value which is unfortunate. So that leaves menu... maybe that's an API flaw in the menu system? Likely.

Anyways, I don't think that because some people make a common practice of putting display logic in their business logic, we should expose the control layer to the theme. We should be looking at the reasons they need to and address those points.

.

Exciting and terrifying all at the same time. An unfortunate side effect, however, is that it more than doubled the time taken by drupal_alter(), and some alter hooks get called a lot. This patch restores drupal_alter() to being fast again.

If you could only use one word to explain the awesomeness of that patch, it would be "pimp". Nicely done.

Actually, I think this one's better. CNR for someone to confirm that opinion.

Looks much cleaner.

Could you provide a quick set of before/after benchmarks?

module_implements() already has a cache. This cache proliferation is getting sickening. We should back this out, if we can't fix this with the caches we have. I would be sad too - this is a nice feature. My .02

Looping through $theme and all of its ancestors and doing a function_exists() on alter hooks like hook_query_alter(), hook_file_url_alter(), hook_username_alter(), and others, every invocation is wasteful, especially if 99+% of sites will be using themes that don't implement these hooks. I'll work on benchmarks later today or tomorrow. I'd be fine with going back to earlier versions of this issue, where module_implements() returns an array that includes themes if we want to leverage its cache. Otherwise, repeatedly called processing should be cached.

Status change was unintentional (though given #65, perhaps appropriate, but I'll leave that to others to decide).

The "expensive" looping you mention is exactly what every version of drupal has done. And adding themes into the mix should add about 2 more functions to check, if site is using a subtheme. Thanks for the benchmarks - those really do ground the debate.

I get the cache proliferation argument, but erm, this is a special case? module_implements() shouldn't return theme implementations, because we only want them to be able to implement _alter hooks, so clearly, only drupal_alter() is able to prepare and cache that info.

(Although I didn't get that module_implements() argument, because we already need to support hook_theme() for themes... so the argument is flawed.)

superscribe

In response to #68, the difference between prior versions of drupal and D7, is that D7 proliferates the use of drupal_alter(), and I think that's a good thing. It's an awesome idiom. IMO, the three most important Drupal functions are module_invoke_all(), theme(), and drupal_alter(). Perhaps drupal_render() should get added to that list. We want to encourage module authors to use these wherever it makes sense to. We don't want module authors hesitating to use these functions due to performance concerns, because that means they'll be writing modules that don't integrate into Drupal the correct way. And we don't want module authors feeling like they have to implement their own versions of these functions or their own static caching just to compensate for these functions being any slower than they need to be. We've already seen examples of this in the url() function with respect to drupal_alter('url_outbound'). Maybe, one could say that url() is such an important performance function, that it deserves a static variable circumventing the use of drupal_alter() no matter how fast drupal_alter() is, but do we want to see that same hackery to get around drupal_alter('query'), drupal_alter('username'), or drupal_alter('file_url')? Or would we rather just make drupal_alter() fast?

This list might not be complete, but here's some alter hooks that get called many times per page request:

• query
• url_outbound
• file_url
• menu_contextual_links
• block_view
• username
• comment_build
• field_attach_view

I micro-benchmarked how long it takes to call drupal_alter(), and with the Minnelli theme (so we're testing a theme that has a base theme, since many sites will use a sub-theme of Zen), on my computer (2GHz Core 2 Duo Macbook running MAMP), the implementation in HEAD takes 13us. For reference, calling a function that does nothing and has no arguments in its signature takes 0.6us, and calling a function that does nothing and has 4 arguments in its signature takes 1.2us. However, with patch 62, drupal_alter() takes 5us and if we can get #619666: Introduce new pattern for drupal_static() in performance-critical functions to land, this can be reduced to 3us. The difference between 13us and 3us is the difference between calling the use of the above alter hooks into question vs. not. And it's the difference between module authors adding a drupal_alter() statement where it makes logical sense to vs. worrying about what the performance impact of doing so would be.

To take an example, I created a node with 50 comments (see #613452: Generating content with comments fails), gave the anonymous role "access comments" permission, and set the site theme to Minnelli. Viewing this node as the anonymous user results in 285 calls to drupal_alter(). Multiplying this by the difference between 13us and 5us results in a predicted savings of 2.3ms. However, I only saw an actual savings of 1.8ms (apache benchmarks attached). I'm not entirely sure why the difference between predicted and actual savings; perhaps it's because the savings only occurs on each alter hook after the first one, and the first use of each alter hook results in a small overhead. Perhaps it's because some alter hooks run before the theme system is initialized, and for those, the savings isn't as big. I haven't been able to quantify this, because it's hard to get profile information for how many times each unique alter hook is called. But anyway, this still represents a savings of 0.8% for this page.

I'm not particularly excited about a 0.8% savings for a node with 50 comments. However, I'm very interested in making drupal_alter() as fast as possible for module authors to use liberally.

[Edit: text deleted cause it was meant for a different issue. ignore this comment and attachments]

Sorry, ignore #72. That was meant for a different issue. Re-uploading patch 62.

@Moshe, Drupal 6 does indeed loop through all module_hook() invocations all the time - berdir profiled a D6 site with a lot of contrib modules, and found module_implements() + module_hook() to take around 10% of the request time, most of that looking for hook invocations which will never exist.

If you compare Drupal 6 to Drupal 7, same page, you can see how many more hook invocations we have between versions, it's approaching an order of magnitude: - 18 calls in D6, 163 calls in D7. http://drupal.org/files/issues/6_0.png http://drupal.org/files/issues/7_0.png

We call drupal_alter() on every built query, on every link (apart from some hacky special-case caching committed earlier this week), we de-opped every hook, we added a lot of brand new hooks too, and here we let more things implement hooks. Multiply that pattern across contrib and that puts our core routing functions under a lot of strain, so if we can optimize them, then we should, and that should result in less special case hacks like #619566: Don't invoke hook_url_outbound_alter() if there's nothing to invoke, not more.

@efflugentsia and catch are terrific explainers in addition to their coding prowess. a proper optimization IMO.

Agreed; nicely done, folks!

Committed #73 to HEAD. I believe this is still needs work for docs.

This is indeed an awesome solution. Thanks, folks! :)

I don't yet see anything about this in the docs. If somebody can point me to some demonstration code, I'll try to get it into the theme upgrade guide.

I don't think there's any demonstration code, it'd just be

@catch, thanks. I assume that means that a theme can now alter anything, so theme_*_alter, including hook_page_alter()? And they all work just like hook_form_alter.

Yep.

I added a short summary of this to the theme update page.

If one of you more familiar with the issue could check it and (probably) add more info to it, it would be appreciated.

Thanks, rfay! Documentation is very helpful. This definitely gives themes a lot of power, and it's good to let people know about it on the themes update page. Looks good!

I was talking with Jacine at the DrupalNYC Camp pre-party and she said the documentation wasn't quite up to par, so I touched it up a bit, in hopes that it will give themers a closer idea as to how to use it.

Awesome Rob! This is MUCH better.

Thank you :)

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