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.
Comment | File | Size | Author |
---|---|---|---|
block.patch | 6.68 KB | moshe weitzman | |
Comments
Comment #1
moshe weitzman CreditAttribution: moshe weitzman commentedComment #2
drewish CreditAttribution: drewish commentednice! looks like a pretty straight forward patch to me.
Comment #3
netaustin CreditAttribution: netaustin commentedI 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.
Comment #4
Crell CreditAttribution: Crell commentedVery 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.
Comment #5
merlinofchaos CreditAttribution: merlinofchaos commentedAren't we beyond module_exists checks?
How about hook_cache_clear_all().
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commented@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.
Comment #7
merlinofchaos CreditAttribution: merlinofchaos commentedIMO an if (module_exists()) is unacceptable even temporarily, but I'll defer to committers.
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commented@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.
Comment #9
Crell CreditAttribution: Crell commentedI'm unclear on what can't be handled right now with hook_flush_caches(). Moshe, can you elaborate?
Comment #10
moshe weitzman CreditAttribution: moshe weitzman commented@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.
Comment #11
moshe weitzman CreditAttribution: moshe weitzman commentedJust 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).
Comment #12
catch+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.
Comment #13
moshe weitzman CreditAttribution: moshe weitzman commentedThanks catch. I'll work on that cache clear patch.
Comment #14
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. See you in #367949: Tidy up cache clearing for clean-up!
Comment #15
Dave ReidShould 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.
Comment #16
dropcube CreditAttribution: dropcube commentedSo, 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
Comment #17
cwgordon7 CreditAttribution: cwgordon7 commentedI 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.
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedI'm not opposed to adding these blocks to minimal. Lets use a new issue for that.
Comment #19
catchWe should also consider adding either a dependency or some nagging in hook_requirements() for menu module.
Comment #20
cwgordon7 CreditAttribution: cwgordon7 commentedOk, new issue opened at #370806: Block module should be enabled in the Drupal (minimal) profile.
Comment #21
cwgordon7 CreditAttribution: cwgordon7 commented@catch - I agree, new issue opened at #370811: Menu module should depend on block module.
Comment #22
webchickWe 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.
Comment #23
moshe weitzman CreditAttribution: moshe weitzman commentedAdded upgrade docs
Comment #25
swentel CreditAttribution: swentel commentedWhile 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
Comment #26
moshe weitzman CreditAttribution: moshe weitzman commentedShouldn't block module itself form alter the performance page to add its fieldset? Thats the core problem.
Comment #27
swentel CreditAttribution: swentel commented@moshe see #424252: Move block caching UI to block module