It's impossible to alter theme regions

I rerolled the system.api.php part of the patch. There was a missing { causing the simpletest fail.

The alter is totally RTBC. chx and I discussed it last week. And coincidentally, we had someone in IRC tonight ask for such a thing in Drupal 6.

JohnAlbin: did you forget to post the patch?

+ * @param @list
+ * List of the available regions from a specified theme.
+ * @param $theme_key
+ * The name of the theme.
+ * @param $show
+ * Possible values: REGIONS_ALL or REGIONS_VISIBLE. Visible excludes hidden
+ * regions.
+ */
+function hook_region_list_alter(&$list

@param @list vs hook_region_list_alter(&$list

is this correct?

The hook itself is great, but overlay module can't use it. David_Rothstein pointed out that when you view the block configuration page, it's dependent on the region list. So if overlay implements hook_region_list_alter, visiting admin/structure/block in the overlay will disable all your sidebar blocks. Not good.

Also,

+++ modules/system/system.module	2010-04-23 03:59:40 +0000
@@ -2512,6 +2512,7 @@ function system_region_list($theme_key, 
+    drupal_alter('system_region_list', $list, $theme_key, $show);

s/system_region_list/region_list

95 critical left. Go review some!

New patch addressing ksenzee's concerns, as requested by chx. The overlay part is no longer in here.

chx: How would someone go about reviewing this patch?

This hook would be really nice! (Although even if it could be used in the overlay I am not sure it would improve performance more than a little bit - the overlay is already making sure not to render these blocks, so the only difference would be for non-block objects inserted into the page.)

I see three issues:

1. I don't think removing the overlay part of the patch really addresses #7, since couldn't any other module implementing this hook accidentally break the block configuration page as well? Seems like at the very least it needs to be documented how and when it is appropriate to use the hook.
2. Speaking of which, the patch does not have any hook documentation at all except for the @param descriptions :) It needs at least one sentence at the top, maybe more.
3. I'd be slightly worried that people are sometimes getting their region lists from other places than system_region_list(), e.g. from the theme's 'info' property directly, as returned by list_themes(). Doing a grep for 'regions' reveals a couple places even in core that look suspect. So it seems like for this to work correctly, we need to enforce or encourage via documentation that people always use system_region_list() rather than some other source.

Random question (perhaps related to #11-3?): Would it make sense for Dashboard module to implement this hook to implement it's custom regions?

we all live in a yellow subscribe-marine...

I think the todo list in #11 is still active.

#8: drupal-778476-8.patch queued for re-testing.

tag

If you are going to push this patch please remove the extra newline

+    unset($list[$theme_key][$show]['sidebar']);
+  }
+}
+
+

Merely removing the whitespace and rerolling for the current head.

This is a api change. Is there any tests?

- Hook description missing
- Tests are required

Shouldn't this be priority "Critical" since it is a beta blocker?

Wtf? This is a new feature. You could never control theme regions from a module in any version of Drupal ever.

Works for me.

Now we just need to update the community initiative page for core:
#871798: Update Community Initiative Page - Drupal 7 Beta Blockers

philbar: Go for it! Everyone should have access to that page, and you've been doing a good job maintaining the list. :)

mmm, as with #908116: hook_block_region_alter() is missing in block_get_blocks_by_region(), this is a very simple alter hook addition, and I'm really not sure how allowing Drupal developers to alter slightly more can be an API breaking change that can no longer be considered for D7? Simple alter hook additions could even be easily backported, if they'd apply.

This entire issue arose of an actual need to be able to do this. And that need was confirmed by others. So, this is a simple alter hook - it's not like we'd rewrite the theme system to fix this bug. :)

It's not that it's a API breaking change. It's that it's a new feature. Feature freeze ended 12 months ago. The only patches we should be working on at this stage are those that lead directly to Drupal 7 getting released. Drupal 8 for this.

Sorry, but that simply doesn't make sense. In case you didn't notice, I'm actively and heavily contributing to get a D7 beta out of the door. However, that does not mean that we have no less-critical bugs in the system that should be fixed.

Adding alter hooks like this is what happens throughout Drupal development sphere all of the time - and reality is, it most often happens in stable branches, as other developers only find out throughout usage that there is a (soft) limitation that should be fixed. In 99% of all cases, a drupal_alter() can be inserted without any issues into the existing code - solving the integration problem and bug for other developers, while keeping and ensuring API compatibility for everyone else.

It is my understanding that the alpha, beta, RC release cycle phases are exactly here to allow developers and early-adopters to use the product and system we've developed. It's exactly in this stage, where we want developers to report bugs about integration issues, so we can fix them.

And still, we are talking about an alter hook. That's no feature. The drupal_alter() feature already exists. We are talking about a missing alter hook, which is a bug from an implementor perspective.

My question there is "Where does it end?" We could literally find integration issues for another 12 months. We need to cut it off somewhere and say "Yes, that's a limitation of D7," just as at one point we cut it off with D6 and said "Yes, that's a limitation of D6." Or else we never would've seen Drupal 6.0, and we haven't seen a Drupal 7.0 in over 18 months.

The time to transition to doing this cut off was when we started rolling alpha releases. It's even more crucial as we're on the cusp of entering beta phase.

I'm sick and tired of playing status ping pong with you. I wish you would respect my views as the release manager, but I guess I'll leave this for Dries and see what he wants to do with it.

Of course, I understand and respect your decisions as release manager. I can totally accept that maintainers call me nuts if I want to break lots of code with stuff like #375397: Make Node module optional, because it really isn't on the table for D7. We can cope with that.

But in this and the other case, the decision solely translates into: "We do not fix normal bugs anymore for D7." There are incidents in which similar fixes have been applied to D6. Not 6.0-dev or 6.0-alphaX, but a stable ~6.19.

With contrib actually chasing HEAD and being able to start to consider D7 as remotely stable, we are starting to use Drupal 7 and actually build (temporary or not) sites just now. While the addition of an alter hook could be understood as a feature under your hat, I'm wearing the site builder and Drupal developer hat, and for me, it's equally important to fix such normal bugs as it is important to fix the remaining critical issues. I cannot simply ignore normal bugs. Especially if we are able to fix them easily.

Of course, we also respect and accept the real limitations of Drupal 7. For example, not being able to perform any kind of mass-operation on any kind of content. That's totally different to simple bug fixes like these, and that is the actual difference for me.

Overall, we are making very good progress with making D7 rock solid, but in my opinion, it would be wrong to assume that we are ready for prime time already. The list of critical issues is just an indicator of what is really broken, but does not have any meaning with regard to other aspects of D7. We both know that I (and all the others) are already working hard to resolve those absolutely critical issues. But let's please not totally forget about other existing issues, which we are trying to resolve in parallel, in our spare time, when not having to work on the criticals.... doesn't that sound like slavery already? It starts to feel like that, as it seems we're only allowed to work on critical issues today. It's the effective communication that ends up on my side here.

All philosophy aside, please see the comments in #7 and #11. So far, the only time we tried implementing this hook in core it broke things, and that could happen elsewhere too, no? Someone might implement this hook trying to dynamically change the page regions that are displayed, but also inadvertently affect anything else on the same page that is trying to display a list of regions for a different purpose (e.g., in a select list in a form): And the block configuration page was a very real example of that. It may turn out to be that this hook is too blunt of a tool...

Is there a specific use case where this hook is necessary that can't already be achieved in Drupal 7 via hook_page_alter()?

Also, there is a small typo in the PHPDoc: "This hook is invoked form" => "This hook is invoked from".

Fixed that typo.

@David: I've grepped, but I'm not sure what you mean, but in any case, if there's another bug, then we should tackle it in a separate issue.

Well, it's possible that #11.3 isn't valid anymore... But the bigger issue (that this hook can easily have lots of unintended side effects, as we saw with the block configuration page) still is.

@David: Every single alter hook in Drupal has unintended side-effects, when improperly used. That's absolutely no reason to object this patch.

Sure, but we also need some examples of proper uses of the hook (that don't have those unintended side effects) and so far I'm not sure we have one.

I also think that between hook_page_alter(), hook_system_info_alter(), hook_block_list_alter(), etc it should be possible to accomplish pretty much anything this new hook could, and without the potential side effects.

- hook_page_alter() has performance issues: everything has been built already.

- hook_system_info_alter() is a permanent change: you need to rebuild module/theme data to get any changes. Doesn't work at all for dynamic changes.

- hook_block_list_alter() only works for Block module, but Block module is pretty much a thing of the past.

Although badly needed, this is D8 material according to the rules (I had to learn today). It may be backported at a later point in time (though that's unlikely).

Since this is now going into 8.x, removed the Issue tag "API addition"

Sorry guys - I thought i was helping. I was under the impression "Api Addition" was added for module devs so that they can quickly scan which API changes came in after the code freeze. Something tells me I'm dead wrong about that, so putting it back exactly the way it was.

Patch won't apply in D8

Rerolled patch for D8 and fixed the broken example code in hook_system_region_list_alter().

I'm still concerned that this makes system_region_list() and list_themes() return inconsistent data. That's pretty confusing. We at least need to change the documentation of system_region_list() to explain the difference.... i.e., specifically mention that this is a dynamic list, whereas list_themes() returns the actual data.

Per above comments, I also think the hook documentation has to do a better job describing proper usage of this vs hook_page_alter(), and pointing out some of the pitfalls described above. Or rather than describing the potential pitfalls, we could just change the Block module and other admin forms in Drupal not to call system_region_list() but rather get the regions via a more direct method. For their use case, they do not want to get a dynamic region list, but rather the actual one. That's probably a better option.

I don't think we should change block module for this, it's up to the developer to use the hook properly. Improper use of any alter hook can break lots of things. I did a grep for system_region_list() and it doesn't get called much in core, so there shouldn't be many side-effects.

Apart from that, I agree that it couldn't hurt to add some more documentation. As suggested by David in #43, I added a comment to system_region_list() to clarify that its return value could have been altered by other modules, unlike list_themes(). I also added a comment to hook_system_region_list_alter() to warn for improper use.

+++ b/core/modules/system/system.module
@@ -2710,6 +2710,12 @@ function system_find_base_themes($themes, $key, $used_keys = array()) {
+ * ¶

Trailing whitespace.

-15 days to next Drupal core point release.

Reroll for whitespace.

Thanks, that documentation looks a lot better. (There is a minor grammatical issue, though: two places where the patch says "themes .info file" should say "theme's .info file" instead.)

***

I did a grep for system_region_list() and it doesn't get called much in core, so there shouldn't be many side-effects.

So, I just did the same grep now, and unfortunately I'm really coming to a different conclusion.

Not counting tests and documentation, there are 12 different calls to this function in core. By my count, 10 of those 12 are places that do not know how to properly handle a dynamic return value; they are calling this function because they really expect to get a full list of the actual regions in the theme.

A particularly fun one is _block_rehash(); that function can get called from a variety of pages in a variety of different ways. If it ever happens to get called on a page where someone implements this hook to remove some regions, all blocks in those regions will get auto-disabled. Not so good.

The only two places where it actually looks like a dynamically-changing region list would be the desired behavior are system_page_alter() and the first part of block_page_build(). I don't think 2 out of 12 is a particularly good ratio.

So I still think we should look at the possibility of having two different functions. If there is a standard function that 'page building' modules can call to get the list of active regions on the current page, then that would be an appropriate function to add this alter hook to. For example, it might naively be called something like system_active_region_list(), and just be a wrapper around system_region_list() with the alter hook at the end...

But system_region_list() itself does not seem like the right place for an alter hook at the moment, given the way this function is currently being used.

Why is this specific to a particular region? Can it not be generalized?

#46: 778476-hook-system-region-list-alter-46.patch queued for re-testing.

I do not think that this will be supported in D8 — in fact, we're actively working hard to remove all global page alterations.

Hence, moving back to D7. Although I'm not sure whether it makes sense to introduce a new feature this late for D7 that will most likely not be supported in D8.