[drupal-devel] [feature] View blocks by roles
tostinni
drupal-devel at drupal.org
Thu Apr 14 17:19:37 UTC 2005
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 at 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 at 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).
More information about the drupal-devel
mailing list