Block module thinks it is special but it isn't (anymore). Lets make it optional, and allow contrib to provide alternate ways to populate the page regions - thats the fundamental goal of block module. For example, Panels will surely consider becoming a block replacement.

This patch adds some standard install/uninstall/hook_flush_caches to block module.

We keep the same user experience during install of default profile, but block module gets enabled and populated by the profile, not by core. Expert profile does not ship with block enabled anymore since it is supposed to show only the required bits.

The only minor blemish I see in the patch is a module_exists() that is added to cache_clear_all(). I'll refactor that function in a subsequent patch. That function needs a serious overhaul as it does performs different functions in each of its branches.

Files: 
CommentFileSizeAuthor
block.patch6.68 KBmoshe weitzman
Passed: 9347 passes, 0 fails, 0 exceptions View

Comments

moshe weitzman’s picture

Title: Male block module optional » Make block module optional
drewish’s picture

nice! looks like a pretty straight forward patch to me.

netaustin’s picture

I like this patch a lot. I only wonder about the very many modules that depend on its functionality to display important information. If this is a "do it if you know what you're doing" feature, that'd probably be fine.

This may mean that other modules which fill regions in other ways call hook_block to maximize compatibility. I'm OK with that, but it could lead to some confusion.

Patches like this make Drupal more flexible and malleable. I like it.

Crell’s picture

Status: Needs review » Needs work

Very old related patch, probably best to just absorb here: #244328: Move block creation to preprocess function

- The new insert statements should use the new syntax.

- Instead of the module_exists('block') call, why not just have block implement hook_flush_caches()? Cleaner code that way.

But yeah, totally +1 on this.

merlinofchaos’s picture

Status: Needs work » Needs review

+ if (module_exists('block')) {
+ cache_clear_all(NULL, 'cache_block');
+ }

Aren't we beyond module_exists checks?

How about hook_cache_clear_all().

moshe weitzman’s picture

@merlinofchaos - yes - see my opening post. thats beyond the scope of this patch IMO. If I do that people will howl about kittens.

@crell - those are not new inserts but rather moved from system.install. also, i did add block_flush_caches(). it doesn't help the module_exists(). and, we already did the cleanup you propose in #244328: Move block creation to preprocess function. tough to keep up with the commits.

merlinofchaos’s picture

IMO an if (module_exists()) is unacceptable even temporarily, but I'll defer to committers.

moshe weitzman’s picture

@merlinofchaos - so your position is patch can't proceed until a cache_clear_all() patch revamp materializes and is committed? that sort of dependency chain murders progress IMO.

Crell’s picture

I'm unclear on what can't be handled right now with hook_flush_caches(). Moshe, can you elaborate?

moshe weitzman’s picture

@Crell - hook_flush_caches() has a different meaning than cache_clear_all(). It includes menu_rebuid() and css()/js() rebuild and so on. It would be nice to have clearer names for these but thats why cache clear needs its own patch - this one is about totally unrelated stuff - block module.

moshe weitzman’s picture

Just to finish that thought - cache_clear_all() purges the page and block cache. Those are hard coded. We need a hook there as merlinofchaos proposes. Thats for a another patch though, which I have already said I will contribute (see my original post).

catch’s picture

Status: Needs review » Reviewed & tested by the community

+1 from me too.

Let's open a new issue for hook_flush_all_caches() or whatever we need now so it doesn't get forgotten after this is committed. I made some small changes to cache_clear_all() in the node_load() caching patch, and agree that whole thing needs cleaning up, but it shouldn't be a dependency here. In fact, here's that issue now - #367949: Tidy up cache clearing

Additionally, those inserts shouldn't hold this patch up either - I wanted to move core input filters to the default profile, simple copy and paste, and that patch has been held up for more than 6 months because it turned into a discussion about a filter CRUD API - #275815: Fix filter and input format saving.

There's no other issues with this patch, so RTBC.

moshe weitzman’s picture

Assigned: Unassigned » moshe weitzman

Thanks catch. I'll work on that cache clear patch.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. See you in #367949: Tidy up cache clearing for clean-up!

Dave Reid’s picture

Should we have also included the block install profile tasks for the minimal install profile since there would be no login or navigation block? I guess I can see why we wouldn't, but just wanted to confirm this was intended.

dropcube’s picture

So, now that module block is optional, we should move block theme definition from system module to block module: #369409: Move block theme definition from system module to block module

cwgordon7’s picture

I agree with Dave Reid - having the block module disabled in the Drupal (minimal) profile makes the site unusable. Also, the Drupal (minimal) profile should probably enable the three blocks enabled now in the default profile.

moshe weitzman’s picture

I'm not opposed to adding these blocks to minimal. Lets use a new issue for that.

catch’s picture

We should also consider adding either a dependency or some nagging in hook_requirements() for menu module.

cwgordon7’s picture

cwgordon7’s picture

@catch - I agree, new issue opened at #370811: Menu module should depend on block module.

webchick’s picture

Status: Fixed » Needs work
Issue tags: +Needs Documentation

We got bit by this at #370806: Block module should be enabled in the Drupal (minimal) profile, which means other install profile developers are going to, as well. Please add a note to the update documentation about this change.

moshe weitzman’s picture

Status: Needs work » Fixed
Issue tags: -Needs Documentation

Added upgrade docs

Status: Fixed » Closed (fixed)

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

swentel’s picture

While this patch is cool, we also forgot to remove the block cache fieldset on the performance page which is redundant when the block module is disabled - imo. There is a patch in the queue which adds some extra functionality on the block settings page (change block cache setting) and takes care of that too, see http://drupal.org/node/347350#comment-1438026

moshe weitzman’s picture

Shouldn't block module itself form alter the performance page to add its fieldset? Thats the core problem.

swentel’s picture