This patch would allow adminstrators to configure blocks to show up only pages of a specified node type.
This patch is being submitted in response to feedback on the proposed changes to the book module http://drupal.org/node/14120. Feedback indicated that if the block generation associated with books is changed, administrators will still require the ability to specify that newly created book blocks behave as the existing book blocks do (i.e. only show up on book pages).
This patch goes one step further and would allow an admin to show a book block (or any block) on any type of page.
While independent of 14120 - this patch in conjuntion with 14120 would allow the removal of a large piece of block generation code from the book module.
Comments and feedback, as always, are welcome.
andre
Comment | File | Size | Author |
---|---|---|---|
#26 | update_inc_121_1.patch | 569 bytes | andremolnar |
#23 | block_module_6.patch | 8.46 KB | andremolnar |
#22 | blocks_db_change_4.patch | 1.95 KB | andremolnar |
#20 | block_config_mockup.png | 15.1 KB | andremolnar |
#15 | blocks_db_change_3.patch | 1.95 KB | andremolnar |
Comments
Comment #1
andremolnar CreditAttribution: andremolnar commentedAssociated db changes required for this patch.
andre
Comment #2
chrismessina CreditAttribution: chrismessina commentedI hope at some point we can go beyond just "showing blocks on certain node types" to actually determine what blocks to show at the "per page" level (although a page in and of itself is somewhat nebulous).
Essentially, if I have '/home/about/' I would like to be able to target that one page a "add blocks" to it in various regions. Of course, doing this in a cascading way would also be great: if I have '/home/about/' to which I add Blocks X, Y and Z and then I have '/home/about/bios/', I could disable X and Y but leave Z alone and it would be "inherited".
I realize that there was a huge discussion about this topic that I didn't completely follow, so hopefully my comments aren't too far off base.
Comment #3
Steven CreditAttribution: Steven commentedChris, have you seen the block configuration in HEAD? Drumm developed that patch...
Comment #4
Dries CreditAttribution: Dries commentedThe coding style (placement of spaces and brackets) needs a bit of work, but the functionality is a welcome addition that I would commit to HEAD.
Plus, I suggest renaming some variables for readability. The following can be improved:
+ if ($enabled && $matched && $typematch) {
1. $typematch -> $type_match
2. $matched -> $page_match
For consistency with the 'page matching', I suggest that you transform the radio-buttons to checkboxes so multiple node types can be selected and that we rename 'block.type' to 'block.types' (cfr. 'block.pages', plural). On drupal.org, I'd like to disable some block on 'project issue' pages which would not be possible with your patch, but which would be possible if checkboxes were used.
Lastly, avoid the word 'node' in output: use 'post' instead.
On a related note, there is also an interest in being able to display blocks on a role-base so if time permits, you might want to introduce a 'block.roles' column too. ;-)
I vaguely remember a patch by Neil to tidy up the block table: not sure if that is still around and whether that needs to be considered.
Comment #5
andremolnar CreditAttribution: andremolnar commentedOkay - here is an updated patch.
1) Cleaned up the coding style for if else statements (my code 'beautifier' insisted that the other style was 'more beauthiful') ;-)
2) Changed the 'type' column to 'types'
3) Changed the code to reflect new column name
4) Changed variable names (e.g. $typematch -> $type_match)
5) Changed radios to checkboxes
6) Changed type matching block to handle multiple choices from checkboxes
Quick question regarding coding style for concatinated strings (didn't see this in the documentation) - do we:
I tend to do both - but lean towards the latter.
andre
p.s. blocks based on roles would be another patch if I have time ;-)
Comment #6
andremolnar CreditAttribution: andremolnar commentedAnd the database patches - type now types
Comment #7
andremolnar CreditAttribution: andremolnar commentedNoticed another minor formatting problem in the last patch - I think I need a new IDE ;-)
This fixes that.
andre
Comment #8
Dries CreditAttribution: Dries commentedSome small nits (nothing major):
1. I'd vote against using
$foo
. Typically, in that context, we use$result
but that variable name might already be in use.2.
if ($type_match = ($node->type == $type)) break 1;
should be:3. We try not to glue words together. For example:
$nodetypes
would be$types
, or$node_types
if$types
is already in use.4. I would change 'node type(s)' to 'content type(s)'.
Otherwise this patch looks perfect! Good job.
Comment #9
(not verified) CreditAttribution: commentedin re: to FActory-joes idea: We can dot this by connectiong blocks to menus in a block.mid column. where mid is the menu-id.
Comment #10
andremolnar CreditAttribution: andremolnar commentedRe: anonymous: - MID in the block table.
No need for this. If the menu system produces a block then block.delta == menu.mid
andre
Comment #11
andremolnar CreditAttribution: andremolnar commentedUpdated patch with the nits picked out ;-)
Exactly - $result was already in use - But, I've now changed $foo to $result_1
I took my cue from the taxonomy module that uses the same variable name in a similar context... I'll submit a patch for taxonomy for consistancy (for both variable name and variable name style)
Thanks. Is there any chance that this will help http://drupal.org/node/14120 make its way in prior to the code freeze?
I'm going to comb that patch for similar code style issues to help it along, but I still haven't heard too much feedback regarding the general approach I took.
Thanks again. I'm glad to help in whatever way I can.
Comment #12
Dries CreditAttribution: Dries commentedThe patch doesn't apply:
Also, I suggest to remove the new SQL query and to replace it with '$node = node_load(array('nid' => arg(1));'. The function node_load is caching, and will therefore save us an extra SQL query.
Can you try to resubmit your patch?
Comment #13
andremolnar CreditAttribution: andremolnar commentedUpdate.
New patch - implements node_load
andre
Comment #14
Dries CreditAttribution: Dries commentedSaving the block configuration doesn't seem to work.
Comment #15
andremolnar CreditAttribution: andremolnar commentedFound the cause of the problem.
When it was only 1 type being saved I used varchar(16) in the database.
Now that we are saving many more possible types I will change it to a text field
This patch updates the database.
andre
Comment #16
Dries CreditAttribution: Dries commentedWhen I make some configuration changes, save my block, and go to the same block configuration page again, the configuration is lost (the checkboxes are no longer checked). I tried this with the forum block. I configured the forum block to show up on forum pages only, yet it continues to show up everywhere. There is no error in my watchdog; the block table has been upgraded properly. The settings do not appear to be saved.
The update path is dated. You should use update_220 or update_221 instead. I can fix that.
Upon trying this patch I found that the configuration form was somewhat unclear. It's not clear that all conditions need to hold true for the block to show. That is, does it use 'OR' or 'AND' to evaluate the various criteria.
Same for the "Show on specific pages:" setting. Does it apply to the newly added "Show for specific content types:" section or just for the "Pages:" section? I think we need to shuffle the form elements and form_group()s around a bit to communicate this better.
We're getting there ...
Comment #17
andremolnar CreditAttribution: andremolnar commentedHmmmm...
Strange - I have been testing from the start and everything works as expected.
You can demo this on my staging server. (please don't mind the drupal theme ;-) - its not used in production - just on my staging machine - plus its not perfect in firefox).
http://s92258948.onlinehome.us/greenbeach/
UN: drupaldev PW: drupaldev
(p.s. this site has http://drupal.org/node/14120 installed as well - so you can demo that functionality too if you like)
Would someone else be kind enough to give this patch a spin?
I'll see what I can do about the configuration page layout - or add some more text to make it clearer.
andre
Comment #18
andremolnar CreditAttribution: andremolnar commenteddang.
I can now reproduce the behaviour you saw dries - I have to roll back a change or two to see what happened - I will resubmit when I have it worked out.
sorry for wasting people's time.
andre
Comment #19
andremolnar CreditAttribution: andremolnar commentedOkay - demo site now working again.
I'm going to roll a new patch and make sure that I submitted the right one.
andre
Comment #20
andremolnar CreditAttribution: andremolnar commentedFollowing Dries advice - I have attempted to make the configuration form a little more clear.
Attached is a mockup of the changes. Or you can demo this change live at http://s92258948.onlinehome.us/greenbeach/
UN: drupaldev PW: drupaldev
After feedback and tweeks I can roll a new patch.
andre
Comment #21
Dries CreditAttribution: Dries commentedLooks a lot better! I have only one small nit: on the screenshot, replace 'node (type)' by 'content (type)'.
I'm going to commit a patch by Neil that updates update.inc so make sure to update your tree first.
Thanks for the great work Andre.
Comment #22
andremolnar CreditAttribution: andremolnar commentedFirst an updated patch to the database files after updating my CVS tree.
Adds 'types' field to the {blocks} table.
Also noticed that the database.pgsql was missing the 'pages' field. I added that in here too.
andre
Comment #23
andremolnar CreditAttribution: andremolnar commentedUpdated patch.
1) Fixes presentation as per dries request
2) Changes the text to use 'conetent types'
3) Also fixes a minor bug (warning was thrown if a user deselected all content types and attempted to save)
Thanks to demo users I noticed the warning in watchdog. It is fixed now.
Thanks for the kind words Dries - hope this applies smoothly.
andre
Comment #24
Dries CreditAttribution: Dries commentedCommitted to HEAD. Thanks. (I made some minor changes to the block configuration page.)
Comment #25
Steven CreditAttribution: Steven commentedPlease note that the following lines were changed incorrectly:
- drupal_set_message(t('The block %name has been deleted.', array('%name' => ''. $box['info'] .'')));
+ drupal_set_message(t('The block %name has been deleted.', array('%name' => '' . $box['info'] . '')));
- $form = '
'. t('Are you sure you want to delete the block %name?', array('%name' => ''. $box['info'] .'')) ."
\n";
+ $form = '
' . t('Are you sure you want to delete the block %name?', array('%name' => '' . $box['info'] . '')) . "
\n";
I fixed them in CVS. It may suond like nitpicking, but it is important for readability and for doing searching on code.
Comment #26
andremolnar CreditAttribution: andremolnar commentedCaught a tiny but important mistake in the last db_patch.
Update_121 created additional field 'type' instead of 'types'.
This corrects that.
andre
Comment #27
Dries CreditAttribution: Dries commentedCommitted to HEAD.
Comment #28
(not verified) CreditAttribution: commented