Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobLoach’s picture

Status: Needs work » Needs review

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.

Priority: Normal » Critical
Status: Needs review » Needs work
Issue tags: +DX (Developer Experience)

The last submitted patch failed testing.

chx’s picture

FileSize
3.04 KB

Status: Needs review » Needs work

The last submitted patch failed testing.

RobLoach’s picture

Category: feature » bug
Status: Needs work » Needs review
Issue tags: +Release blocker

Release blocker due to discussion in #583400: jQuery UI libraries are loading too much CSS that needs to be reverted / 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.

catch’s picture

Title: Allows themes to alter forms and page » Allow themes to alter forms and page
Issue tags: -Release blocker

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.

chx’s picture

FileSize
3.05 KB

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.

coltrane’s picture

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.

Crell’s picture

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?

Zarabadoo’s picture

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.

catch’s picture

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.

chx’s picture

FileSize
5.17 KB

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

moshe weitzman’s picture

Status: Needs work » Needs review

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.

moshe weitzman’s picture

Status: Needs review » Needs work

x-post

chx’s picture

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.

chx’s picture

Status: Needs work » Needs review
FileSize
5.24 KB

let's see whtehr this passes.

Status: Needs review » Needs work

The last submitted patch failed testing.

neclimdul’s picture

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.

drupal_render($form['element_we_dont_like']);
$form['element_to_change']['#prefix'] = 'I totally altered this';
print(drupal_render($form);

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.

dixon_’s picture

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.

coltrane’s picture

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.

liberatr’s picture

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.

RobLoach’s picture

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.

Gábor Hojtsy’s picture

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.

sun’s picture

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.

+++ includes/common.inc	2009-10-01 01:38:12 +0000
@@ -3922,6 +3922,9 @@ function _drupal_bootstrap_full() {
   menu_set_custom_theme();
+  if (!defined('MAINTENANCE_MODE') && !_menu_site_is_offline()) {
+    drupal_theme_initialize();
+  }
   // Let all modules take action before menu system handles the request
   // We do not want this while running update.php.
   if (!defined('MAINTENANCE_MODE') || MAINTENANCE_MODE != 'update') {

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.

+++ includes/module.inc	2009-09-30 16:26:45 +0000
@@ -378,6 +379,12 @@ function module_implements($hook, $sort 
+    if ($theme && substr($hook, -6) == '_alter') {

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.

+++ includes/module.inc	2009-09-30 16:26:45 +0000
@@ -378,6 +379,12 @@ function module_implements($hook, $sort 
+      $list[] = $theme;
+      foreach ($base_theme as $base) {
+        $list[] = $base->name;
+      }

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

+++ modules/simpletest/tests/form.test	2009-09-30 16:00:05 +0000
@@ -363,6 +363,33 @@ class FormsFormCleanIdFunctionalTest ext
+class FormsAlterThemeTest extends DrupalWebTestCase {
+
+  public static function getInfo() {
+    return array(
+      'name' => 'hook_form_alter by themes test',
+      'description' => 'Test the capability of themes to alter forms.',
+      'group' => 'Form API',
+    );
+  }

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.

sun’s picture

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.

chx’s picture

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

moshe weitzman’s picture

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

sun’s picture

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.

sun’s picture

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

Jacine’s picture

Just chiming in to support this patch.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
6.06 KB

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.

RobLoach’s picture

FileSize
5.47 KB

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

Status: Needs review » Needs work

The last submitted patch failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
5.47 KB

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

Status: Needs review » Needs work

The last submitted patch failed testing.

RobLoach’s picture

FileSize
6.06 KB

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

RobLoach’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

neclimdul’s picture

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

chx’s picture

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

sun’s picture

Issue tags: +D7 API clean-up

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.

moshe weitzman’s picture

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

sun’s picture

+++ includes/common.inc	9 Oct 2009 07:19:06 -0000
@@ -4254,6 +4254,30 @@ function drupal_alter($type, &$data) {
+  // Expose selected alterations to the theme layer. Only the following
+  // alterations are applied as others are called too early for the theme layer
+  // to be initialized.

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

I'm on crack. Are you, too?

sun’s picture

Status: Needs work » Needs review
FileSize
6.13 KB

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

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
6.09 KB

Sorry, wrong check.

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

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Code looks good. Bots happy.

catch’s picture

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

JohnAlbin’s picture

Assigned: chx » Unassigned
Status: Reviewed & tested by the community » Needs work

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…

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
4.67 KB

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().
sun’s picture

Status: Needs review » Reviewed & tested by the community

Awesome job!

matt2000’s picture

Ryan Palmer’s picture

+1 for #53

JohnAlbin’s picture

The problem described in #371375: Do not allow a module and theme to use 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.

matt2000’s picture

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

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs documentation

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.

neclimdul’s picture

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.

sun’s picture

Issue tags: -D7 API clean-up

.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
4.11 KB

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.

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

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

effulgentsia’s picture

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

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

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

Looks much cleaner.

webchick’s picture

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

moshe weitzman’s picture

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

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review

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.

effulgentsia’s picture

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

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

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.

sun’s picture

Status: Reviewed & tested by the community » Needs review

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

bleen’s picture

superscribe

effulgentsia’s picture

Issue tags: +Performance
FileSize
2.15 KB

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: Make performance-critical usage of drupal_static() grokkable 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.

effulgentsia’s picture

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

effulgentsia’s picture

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

catch’s picture

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

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Agreed; nicely done, folks!

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

sun’s picture

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

rfay’s picture

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.

catch’s picture

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

mytheme_form_alter(&$form, &$form_state, $form_id) {
}
rfay’s picture

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

catch’s picture

Yep.

rfay’s picture

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.

RobLoach’s picture

Status: Needs work » Fixed

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!

RobLoach’s picture

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.

Jacine’s picture

Awesome Rob! This is MUCH better.

Thank you :)

Status: Fixed » Closed (fixed)

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

amitaibu’s picture

Priority: Critical » Normal
Status: Closed (fixed) » Active

I have noticed a regression when I tried to implement function hook_system_info_alter().

With chx's suggestion I've debugged drupal_alter() and indeed noticed that $theme is uninitialized.

chx’s picture

We have lazy initialized theme in Drupal 6 and with rendered arrays theme() is not called until the theming phase so this patch is nil and void. What should we do? Kill lazy init by init'ing in drupal_alter?

amitaibu’s picture

As a "compromise", maybe we can init theme, only after cache clear?

catch’s picture

Which cache would get cleared? drupal_alter() only caches statically.

webchick’s picture

Can we please discuss this in a separate issue (cross-posted here) and not re-open this monster?

sun’s picture

I think this is the right issue, since that regression could easily turn out that we need to roll back this feature. Furthermore, the right folks are on this issue already.

AFAICS, we already added drupal_static_reset()s to module_implements() and drupal_theme_initialize() to accomodate for the fact that themes are lazy-initialized later in the request.

system_info(), or rather hook_system_info_alter(), is invoked very early in the request though. Possibly so early that, by initializing the theme, we'd nullify the purpose of $custom_theme and 'theme callback' for the current request.

OTOH, we purposively said that "drupal_alter() for themes" won't allow themes to alter everything. Earlier patches even took a white-list approach, but that was deemed to be too limiting. Therefore, I fear that the consequences of initializing the theme too early are too mind-blowing to even try to support that.

I'd guess that the only way to remotely support this would be to load, but not initialize a theme, i.e. treat themes similar to how we treat modules in the bootstrap process: what's enabled is loaded, disregarding whether it will be actually used.

If we rescue to "not for altering everything, cowboy!", then I guess we simply need to clarify the documentation.

moshe weitzman’s picture

Status: Active » Fixed

I agree with ""not for altering everything, cowboy!". Lets do as webchick suggests and put this issue back to fixed. If someone wants to add some docs, do so in new issue.

effulgentsia’s picture

Status: Fixed » Needs review
FileSize
1.85 KB

I agree with ""not for altering everything, cowboy!"

I agree too, but if this issue is to make any sense at all, then themes at the very least need to be able to implement hook_form_alter(), hook_page_alter(), and hook_(node|comment|user|field_attach|block)_view_alter(). I agree with chx in #88 that if we've done our job right with respect to the D7 render system, then theme() doesn't get called until after all these hooks have already run, and that calls into question the entire premise of lazy theme initialization. What's the point of lazy theme initialization? We now have menu items supporting 'theme callback'. Between that and hook_init(), can we assume that we know everything we need to know just before invoking the page callback and initialize the theme then? Here's a patch that does that.

moshe weitzman’s picture

Status: Needs review » Needs work

Thats a good direction, but we need to stop short of actually building the theme registry. Thats expensive, and serves no purpose on unthemed pages like rss feeds, ajax responses, private file requests, etc. Its true that the theme reg often comes out of cache but still. sun just suggested split the loading of theme from the init of theme. Thats a good approach, IMO.

effulgentsia’s picture

Priority: Normal » Critical

Bumping priority, so I don't forget to keep my eye on this, and for others to give attention to it. We can roll this issue back. Or we can make it work. But we can't release D7 with such a major thing broken.

effulgentsia’s picture

Issue tags: -Performance

Performance issues were solved in #76.

effulgentsia’s picture

Issue tags: -Needs documentation

Documentation was completed in #85.

(Sorry for doing in 3 comments what should have been done in 1.)

effulgentsia’s picture

Component: base system » theme system

.

Nick Lewis’s picture

Priority: Critical » Normal

It feels like there's disagreement here, and the problem doesn't strike me as being well defined at this point. Bumping it to normal for now (forgive me).

Here's a few questions i have after reviewing the discussion:
1) did we unintentionally and unknowingly break a large portion of contrib, or drupal 6 sites with the patch
2) are performance problems benchmarked (if they exist) and do they pose a serious problem? (serious face) and how serious.. (/serious face)
3) have regressions defeated the purpose of this patch in the first place?
4) will this make drupal's API suck more, or will drupal api's just suck the same as they always have? (crude way to put it, but you catch the drift)

In any case, just need a straightforward and scary description of why this is critical if it is.
***
Side Note:
I do think we need a separate issue at this point (ESPECIALLY if this is critical). There's way too much backstory and its hard to separate the problems from the stuff the patch solved.

effulgentsia’s picture

Status: Needs work » Closed (fixed)

Fair enough. Back to closed as per #86. New issue: #812016: Themes cannot always participate in drupal_alter().

tnoronha’s picture

Just one note, if I for i.e. use mytheme_form_article_node_form_alter to add a field to the form, it only works if I add it to Seven (the admin theme), is this intentional?