April 2005 - Development - lists.drupal.org

[drupal-devel] [bug] Blocks with no title kill Drupal
by Steven 15 Apr '05

15 Apr '05

Issue status update for http://drupal.org/node/19965 Project: Drupal Version: cvs Component: block.module Category: bug reports Priority: critical Assigned to: Robin Monks Reported by: budda Updated by: Steven Status: patch Sorry for dropping in so late, but I disagree that block descriptions should be filled in. A while ago I even made a patch to display the block title in the admin list if the description was empty: I never fill in descriptions for my own blocks, I know what they are. It seems a big usability hit to force people to enter descriptions. Also, the updates seem to re-add the index on (title) but not on (info). I think the two versions (database.sql and updates.inc) are now out of sync. Steven Previous comments: ------------------------------------------------------------------------ April 4, 2005 - 19:03 : budda When setting more than one block's title to '' (blank) Drupal dies, and doesn't catch the SQL query error nicely. The error is as follows: user error: Duplicate entry '' for key 2 query: UPDATE boxes SET title = '', body = ' ...<cut>... in /var/www/html/drupal-cvs/includes/database.mysql.inc on line 66. Using 4.6 build from 4/4/2005 The page ends with: warning: Cannot modify header information - headers already sent by (output started at /var/www/html/drupal-cvs/includes/common.inc:384) in /var/www/html/drupal-cvs/includes/common.inc on line 192. ------------------------------------------------------------------------ April 8, 2005 - 10:08 : robertDouglass There should be a check before the insert is attempted and if the block title already exists the user should be notified with form_error(). ------------------------------------------------------------------------ April 8, 2005 - 11:46 : Poolio this certainly doesn't solve the problem, but I've worked around this issue by using html tags in the title field for blocks that I don't want to show a title. ------------------------------------------------------------------------ April 9, 2005 - 02:31 : Robin Monks Attachment: http://drupal.org/files/issues/block.module.blank.title.patch (955 bytes) A small patch that checks to ensure the title does not already exit in the Blocks list. Also, if the field was left blank, in adds an HTML comment with the current date to the second to ensure that a duplicate title isn't created. This is my first patch, so I probably did something wrong, but I'm willing to fix it too. Robin Monks MozNetwork.org ------------------------------------------------------------------------ April 9, 2005 - 03:22 : Morbus Iff I'm, personally, fine with how you've fixed the blank block title. You've really solved two problems in one: duplicate titles (which is the cause of this Issue), and the desire to have blank (or no) titles (which is the reason for this Issue) for the end user. With that said, I'm not a fan of your error message (but, then again, anal wording is my specialty, so don't think I'm picking on you): I'd rename it to "That block title is already in use; please choose another." The goals here: The user may not remember all the previous titles he's used. Using "unique" starts forcing him to remember. We shouldn't do that. The user already knows he entered it - we don't have to repeat that to him. It's active, not passive (no use of "have"). Use a semi-colon, not a comma. It's much shorter and concise. ------------------------------------------------------------------------ April 9, 2005 - 08:27 : chx Attachment: http://drupal.org/files/issues/box_title.patch (1.49 KB) Why display any message at all? ------------------------------------------------------------------------ April 9, 2005 - 16:11 : Morbus Iff Displaying no message at all would only work if the user wasn't typing in ANY title whatsoever (since then we'd make one for them based on the date). But, if a user names a block "Users" one week, and then creates another block called "Users" three weeks later, than an whoopsidaisy error needs to be shown - we shouldn't modify the text randomly. ------------------------------------------------------------------------ April 9, 2005 - 17:10 : chx Well, let's discuss the main point: why shoiuld be the title unique? ------------------------------------------------------------------------ April 9, 2005 - 17:38 : Morbus Iff Are you asking why Drupal enforces them being unique, or why, visually and to the end-user, they should be unique? ------------------------------------------------------------------------ April 9, 2005 - 17:43 : chx I guess Drupal forces them 'cos for some reasons it is thought that for the users they should be unique. This is not the case, however. Yes, it's confusing if they are on the same page. But I can think on circumstances where they would not be on the same page, so the force from Drupal seems unnecessary. ------------------------------------------------------------------------ April 9, 2005 - 21:00 : robertDouglass agree with chx on the unique point. Like the functionality of adding html comments and timestamp (why timestamp? why not a block id?) if left blank if they must be uniqe. Would be in favor of making the visible title completely optional. Administrators have description to keep them separate on the admin screen. ------------------------------------------------------------------------ April 9, 2005 - 21:48 : Robin Monks Attachment: http://drupal.org/files/issues/drupal_0.patch (3.67 KB) This new patch no longer requires a unique value for title, and also checks to ensure the Block Description is not a duplicate or empty value. Big thanks go out to chx for assistance. Robin ------------------------------------------------------------------------ April 11, 2005 - 16:20 : Anonymous Shouldn't we add a test too to check info unicity ? ------------------------------------------------------------------------ April 11, 2005 - 16:38 : chx <?php if (empty($edit['info']) || db_num_rows(db_query("SELECT info FROM {boxes} WHERE info='%s'", $edit['info']))) { ?> To me, this seems like a check for info unicity... not to mention that the UNIQUE(info) DB constraint is not touched. ------------------------------------------------------------------------ April 11, 2005 - 16:46 : tostinni Ups, sorry for the anonymous and the message, I didn't saw this verification, I was only remembering block.module code. That's all fine for me. ------------------------------------------------------------------------ April 11, 2005 - 18:27 : njivy +1 to Robin Monks' patch. When I install Drupal, the first thing I always do is remove the /unique key/ constraint on block titles. I have not tested the other parts of Monks' patch, though, including the uniqueness constraint on block descriptions. ------------------------------------------------------------------------ April 11, 2005 - 21:04 : drumm Code looks okay. +1 "You must enter unique data into the block description field." doesn't sound like the friendliest of error messages. ------------------------------------------------------------------------ April 11, 2005 - 21:16 : Dries I'm OK with this patch but suggest that we look into improving the error message. Small nit: fix the spacing around '=' in the SQL query that checks for duplicates. ------------------------------------------------------------------------ April 12, 2005 - 01:40 : Robin Monks Attachment: http://drupal.org/files/issues/drupal_1.patch (3.97 KB) Here is a new patch with the changes and with the updates.inc file changed to reflect patchs applied after the original patch was made. Robin ------------------------------------------------------------------------ April 12, 2005 - 20:52 : Dries Committed to HEAD. Thanks. The patch did not apply against DRUPAL-4-6. Possibly needs to get backported. ------------------------------------------------------------------------ April 13, 2005 - 17:36 : Robin Monks Attachment: http://drupal.org/files/issues/4.6 blank title (3.63 KB) Here is the patch backported for 4.6. Robin MozNetwork.org ------------------------------------------------------------------------ April 13, 2005 - 17:45 : Robin Monks Attachment: http://drupal.org/files/issues/4_0.6 blank title (3.62 KB) That patch had a bug in it, new patch attached. Robin ------------------------------------------------------------------------ April 14, 2005 - 13:38 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/block-fix.patch (673 bytes) The patch got apparently into core. It introduces a problem with php 5. $edit is not defined as an array if the form is called for the fiest time (default in switch). This patch fixes this.

1 0

[drupal-devel] [feature] New hook, header to allow pre-header module functions
by RobinMonks 15 Apr '05

15 Apr '05

Issue status update for http://drupal.org/node/20562 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: RobinMonks(a)moznetwork.org Reported by: RobinMonks(a)moznetwork.org Updated by: RobinMonks(a)moznetwork.org Status: patch Attachment: http://drupal.org/files/issues/header hook.inc.patch (612 bytes) One function that is needed in Drupal (and that our modules currently don't have), is access to write headers. This patch aims to fix that, by providing a hook to send headers before the main header for Drupal is sent. This will also help with modules such as securesite, by allowing HTTP Auth to be called before we sent content type headers. Robin Monks MozNetwork.org RobinMonks(a)moznetwork.org

2 1

[drupal-devel] [bug] Fix a serious bug and clean up teaser generation
by Goba 15 Apr '05

15 Apr '05

Issue status update for http://drupal.org/node/18836 Project: Drupal Version: cvs Component: node system Category: bug reports Priority: normal Assigned to: Goba Reported by: Goba Updated by: Goba Status: patch Attachment: http://drupal.org/files/issues/Drupal.teaser-bugfix-and-cleanup.patch (4.48 KB) The node teaser generator keeps the whole body as the teaser, if there is indication that PHP code is included in the body (since it is unkown, what is going to come out of the PHP parser). The only serious problem is that it does not check if the PHP filter is included in the input format, so it does keep the complete body even if there is no PHP parsing on the code. This breaks PHP code examples in articles or blog posts, which are not supposed to be parsed, but which require proper teasers. The solution is to make the teaser generation code aware of the input format (pass the whole node), and let it provide the teaser that way, depending on the input format. The attached patch also cleans up that very unprofessionaly looking sequence of copy-pasted stuff in the node_teaser function. Goba

5 25

[drupal-devel] [bug] Free tagging doesn't support taxonomy_node_form $name
by Morbus Iff 15 Apr '05

15 Apr '05

Issue status update for http://drupal.org/node/20561 Project: Drupal Version: cvs Component: taxonomy.module Category: bug reports Priority: normal Assigned to: Morbus Iff Reported by: Morbus Iff Updated by: Morbus Iff Status: patch Attachment: http://drupal.org/files/issues/_p_freetagname.patch (1.12 KB) My free tagging patch doesn't correctly support taxonomy_node_form's $name parameter. The attached patch fixes that. No core code trips up on this (as no code actually /uses/ $name) but moshe's taxononmy_multi_edit.module does. Thanks to moshe for pointing this out. Morbus Iff

1 0

[drupal-devel] Re: [contributions:mathias] /modules/tinymce INSTALL.txt
by Brian Hay 15 Apr '05

15 Apr '05

drupal-cvs(a)drupal.org wrote: > Log message: > * This module only works the CVS version of TinyMCE. See this file for more details. What has changed with this version? Readme doesn't say. I'm having a problem with 1.43 (issue here: http://drupal.org/node/20478) Although TinyMCE docs and forum seem to imply that it creates <strong> and <em> instead of <b> and <i>, I am still getting the deprecated tags. I've created a drupal input filter as advised in the readme but this didn't prevent the problem. Any ideas? -- Regards, Brian.

2 1

[drupal-devel] Proposal: change "fixed" status to mean "ready to commit"
by Nedjo Rogers 14 Apr '05

14 Apr '05

A patch I proposed nine weeks ago to improve project the project issue review process went nowhere. Issue: http://www.drupal.org/node/17052 Patch: http://drupal.org/files/issues/admin_configurable_status_options.patch But (not to be deterred, since I still believe barriers to community participation in the development process are the most pressing issues in Drupal development) I have another, much more modest suggestion, that I think could help move things along without (gasp) changing code. The problem: there is no way for qualified Drupal developers to push patches up the queue, or in other words to designate a patch as meeting acceptance criteria. Patches sit at "patch" status indefinitely, waiting until one of the three CVS committers personally reviews and accepts (or denies) them, setting the status then to "fixed" (or "won't fix"), which in many cases never happens. My proposal: that we change "fixed" to mean "ready to commit" or "meets acceptance criteria (i.e., works as promised and is a worthwhile change fitting with Drupal aims and principles)". That way, qualified developers can designate a patch as ready to apply. If it is applied, the CVS committer would set the status to "closed". This change would be consistent with e.g., bugzilla on mozilla, where "fixed" is set before a change is applied, and preceeds final review and application. A full solution? No. A valuable start, and much better than doing nothing? I think, yes. -------- Nedjo Rogers Learning Technologies Developer GroundWorks Learning Centre (250) 360-0799 nedjo(a)gworks.ca

3 3

[drupal-devel] OT: Chronicle of Philanthropy: Events Seek to 'Demystify' Open-Source Software
by Chris Messina 14 Apr '05

14 Apr '05

There was a nice write-up in the Chronicle of Philanthropy about the Bay Area PenguinDay. It can be found at http://philanthropy.com/premium/articles/v17/i12/12004801.htm, and for those without a subscription, it's posted below. During the SpeedGeek session (http://penguinday.blueoxen.net/cgi-bin/wiki.pl?CivicSpaceSpeedGeekingSession) I presented CivicSpace and talked up Drupal to non-profits who were very excited about its potential for their organizations. I also wrote up a brief blog entry about this: http://civicspacelabs.org/home/blog/chris/attending-penguinday Chris Events Seek to 'Demystify' Open-Source Software By Nicole Wallace A series of events this spring will bring together charity technology employees, consultants, and software developers to discuss the potential -- and the challenges -- of developing open-source software. "We want to demystify open-source software for nonprofit organizations," says Katrin Verclas, co-director of Aspiration, an Amherst, Mass., nonprofit group that seeks to create a software landscape in which charities have access to good-quality, low-cost tools that help them do their work. Open-source software gives users access to the "source code" of an application, which allows organizations to make changes or enhancements to fit their needs. Developers of such software encourage users to modify the programs and to share them with others. Aspiration is working with local technology charities to sponsor the gatherings, called Penguin Days after the mascot of the open-source giant Linux. Penguin Day Chicago took place on March 26, and events are scheduled in the San Francisco Bay Area on April 12, and in New York on May 7. Many nonprofit organizations have very specific software needs, but the market of potential customers that share those needs is too small for commercial software companies to build those products, says Ms. Verclas. She says charities that need to develop such specialized tools can cut their costs by adapting open-source software, rather than starting from scratch. The transparency of open-source software code can also be vital to organizations that handle sensitive information, such as international groups investigating human-rights abuses. "With open source, you can look at the code and see if there are any back doors someone could use to try to get in," says Ms. Verclas. But significant barriers keep nonprofit organizations from widely adopting open-source software. Charities' limited capacity to develop software and open-source software's tendency to be less well-documented and user-friendly than proprietary software have combined to make open source a daunting choice for nonprofit organizations, says Ms. Verclas. For nonprofit groups to be able to take advantage of the promise of open-source software, she says, technology organizations will have to develop, enhance, and support open-source tools for charities. For more information: Go to http://www.penguinday.org.

1 0

[drupal-devel] [feature] View blocks by roles
by tostinni 14 Apr '05

14 Apr '05

Issue status update for http://drupal.org/node/12988 Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: normal Assigned to: tostinni Reported by: tostinni Updated by: tostinni Status: patch In fact this was the way how was working my first patch, but then drumm advice me to go ahead with the block_visibility table, that's done with last version. Any more improvement ? tostinni Previous comments: ------------------------------------------------------------------------ November 15, 2004 - 17:35 : tostinni Attachment: http://drupal.org/files/issues/block_0_0.patch (5.45 KB) After the release of drumm's work about new block.module in CVS : Make block configuration understandable [1] and another comment : Set blocks to be hidden from anonymous users. [2], I decided to create a little patch to allow use of 'roles' for 'blocks'. So my patch provide a set of checkboxes in block configure (the new field drumm has setting up) that allow roles to see a block. Way it works : 1. If no roles set, block is visible by everybody. If some roles are set, only users with at least one of the roles can see the block. 2. Database : As I'm not sure this block will be included in core because a team is working on it (see Drupal 4.6 roadmap [3] - I will post a comment on civiclabs site), I just set up a new field 'roles' in blocks table. So when you modify the roles of a block, it just update this field with a concatenation of roles_id separated by comma. I'm conscient it's quite ugly, but if this patch find his public, I was thinking in creating a new table block_roles (as done for user_roles). Also update.inc isn't updated for the reasons mentioned above. Well I hope your comments. [1] http://drupal.org/node/11875 [2] http://drupal.org/node/12168 [3] http://drupal.org/node/12202 ------------------------------------------------------------------------ November 15, 2004 - 19:55 : drumm You use user_roles(1) which, if I understand that function correctly, removes the anonymous user role from the list. I think that this should be included. Use case: you want information for non-logged in users which is useless to those who are logged in. Watch your editor's use of tabs instead of spaces. It can and should be included. I haven't started work on this, and it saves me a lot of time if I have patches such as this to review. Why not have all the boxes checked by default to indicate visible to everyone instead of none checked? I would even support adding a validate step to make sure at least one is checked (probably belongs in another patch). I think another table is the way to go. Then you could even write the test for visibility by role to that query in block_list() instead of a loop in php. ------------------------------------------------------------------------ November 15, 2004 - 20:49 : tostinni Hi, Thanks for your feedback. Well a few mistakes (tabs, my editor is now configured, and user_roles(0)) I will correct in next patch. A little comment : for the moment I choosed NULL value for everybody if not, we face a few problems : - if the admin add a new role, it won't be asigned the right to view blocks with the "everybody" grant. - also if people import their blocks or if they apply this patch with a site containing blocks settings, they would be invisble unless explicitly allowed. That's why I considered this option, but if you prefer, I can select all checkboxes. For the validate step, I was thinking of it, but for the moment and the design, It was useless. Regarding of the table, the only issue I was contempling, was the lack of a block_id in blocks table, so I would have to create one. Are you ok with that ? ------------------------------------------------------------------------ March 13, 2005 - 15:37 : killes(a)www.drop.org New patch needed nased on drumm's feedback. ------------------------------------------------------------------------ April 11, 2005 - 19:02 : tostinni Attachment: http://drupal.org/files/issues/block_visibility.patch (8.09 KB) Hi, Sorry for the delay, here is the new patch based on current CVS. *Observations :* - I created a new table : block_visibility that store the visibility for each role / block When you update db scheme it would make visible blocks for all your availables roles, i.e. it changes nothing ;), just set up correctly permissions. - When you configure blocks, you will see a set of checkboxes to allow block to be visible regarding of the user's roles. If no role checked, nobody will see it. *Know bugs (by design ?) :* - When a custom block is created it receives no permissions, it means that it would be invisible unless you configure role in "configure" screen. I'm aware of this problem, but I still don't now how to deal with it. The major issue is that I can't predict which id will be inserted in 'boxes' table as it works with auto_increment / SERIAL columns instead of Drupal own sequence table. We can work around it, but I'm not sure it's a good solution to change this, it would cause a lot of troubles... Maybe look for a db_insert_id function ? Any ideas ? For the moment, the user have to grant permissions to role to be able to see custom block. *To do :* - Add visibility rehash in _block_rehash function. - Change help messages ? - Does my queries look nice ? (It works for sure, but is it a good way to write a join : FROM blocks b LEFT JOIN role r ON 1 = 1 ? ) - Others suggestions ? ------------------------------------------------------------------------ April 12, 2005 - 13:59 : drumm "When a custom block is created it receives no permissions, it means that it would be invisible unless you configure role in "configure" screen. " It should probably display the checkboxes for permissions on the block creation screen. All checkboxes check by default. "The major issue is that I can't predict which id will be inserted in 'boxes' table as it works with auto_increment / SERIAL columns instead of Drupal own sequence table. " It really should use Drupal's sequesnces table. You can safely ignore any auto increment or SERIAL. Just use db_next_id as usual. "- Does my queries look nice ? (It works for sure, but is it a good way to write a join : FROM blocks b LEFT JOIN role r ON 1 = 1 ? ) " I've never seen the INSERT ... SELECT syntax elsewhere in Drupal. This may mean that it is not Postgres-compatible or it is avaided for other reasons. A real join condition should be used, such as r.rid = v.rid. ------------------------------------------------------------------------ April 12, 2005 - 18:51 : tostinni "It should probably display the checkboxes for permissions on the block creation screen. All checkboxes check by default. " I was thinking of this issue, it's just that I didn't find out how to display it without redundancy between form's groups from block_box_form and from block_block, it's done now. "It really should use Drupal's sequesnces table. You can safely ignore any auto increment or SERIAL. Just use db_next_id as usual. " Ok, I will make the changes. "I've never seen the INSERT ... SELECT syntax elsewhere in Drupal. This may mean that it is not Postgres-compatible or it is avaided for other reasons. " Well INSERT ... SELECT syntax is part of SQL ANSI 92 and fully implemented in Postgre. I couldn't find since when, but at least since 6.4 version [4] of Postgre. So I think it's good enough. And maybe if no drupal developer ever used it, it's maybe they never needed it ;) Btw, if there's any limitation not to use it, who can tell me ? "A real join condition should be used, such as r.rid = v.rid. " I've done this on purpose : - If I'm modifying the block options and a new role has been added posterior to the block. Now I got a sort of inconsistance in my block_visibility table, because I miss this new role_id for the (module, delta) tuple. So when I'm reviewing/modifying my block configuration, I need to detect this inconsistence to be able to add this new (module, delta, rid) tuple in block_visibility. This is made doing a JOIN between role and blocks with this strange condition : 1=1 because they don't have a commun column. The other reason I've done it this way is because my MySQL documentation told me to prefer LEFT JOIN to RIGHT JOIN for compatibility. What do you think about it ? Should I use RIGHT JOIN without fear ? It would gives us : SELECT r.rid, v.visible FROM blocks b LEFT JOIN block_visibility v ON v.module = b.module AND v.delta = b.delta RIGHT OUTER JOIN role r ON r.rid = v.rid WHERE b.module = '%s' AND b.delta = '%s' and works the same way. Tomorrow I'll post my changes. [4] http://www.postgresql.org/docs/6.4/static/sql-insert.htm ------------------------------------------------------------------------ April 13, 2005 - 18:47 : tostinni Attachment: http://drupal.org/files/issues/block_visibility_0.patch (9.95 KB) Ok here is the new version including these changes : - based on latest CVS version - I created a new function _block_roles_save to handle saving roles - when a new custom block is created, it automatically selects all availables roles (user can change it) and save it - it also handles drupal sequence system for boxes table to allow insertion of the roles next to the custom block creation Still pending : make a _block_roles_rehash. Well I'm not very sure about this one, because it would generate a lot of queries : number of rows in blocks table x num in role _block_rehash works this way, but is it reasonnable ? If _block_rehash is needed not to display blocks that doesn't exists anymore, _block_roles_rehash is only here to clean the block_visibility table, but there won't be any problem with new or missing roles, block would just respect current roles. What should I do ? ------------------------------------------------------------------------ April 14, 2005 - 05:07 : Bèr Kessels Nice feauture. Nice interface, no clutter. But a big -1 on the way you use the database. This can be done much easier by adding a single column in the existing block table. Why introduce a new table and a lot of extra sql queries, update qeuries etc? Bèr ------------------------------------------------------------------------ April 14, 2005 - 05:13 : killes(a)www.drop.org Ber: I am not sure how you do want to add visibility per role through a single column in the existing table. Can you elaborate? What I'd like to see would be block IDs that aren't only defined through the modules. Ie like the menu module handles them. ------------------------------------------------------------------------ April 14, 2005 - 11:53 : drumm The data would not fit in one column, we would end up having serialized or comma separated values which would require parsing by regexp or expansion in php on block viewing. Normalizing data in the database is always better (except for caching).

1 0

[drupal-devel] Detecting table existence/absence in a Drupally-correct way
by Syscrusher 14 Apr '05

14 Apr '05

Good evening! Is there a well-behaved way to use Drupal's DB abstraction to find out if a table exists? I'm working on a migration feature, and need to be able to detect the existence of the table so that I can issue a user-friendly "you don't need to do this step" message if the table isn't there. The obvious thing to try was calling @db_query() to try to suppress the error message. That does what I need, but puts the database error into the output shown to the user. Since I know what is the specific situation, I want to suppress that output for this call only, without overriding the site-wide setting that causes errors to go only into the log. (Ideally, this particular error doesn't belong in the log, either, since I essentially am generating it on purpose). It might be worth adding a new parameter to db_query() and db_queryd() in the HEAD version to allow this sort of error suppression, in cases like mine where the calling code wants to explicitly take responsibility for error handling. And I know this isn't a trivial thing across databases, but..."Boy, it sure would be nice if there was a way to ask Drupal for a list of names of all the tables that exist in the database." :-) I think I have some code lying about that does this (though not in the Drupal environment) which worked with at least PostgreSQL and MySQL and (if I recall) ODBC. If it's of interest to someone, I'll try to dig it up and look at porting it into Drupal. Before I put in that effort, though, I'd like to have some indication of whether or not it was likely that kind of code would be accepted into core. Comments/suggestions welcome. Scott -- ------------------------------------------------------------------------------- Scott Courtney Drupal user name: "syscrusher" http://drupal.org/user/9184 scott(a)4th.com Drupal projects: http://drupal.org/project/user/9184 Sandbox: http://cvs.drupal.org/viewcvs/drupal/contributions/sandbox/syscrusher

5 8

[drupal-devel] [feature] View blocks by roles
by drumm 14 Apr '05

14 Apr '05

Issue status update for http://drupal.org/node/12988 Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: normal Assigned to: tostinni Reported by: tostinni Updated by: drumm Status: patch The data would not fit in one column, we would end up having serialized or comma separated values which would require parsing by regexp or expansion in php on block viewing. Normalizing data in the database is always better (except for caching). drumm Previous comments: ------------------------------------------------------------------------ November 15, 2004 - 16:35 : tostinni Attachment: http://drupal.org/files/issues/block_0_0.patch (5.45 KB) After the release of drumm's work about new block.module in CVS : Make block configuration understandable [1] and another comment : Set blocks to be hidden from anonymous users. [2], I decided to create a little patch to allow use of 'roles' for 'blocks'. So my patch provide a set of checkboxes in block configure (the new field drumm has setting up) that allow roles to see a block. Way it works : 1. If no roles set, block is visible by everybody. If some roles are set, only users with at least one of the roles can see the block. 2. Database : As I'm not sure this block will be included in core because a team is working on it (see Drupal 4.6 roadmap [3] - I will post a comment on civiclabs site), I just set up a new field 'roles' in blocks table. So when you modify the roles of a block, it just update this field with a concatenation of roles_id separated by comma. I'm conscient it's quite ugly, but if this patch find his public, I was thinking in creating a new table block_roles (as done for user_roles). Also update.inc isn't updated for the reasons mentioned above. Well I hope your comments. [1] http://drupal.org/node/11875 [2] http://drupal.org/node/12168 [3] http://drupal.org/node/12202 ------------------------------------------------------------------------ November 15, 2004 - 18:55 : drumm You use user_roles(1) which, if I understand that function correctly, removes the anonymous user role from the list. I think that this should be included. Use case: you want information for non-logged in users which is useless to those who are logged in. Watch your editor's use of tabs instead of spaces. It can and should be included. I haven't started work on this, and it saves me a lot of time if I have patches such as this to review. Why not have all the boxes checked by default to indicate visible to everyone instead of none checked? I would even support adding a validate step to make sure at least one is checked (probably belongs in another patch). I think another table is the way to go. Then you could even write the test for visibility by role to that query in block_list() instead of a loop in php. ------------------------------------------------------------------------ November 15, 2004 - 19:49 : tostinni Hi, Thanks for your feedback. Well a few mistakes (tabs, my editor is now configured, and user_roles(0)) I will correct in next patch. A little comment : for the moment I choosed NULL value for everybody if not, we face a few problems : - if the admin add a new role, it won't be asigned the right to view blocks with the "everybody" grant. - also if people import their blocks or if they apply this patch with a site containing blocks settings, they would be invisble unless explicitly allowed. That's why I considered this option, but if you prefer, I can select all checkboxes. For the validate step, I was thinking of it, but for the moment and the design, It was useless. Regarding of the table, the only issue I was contempling, was the lack of a block_id in blocks table, so I would have to create one. Are you ok with that ? ------------------------------------------------------------------------ March 13, 2005 - 14:37 : killes(a)www.drop.org New patch needed nased on drumm's feedback. ------------------------------------------------------------------------ April 11, 2005 - 18:02 : tostinni Attachment: http://drupal.org/files/issues/block_visibility.patch (8.09 KB) Hi, Sorry for the delay, here is the new patch based on current CVS. *Observations :* - I created a new table : block_visibility that store the visibility for each role / block When you update db scheme it would make visible blocks for all your availables roles, i.e. it changes nothing ;), just set up correctly permissions. - When you configure blocks, you will see a set of checkboxes to allow block to be visible regarding of the user's roles. If no role checked, nobody will see it. *Know bugs (by design ?) :* - When a custom block is created it receives no permissions, it means that it would be invisible unless you configure role in "configure" screen. I'm aware of this problem, but I still don't now how to deal with it. The major issue is that I can't predict which id will be inserted in 'boxes' table as it works with auto_increment / SERIAL columns instead of Drupal own sequence table. We can work around it, but I'm not sure it's a good solution to change this, it would cause a lot of troubles... Maybe look for a db_insert_id function ? Any ideas ? For the moment, the user have to grant permissions to role to be able to see custom block. *To do :* - Add visibility rehash in _block_rehash function. - Change help messages ? - Does my queries look nice ? (It works for sure, but is it a good way to write a join : FROM blocks b LEFT JOIN role r ON 1 = 1 ? ) - Others suggestions ? ------------------------------------------------------------------------ April 12, 2005 - 12:59 : drumm "When a custom block is created it receives no permissions, it means that it would be invisible unless you configure role in "configure" screen. " It should probably display the checkboxes for permissions on the block creation screen. All checkboxes check by default. "The major issue is that I can't predict which id will be inserted in 'boxes' table as it works with auto_increment / SERIAL columns instead of Drupal own sequence table. " It really should use Drupal's sequesnces table. You can safely ignore any auto increment or SERIAL. Just use db_next_id as usual. "- Does my queries look nice ? (It works for sure, but is it a good way to write a join : FROM blocks b LEFT JOIN role r ON 1 = 1 ? ) " I've never seen the INSERT ... SELECT syntax elsewhere in Drupal. This may mean that it is not Postgres-compatible or it is avaided for other reasons. A real join condition should be used, such as r.rid = v.rid. ------------------------------------------------------------------------ April 12, 2005 - 17:51 : tostinni "It should probably display the checkboxes for permissions on the block creation screen. All checkboxes check by default. " I was thinking of this issue, it's just that I didn't find out how to display it without redundancy between form's groups from block_box_form and from block_block, it's done now. "It really should use Drupal's sequesnces table. You can safely ignore any auto increment or SERIAL. Just use db_next_id as usual. " Ok, I will make the changes. "I've never seen the INSERT ... SELECT syntax elsewhere in Drupal. This may mean that it is not Postgres-compatible or it is avaided for other reasons. " Well INSERT ... SELECT syntax is part of SQL ANSI 92 and fully implemented in Postgre. I couldn't find since when, but at least since 6.4 version [4] of Postgre. So I think it's good enough. And maybe if no drupal developer ever used it, it's maybe they never needed it ;) Btw, if there's any limitation not to use it, who can tell me ? "A real join condition should be used, such as r.rid = v.rid. " I've done this on purpose : - If I'm modifying the block options and a new role has been added posterior to the block. Now I got a sort of inconsistance in my block_visibility table, because I miss this new role_id for the (module, delta) tuple. So when I'm reviewing/modifying my block configuration, I need to detect this inconsistence to be able to add this new (module, delta, rid) tuple in block_visibility. This is made doing a JOIN between role and blocks with this strange condition : 1=1 because they don't have a commun column. The other reason I've done it this way is because my MySQL documentation told me to prefer LEFT JOIN to RIGHT JOIN for compatibility. What do you think about it ? Should I use RIGHT JOIN without fear ? It would gives us : SELECT r.rid, v.visible FROM blocks b LEFT JOIN block_visibility v ON v.module = b.module AND v.delta = b.delta RIGHT OUTER JOIN role r ON r.rid = v.rid WHERE b.module = '%s' AND b.delta = '%s' and works the same way. Tomorrow I'll post my changes. [4] http://www.postgresql.org/docs/6.4/static/sql-insert.htm ------------------------------------------------------------------------ April 13, 2005 - 17:47 : tostinni Attachment: http://drupal.org/files/issues/block_visibility_0.patch (9.95 KB) Ok here is the new version including these changes : - based on latest CVS version - I created a new function _block_roles_save to handle saving roles - when a new custom block is created, it automatically selects all availables roles (user can change it) and save it - it also handles drupal sequence system for boxes table to allow insertion of the roles next to the custom block creation Still pending : make a _block_roles_rehash. Well I'm not very sure about this one, because it would generate a lot of queries : number of rows in blocks table x num in role _block_rehash works this way, but is it reasonnable ? If _block_rehash is needed not to display blocks that doesn't exists anymore, _block_roles_rehash is only here to clean the block_visibility table, but there won't be any problem with new or missing roles, block would just respect current roles. What should I do ? ------------------------------------------------------------------------ April 14, 2005 - 04:07 : Bèr Kessels Nice feauture. Nice interface, no clutter. But a big -1 on the way you use the database. This can be done much easier by adding a single column in the existing block table. Why introduce a new table and a lot of extra sql queries, update qeuries etc? Bèr ------------------------------------------------------------------------ April 14, 2005 - 04:13 : killes(a)www.drop.org Ber: I am not sure how you do want to add visibility per role through a single column in the existing table. Can you elaborate? What I'd like to see would be block IDs that aren't only defined through the modules. Ie like the menu module handles them.

1 0