[drupal-devel] [feature] Add configuration option to show blocks
only on pages of certain node type
andremolnar
drupal-devel at drupal.org
Tue Jan 25 19:50:51 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/block_module_5.patch (5.75 KB)
Update.
New patch - implements node_load
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?
--
View: http://drupal.org/node/16074
Edit: http://drupal.org/project/comments/add/16074
More information about the drupal-devel
mailing list