Spinning this off from #135464: Add language options to block visibility.

I think we should add a hook_block_visibility() to _block_load_blocks() - this would take an array of all the blocks loaded and remove any from the array which shouldn't be visible (so always subtractive).

http://api.drupal.org/api/function/_block_load_blocks/7 has user/role specific code in there we could move to user.module. Multilingual blocks could be entirely handled by locale() module, PHP block visibility could probably be handled by PHP module entirely.

This should also mean that we could remove the join on block_role, and run a separate query from user_block_visibility() instead - which would be a lot less expensive than the single query with temporary tables and filesorts we have now. We could also skip that query entirely if role visibility isn't being used (i.e. add a setting somewhere and check the variable before it runs).

Additionally, adding the hook would likely mean we could kill the db_rewrite_sql()/hook_query_alter() from here - meaning it'd just be a straight db_query on the blocks table, allowing us to avoid running a db_select() on it.

Here's an explain on the current query in block_list() from a real Drupal 6 site.

+----+-------------+-------+------+---------------+---------+---------+----------------------------------+------+----------------------------------------------+
| id | select_type | table | type | possible_keys | key     | key_len | ref                              | rows | Extra                                        |
+----+-------------+-------+------+---------------+---------+---------+----------------------------------+------+----------------------------------------------+
|  1 | SIMPLE      | b     | ref  | tmd,list      | list    | 195     | const,const                      |   17 | Using where; Using temporary; Using filesort | 
|  1 | SIMPLE      | r     | ref  | PRIMARY       | PRIMARY | 292     | libcom6.b.module,libcom6.b.delta |    2 | Using where; Using index; Distinct           | 
+----+-------------+-------+------+---------------+---------+---------+----------------------------------+------+----------------------------------------------+
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Status: Active » Needs work
FileSize
2.54 KB

Very, very rough incomplete untested patch to give an idea where this is going. Takes a bit of work to re-implement the block visibility in PHP (if it even works), but everything else already acts on the blocks loaded from the database, so that would all be a lot simpler and cleaner.

catch’s picture

Status: Needs work » Needs review
FileSize
8.07 KB

Here's a working patch. All block tests pass, although there's none for actual block visibility yet (tested this manually and it works though).

In the hook_block_visibility() documentation, I've added a suggested way in which blocks could be filtered out by language as an example of a hook implementation (the immediate use case this patch is designed for). Otherwise block module continues to control role/page/user visibility in it's own implementation of the hook, at least for now.

So while this functionality needs tests written, there's none there now, and it's working well. I'll see if there's an existing issue for block visibility tests elsewhere and update that, and/or we could introduce some with the multilingual blocks patch to test the hook itself.

RobLoach’s picture

This is awesome!....... Unsubscribing.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
9.37 KB

Not sure why that test is failing, very odd.

Discussed this briefly with webchick in irc, and she pointed out that this hook also allows you to set $block->content before block module attempts to do rendering - so you could alter the contents of a block before it gets loaded. This would make it more than hook_block_visibility(), but I'm not really sure what else we can call it - _alter() suggests the fully rendered block is available to alter, and that's not the case here. _pre_load() might work.

Either way, here it is with tests added for page and role visibility - since block module implements its own hooks, this tests the hook and it brings our block visibility coverage up to a reasonable amount from absolutely zero.

Dave Reid’s picture

Very impressive work. In my opinion, _block_visibility is a little too vague. It seems like the purpose of this new hook would be something like _block_access() (similar to node_access) unless we're going to generalize the hook into a full-on alter like the suggested _pre_load() in #5. Is there an advantage to this approach vs introducing a new _block_alter() to be called after block rendering?

catch’s picture

So the main advantage here is everything is run before hook_block_load() gets called.

This means you're killing blocks dead before their content gets loaded - saves doing the work twice. It also means you can (technically, even if it wasn't the original purpose of the patch), swap in your own content before they get rendered (since there's an isset($block->content) check in block rendering already - which again would allow for changing things around before any real work gets done.

I don't think there's much use for a hook_block_alter() as such - because we have hook_page_alter() now and blocks are available there - so if you really need to operate after everythings rendered, you already can.

If we change the name to hook_block_pre_load() how does that sound?

catch’s picture

FileSize
9.54 KB

Discussed this in irc, and renamed to the more generic hook_block_load()

catch’s picture

Title: Add hook_block_visibility() » Add hook_block_load()
quicksketch’s picture

I would have to oppose using the name "hook_block_load". This goes against most of our APIs, where a load operation involves loading a specific object's information from the database (hook_user_load() and hook_nodeapi_load() for example). This is doing something rather different, where it's actually removing blocks, rather than loading them.

I'd be supportive of hook_block_access(), but again it's not a single block that's being checked for access. Would we be willing to mimic other APIs by calling this function individually for each block? This hook_block_access() would receive each block object individually, then return TRUE, FALSE, or NULL (for no opinion) for each block.

My primary reason for avoiding the name of hook_block_load() is because I want to actually create this hook in a different issue. Similar to users or nodes, we'd load a block object and add properties to it. Then in hook_block_view(), we would check these properties and render the block appropriately, as opposed to our current approach of calling variable_get() directly in the view operation.

catch’s picture

I'd be 100% opposed to having this operate on each individual block - the whole point is to fix the performance implications of block visibility, and a check on every block is going to mean all the calling functions either doing duplicate queries or having to deal with a static cache.

How about just going back to hook_block_visibility() and leave the $content cleverness as a potentially useful side-effect?

catch’s picture

Title: Add hook_block_load() » refactor block visibility

retitling to something more generic.

z.stolar’s picture

See http://drupal.org/node/135464#comment-1247188 for locale.module implementation.

Jose Reyero’s picture

Though I think the block visibility hook is a good idea, it shouldn't totally replace block query conditions. The reason is that there may be a really big number of blocks for some sites, they may be architected around 'Node As Block' or Pannels modules, and this can make the list really big.

Thus I'm thinking of generalizing the 'visibility' settings and, as the relationship will be 1 to many most of the times, have a number of block_visibility_* tables. Some of the options that may go in here are:
- Per user or custom blocks
- Per language (new)
- Per region
- Per theme
- Per role
- Per node type (new)
......
So we'd need some hook for modules to add in query conditions before the query is thrown, and then to pass the results through hook_block_visibility so other modules can do aditional tweaking (complex path matching, etc..)

catch’s picture

@Jose - the block_list() query is run on every page, and the basic query, out of the box, does a temporary table and a filesort (see #1) - add lots of blocks to this, and extra joins, and it's going to be a very slow query. Loading an array of 100 or so items and filtering stuff out of them is nothing compared to mysql having to create temporary tables. Even if every module doing block visibility ends up doing it's only table and a simple select out of it, those 4-5 queries aren't going to compete with joins + conditions on the same number of tables.

We should also be moving hook_block() to an array based approach - see #257032: Split block $ops into separate functions - but that doesn't yet deal with the block loading issues here (in fact role visibility is still a TODO), so IMO that patch and this one should play reasonably well with each other.

Once these two are in, I'd like to see us move to a more menu system based approach to blocks - i.e. you specify a set of blocks, then assign this set to a router path or paths for blocks (node/%, user/% node/1, ), with the most specific path winning at any one time. We'd then be querying only the blocks attached to a specific path, and then doing visibility against that (usually) smaller number. This would mean the path matching stuff gets refactored to effectively happening when you save a block (or block set) rather than on load - but per user/role/custom PHP/language are still going to be needed, and the menu system doesn't have a corollary for that.

Note that I agree with quicksketch about the hook naming - but #5 is still valid pending showstoppers.

JohnAlbin’s picture

subscribe.

Status: Needs review » Needs work

The last submitted patch failed testing.

Gábor Hojtsy’s picture

Concept looks good, since this would finally allow contrib modules to add more block visibility without people resorting to PHP snippets. Currently, modules could alter the form for settings, but cannot go into the process of the actual block selection.

However, looking at the code: $blocks passed by reference all through the hook implementations being altered all the way, this sounds more like a hook_block_list_alter() or something to suggest a hopefully better name.

Pasqualle’s picture

rerolled

changed the hook name to hook_block_list_alter
changed the $blocks array key to $bid (as "$module . $delta" should not be used as array key)

untested

Pasqualle’s picture

Status: Needs work » Needs review
Pasqualle’s picture

fixed a trivial bug in the api example

Status: Needs review » Needs work

The last submitted patch failed testing.

Pasqualle’s picture

the block_block_list_alter() needs a first check for theme and status, because if I add a block to the block list (in my module) I want that block to remain in the list, despite of other visibility settings (Context module behavior).

if ($block->theme != $theme_key || $block->status != 1) {
  // This block was added by a contrib module, leave it in the list.
  continue;
}

maybe we will need two hooks, one which adds to the list and one which removes from the list if this hook will be overused in modules with complex visibility logic..

catch’s picture

Adding blocks to the list I hadn't included in my original use case, but that'd be possible, so we need to work out how to deal with it. This is a bit like hook_form_alter() in terms of modules potentially conflicting with each other - I'm not sure if two hooks would work to simplify that - if they both worked similarly to this, then you'd be able to use both hooks for both purposes regardless.

Gábor Hojtsy’s picture

Agreed. In the form_alter() case, core does not care about resolving potential conflicts either, it is completely up to the modules to synchronize somehow.

Pasqualle’s picture

Status: Needs work » Needs review
FileSize
9.91 KB

added check from #25

fixed variable name bug in #23 $blocks_roles vs $block_roles

Berdir’s picture

Status: Needs review » Needs work

Some things, mostly DBTNG related..

+  $result = db_query('SELECT module, delta, language FROM {my_table}')->fetchAll();
+  $block_languages = array();
+  foreach ($result as $record) {
...
+  $result = db_query('SELECT module, delta, rid FROM {block_role}')->fetchAll();
+  foreach ($result as $record) {
+    $block_roles[$record->module][$record->delta][] = $record->rid;
+  }

You don't need fetchAll() to loop over the result with foreach.

+  $result = db_query('SELECT * FROM {block} WHERE theme = :theme AND status = 1 ORDER BY region, weight, module', array(':theme' => $theme_key));
+  foreach ($result as $record) {
+    $queried_blocks[$record->bid] = $record;
+  }

I haven't used that myself yet, but I think here you can use $queried_blocks = ...->fetchAllAssoc('bid'). See http://drupal.org/node/310072

+  // Invoke hook_block_list_alter() to allow modules to alter the list.
+  foreach (module_implements('block_list_alter') as $module) {
+    $function = $module . '_block_list_alter';
+    $function($queried_blocks);
+  }

Is there a reason that drupal_alter('block_list', $queried_blocks); doesn't work instead of that?

Pasqualle’s picture

Status: Needs work » Needs review
FileSize
9.69 KB

made all changes from #29

Pasqualle’s picture

a better explanation why the special check #25 is required

any module using this alter should inspect the data before changing it, to ensure it is what they expect.

quote from (a non-related issue) #451152-22: Implementing a hook on the behalf of another module

Status: Needs review » Needs work

The last submitted patch failed testing.

Pasqualle’s picture

Status: Needs work » Needs review
FileSize
9.69 KB

reroll

Dries’s picture

Issue tags: +Favorite-of-Dries

Like it, but some feedback:

- Instead of writing "Implementation of hook_block_list_alter()" I think it is worth being a bit more verbose about the purpose of this function.

- In the documentation, I'd mention that the _alter() function uses the block list, not the actual blocks. For performance reasons, the blocks are retrieved after modules have had a chance to manipulate the block list.

- // This block was added by a contrib module, leave it in the list. -- let's be a bit more verbose/helpful in the code comments of both the api documentation and block_block_list_alter(). (See link in previous comment.)

- The only minor concern that I have from an API's point of view is that the _block_list_alter() hook removes blocks from the list. This is probably fine, but it limits what the next _block_list_alter() hook can do. There leaves no room for more complex block negotiation between modules. Specifically, it is impossible for another module to re-enable a block.

- testBlockVisibility() is good for now but should be extended over time.

- Some basic benchmarking would be good.

Should be pretty easy to drive this one home!

Pasqualle’s picture

I do not know what other kind of block negotiation is needed, and how should it work. The current core functionality is to remove the block from the list when the visibility conditions (user role, path, content type, actual language) are not met.

The context module leaves the core block list intact, only adds new blocks to the list. I think it can be achieved with this hook, probably setting the $block->status = 2, so the new blocks added by the context module will remain in the final list..
Currently context-6.x is using a dirty hack (overriding the theme_blocks) to achieve this, therefore it has a conflict with i18nblocks which is using the same hack.
So the i18nblocks_block_list_alter() should be called after core and context module made their block list modifications, to translate the final blocks. And as I know that can be achieved with a larger module weight..

If more complex negotiation is needed then we need to know the requirements..

Dries’s picture

I don't think we need to have more advanced block negotiation -- I think we're OK for now. I was merely pointing it out, not requesting that we change anything.

Pasqualle’s picture

My English is not suitable for writing documentation so if you find something wrong in the comments, then don't be polite, just tell me or just fix it..

I think I made all the required modifications from #35.

I can't provide benchmarking, I would leave it to someone more experienced..

small note: in _block_load_blocks() function there is an ->execute() and also a ->fetchAllAssoc('bid'). I am not sure if both are needed, but it does not looks good for me. Sorry, did not have the time to learn the new db abstraction yet..

catch’s picture

If #257032: Split block $ops into separate functions gets in, I think we could look at block visibility being an 'access callback' - except we'd probably need to make access callbacks an array (so you can easily combine language and content type visibility without having to copy entire functions into your custom callback). That's a significant refactoring with it's own design issues though so I still think this is a good incremental improvement for now.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Enabled all blocks in the default profile.

ab -c1 n1000 - 2 times each

HEAD:
14.88 [#/sec] 15.47 [#/sec]

Patch:
15.01 [#/sec] / 15.69 [#sec]

So no measurable difference, which there shouldn't be. However I'm hoping this means patches like #135464: Add language options to block visibility won't need to rewrite the block_list() query every time we need a new additional visibility rule, which should mean much less chance of unindexed queries. Since pasqualle's comments look fine to me, marking this RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Great! Committed to CVS HEAD.

Marking this 'needs work' until we updated the upgrade instructions in the handbook.

catch’s picture

Status: Needs work » Fixed
catch’s picture

Just noticed this was committed with http://drupal.org/cvs?commit=222410 - looks like it's the only extra which got committed there.

Status: Fixed » Closed (fixed)

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

alanburke’s picture