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@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@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).