[drupal-devel] [feature] Add configuration option to show blocks
only on pages of certain node type
andremolnar
drupal-devel at drupal.org
Thu Jan 27 17:02:21 UTC 2005
Project: Drupal
Version: cvs
Component: block.module
Category: feature requests
Priority: normal
Assigned to: andremolnar
Reported by: andremolnar
Updated by: andremolnar
Status: patch
Attachment: http://drupal.org/files/issues/blocks_db_change_4.patch (1.95 KB)
First 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
andremolnar
Previous comments:
------------------------------------------------------------------------
January 24, 2005 - 01:47 : andremolnar
Attachment: http://drupal.org/files/issues/block_module_1.patch (8.14 KB)
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
------------------------------------------------------------------------
January 24, 2005 - 01:48 : andremolnar
Attachment: http://drupal.org/files/issues/blocks_db_change.patch (1.98 KB)
Associated db changes required for this patch.
andre
------------------------------------------------------------------------
January 24, 2005 - 08:17 : FactoryJoe at civicspacelabs.org
I 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.
------------------------------------------------------------------------
January 24, 2005 - 10:17 : Steven
Chris, have you seen the block configuration in HEAD? Drumm developed
that patch...
------------------------------------------------------------------------
January 24, 2005 - 15:12 : Dries
The 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:
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.
------------------------------------------------------------------------
January 24, 2005 - 18:16 : andremolnar
Attachment: http://drupal.org/files/issues/block_module_2.patch (7.61 KB)
Okay - 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:
'string string string'. $foo .'string' OR
'string string string' . $foo . 'string'
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 ;-)
------------------------------------------------------------------------
January 24, 2005 - 18:19 : andremolnar
Attachment: http://drupal.org/files/issues/blocks_db_change_2.patch.txt (1.98 KB)
And the database patches - type now types
------------------------------------------------------------------------
January 25, 2005 - 07:15 : andremolnar
Attachment: http://drupal.org/files/issues/block_module_3.patch (7.62 KB)
Noticed another minor formatting problem in the last patch - I think I
need a new IDE ;-)
This fixes that.
andre
------------------------------------------------------------------------
January 25, 2005 - 09:22 : Dries
Some 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:
if ($node->type == $type) {
$type_match = TRUE;
break;
}
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.
------------------------------------------------------------------------
January 25, 2005 - 13:10 : Anonymous
in 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.
------------------------------------------------------------------------
January 25, 2005 - 17:34 : andremolnar
Re: anonymous: - MID in the block table.
No need for this. If the menu system produces a block then block.delta
== menu.mid
andre
------------------------------------------------------------------------
January 25, 2005 - 18:03 : andremolnar
Attachment: http://drupal.org/files/issues/block_module_4.patch (7.69 KB)
Updated patch with the nits picked out ;-)
in that context, we use $result but that variable name might already be
in use
Exactly - $result was already in use - But, I've now changed $foo to
$result_1
We try not to glue words together. For example: $nodetypes...
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)
Otherwise this patch looks perfect! Good job
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.
------------------------------------------------------------------------
January 25, 2005 - 19:29 : Dries
The patch doesn't apply:
patching file modules/block.module
patch unexpectedly ends in middle of line
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?
------------------------------------------------------------------------
January 25, 2005 - 19:50 : andremolnar
Attachment: http://drupal.org/files/issues/block_module_5.patch (5.75 KB)
Update.
New patch - implements node_load
andre
------------------------------------------------------------------------
January 25, 2005 - 20:08 : Dries
Saving the block configuration doesn't seem to work.
------------------------------------------------------------------------
January 25, 2005 - 20:50 : andremolnar
Attachment: http://drupal.org/files/issues/blocks_db_change_3.patch (1.95 KB)
Found 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
------------------------------------------------------------------------
January 25, 2005 - 21:39 : Dries
When 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 ...
------------------------------------------------------------------------
January 26, 2005 - 05:40 : andremolnar
Hmmmm...
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
------------------------------------------------------------------------
January 26, 2005 - 06:11 : andremolnar
dang.
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
------------------------------------------------------------------------
January 26, 2005 - 06:20 : andremolnar
Okay - demo site now working again.
I'm going to roll a new patch and make sure that I submitted the right
one.
andre
------------------------------------------------------------------------
January 26, 2005 - 18:55 : andremolnar
Attachment: http://drupal.org/files/issues/block_config_mockup.png (15.1 KB)
Following 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
------------------------------------------------------------------------
January 26, 2005 - 23:09 : Dries
Looks 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.
--
View: http://drupal.org/node/16074
Edit: http://drupal.org/project/comments/add/16074
More information about the drupal-devel
mailing list