[drupal-devel] [feature] Add configuration option to show blocks only on pages of certain node type
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_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 andremolnar -- View: http://drupal.org/node/16074 Edit: http://drupal.org/project/comments/add/16074
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.patch (1.98 KB) Associated db changes required for this patch. 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 -- View: http://drupal.org/node/16074 Edit: http://drupal.org/project/comments/add/16074
Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: normal Assigned to: andremolnar Reported by: andremolnar Updated by: FactoryJoe@civicspacelabs.org Status: patch 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. FactoryJoe@civicspacelabs.org Previous comments: ------------------------------------------------------------------------ January 23, 2005 - 17: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 23, 2005 - 17:48 : andremolnar Attachment: http://drupal.org/files/issues/blocks_db_change.patch (1.98 KB) Associated db changes required for this patch. andre -- View: http://drupal.org/node/16074 Edit: http://drupal.org/project/comments/add/16074
Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: normal Assigned to: andremolnar Reported by: andremolnar Updated by: Steven Status: patch Chris, have you seen the block configuration in HEAD? Drumm developed that patch... Steven Previous comments: ------------------------------------------------------------------------ January 24, 2005 - 02: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 - 02: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 - 09:17 : FactoryJoe@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. -- View: http://drupal.org/node/16074 Edit: http://drupal.org/project/comments/add/16074
Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: normal Assigned to: andremolnar Reported by: andremolnar Updated by: Dries Status: patch 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. Dries Previous comments: ------------------------------------------------------------------------ January 24, 2005 - 03: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 - 03: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 - 10:17 : FactoryJoe@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 - 12:17 : Steven Chris, have you seen the block configuration in HEAD? Drumm developed that patch... -- View: http://drupal.org/node/16074 Edit: http://drupal.org/project/comments/add/16074
Dries wrote:
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 gave this idea a moment of thought. It would be easy enough to introduce 'display block only for the following roles' - but I'm not entirely sure how one would go about setting permissions for this. I don't think that just anyone allowed to 'administer blocks' should be given the power to decide who (what roles) gets to see blocks. So I figure that you would have to grant permissions to a user to be able to see that specific configuration option within the block configuration page. So, being a complete drupal permissions newbie, I suppose my question is - what would be the best way to go about doing this? Would adding a local task to the configuration page for this specific setting be the way to go? And add a perm for 'administer block visibility by role' (or something similar) and control access to the local task based on that permission? Let me know if that sounds right - or point me in a better direction and I will see what I can do about baking a patch for this feature. (No better way to learn about something than writing some code for it ;-)) andre
Andre Molnar wrote:
I don't think that just anyone allowed to 'administer blocks' should be given the power to decide who (what roles) gets to see blocks. So I figure that you would have to grant permissions to a user to be able to see that specific configuration option within the block configuration page.
The 'administer blocks' permission should be enough. I wouldn't worry about it. Filters have role-specific settings too, and so has posting nodes when node-level permissions are enabled, to name two examples. -- Dries Buytaert :: http://www.buytaert.net/
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_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 ;-) 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@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. -- View: http://drupal.org/node/16074 Edit: http://drupal.org/project/comments/add/16074
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_2.patch.txt (1.98 KB) And the database patches - type now types 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@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 ;-) -- View: http://drupal.org/node/16074 Edit: http://drupal.org/project/comments/add/16074
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_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 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@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 -- View: http://drupal.org/node/16074 Edit: http://drupal.org/project/comments/add/16074
Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: normal Assigned to: andremolnar Reported by: andremolnar Updated by: Dries Status: patch 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. Dries Previous comments: ------------------------------------------------------------------------ January 24, 2005 - 03: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 - 03: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 - 10:17 : FactoryJoe@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 - 12:17 : Steven Chris, have you seen the block configuration in HEAD? Drumm developed that patch... ------------------------------------------------------------------------ January 24, 2005 - 17: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 - 20: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 - 20: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 - 09: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 -- View: http://drupal.org/node/16074 Edit: http://drupal.org/project/comments/add/16074
Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: normal Assigned to: andremolnar Reported by: andremolnar Updated by: Anonymous Status: patch 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. Anonymous 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@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. -- View: http://drupal.org/node/16074 Edit: http://drupal.org/project/comments/add/16074
Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: normal Assigned to: andremolnar Reported by: andremolnar Updated by: andremolnar Status: patch Re: anonymous: - MID in the block table. No need for this. If the menu system produces a block then block.delta == menu.mid 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@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. -- View: http://drupal.org/node/16074 Edit: http://drupal.org/project/comments/add/16074
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_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. 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@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 -- View: http://drupal.org/node/16074 Edit: http://drupal.org/project/comments/add/16074
Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: normal Assigned to: andremolnar Reported by: andremolnar Updated by: Dries Status: patch 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? Dries Previous comments: ------------------------------------------------------------------------ January 24, 2005 - 03: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 - 03: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 - 10:17 : FactoryJoe@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 - 12:17 : Steven Chris, have you seen the block configuration in HEAD? Drumm developed that patch... ------------------------------------------------------------------------ January 24, 2005 - 17: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 - 20: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 - 20: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 - 09: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 - 11: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 - 15: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 - 19: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 - 20: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. -- View: http://drupal.org/node/16074 Edit: http://drupal.org/project/comments/add/16074
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@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
Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: normal Assigned to: andremolnar Reported by: andremolnar Updated by: Dries Status: patch Saving the block configuration doesn't seem to work. Dries Previous comments: ------------------------------------------------------------------------ January 24, 2005 - 03: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 - 03: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 - 10:17 : FactoryJoe@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 - 12:17 : Steven Chris, have you seen the block configuration in HEAD? Drumm developed that patch... ------------------------------------------------------------------------ January 24, 2005 - 17: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 - 20: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 - 20: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 - 09: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 - 11: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 - 15: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 - 19: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 - 20: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 - 21: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 - 21:50 : andremolnar Attachment: http://drupal.org/files/issues/block_module_5.patch (5.75 KB) Update. New patch - implements node_load andre -- View: http://drupal.org/node/16074 Edit: http://drupal.org/project/comments/add/16074
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_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 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@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. -- View: http://drupal.org/node/16074 Edit: http://drupal.org/project/comments/add/16074
Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: normal Assigned to: andremolnar Reported by: andremolnar Updated by: Dries Status: patch 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 ... Dries Previous comments: ------------------------------------------------------------------------ January 24, 2005 - 03: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 - 03: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 - 10:17 : FactoryJoe@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 - 12:17 : Steven Chris, have you seen the block configuration in HEAD? Drumm developed that patch... ------------------------------------------------------------------------ January 24, 2005 - 17: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 - 20: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 - 20: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 - 09: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 - 11: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 - 15: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 - 19: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 - 20: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 - 21: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 - 21: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 - 22:08 : Dries Saving the block configuration doesn't seem to work. ------------------------------------------------------------------------ January 25, 2005 - 22: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 -- View: http://drupal.org/node/16074 Edit: http://drupal.org/project/comments/add/16074
Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: normal Assigned to: andremolnar Reported by: andremolnar Updated by: andremolnar Status: patch 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 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@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 ... -- View: http://drupal.org/node/16074 Edit: http://drupal.org/project/comments/add/16074
Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: normal Assigned to: andremolnar Reported by: andremolnar Updated by: andremolnar Status: patch 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 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@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 -- View: http://drupal.org/node/16074 Edit: http://drupal.org/project/comments/add/16074
Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: normal Assigned to: andremolnar Reported by: andremolnar Updated by: andremolnar Status: patch Okay - demo site now working again. I'm going to roll a new patch and make sure that I submitted the right one. 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@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 -- View: http://drupal.org/node/16074 Edit: http://drupal.org/project/comments/add/16074
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_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 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@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 -- View: http://drupal.org/node/16074 Edit: http://drupal.org/project/comments/add/16074
Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: normal Assigned to: andremolnar Reported by: andremolnar Updated by: Dries Status: patch 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. Dries Previous comments: ------------------------------------------------------------------------ January 24, 2005 - 03: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 - 03: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 - 10:17 : FactoryJoe@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 - 12:17 : Steven Chris, have you seen the block configuration in HEAD? Drumm developed that patch... ------------------------------------------------------------------------ January 24, 2005 - 17: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 - 20: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 - 20: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 - 09: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 - 11: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 - 15: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 - 19: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 - 20: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 - 21: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 - 21: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 - 22:08 : Dries Saving the block configuration doesn't seem to work. ------------------------------------------------------------------------ January 25, 2005 - 22: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 - 23: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 - 07: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 - 08: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 - 08: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 - 20: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 -- View: http://drupal.org/node/16074 Edit: http://drupal.org/project/comments/add/16074
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@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
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_6.patch (8.46 KB) Updated 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 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@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. ------------------------------------------------------------------------ January 27, 2005 - 17:02 : andremolnar 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 -- View: http://drupal.org/node/16074 Edit: http://drupal.org/project/comments/add/16074
Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: normal Assigned to: andremolnar Reported by: andremolnar Updated by: andremolnar -Status: fixed +Status: patch Attachment: http://drupal.org/files/issues/update_inc_121_1.patch (569 bytes) Caught a tiny but important mistake in the last db_patch. Update_121 created additional field 'type' instead of 'types'. This corrects that. 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@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. ------------------------------------------------------------------------ January 27, 2005 - 17:02 : andremolnar 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 ------------------------------------------------------------------------ January 27, 2005 - 17:14 : andremolnar Attachment: http://drupal.org/files/issues/block_module_6.patch (8.46 KB) Updated 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 ------------------------------------------------------------------------ January 27, 2005 - 19:40 : Dries Committed to HEAD. Thanks. (I made some minor changes to the block configuration page.) ------------------------------------------------------------------------ January 27, 2005 - 19:55 : Steven Please 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. -- View: http://drupal.org/node/16074 Edit: http://drupal.org/project/comments/add/16074
participants (7)
-
Andre Molnar -
andremolnar -
Anonymous
-
Dries
-
Dries Buytaert -
FactoryJoe
-
Steven