The title pretty much says it: If I add a block to the Dashboard and restrict it to certain roles, users of other roles will still see the block ... that is, a block with the title and the content replaced by 'empty'. Not pretty (and confusing).

As far as I can see, this is because of this in dashboard.module / dashboard_page_build

      // Find blocks which were not yet displayed on the page (were empty), and
      // add placeholder items in their place for rendering.
      $block_list = db_select('block')
        ->condition('theme', $theme_key)
        ->condition('status', 1)
        ->condition('region', $region)
        ->fields('block')
        ->execute();

Afterwards, all rows returned by this query that is not an active block in the region is entered as an 'empty' block---including those that have been hidden because the user should not have acces to them. I assume that is not quite intended behavior?

I am not quite sure, though, because I don't understand why this (adding blocks back in as 'empty') is done in the first place ....

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
1.84 KB

This bit of code makes no sense to me. Removing it solves the problem. Too easy?

tim.plunkett’s picture

Status: Needs review » Needs work

This breaks the ability to drag-n-drop empty blocks. So, not a solution. But this is still a bug.

You can't even do .dashboard-block-empty { display: none; }, since there will still be an <h2> above it.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.92 KB

Now it only displays the empty blocks if the user can use drag-n-drop.

sven.lauer’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work
FileSize
1.16 KB

While the patch in #3 probably takes care of the issue in most realistic scenarios, it does not really address the problem, but rather works around it ... here is an edge case where it does the wrong thing: Suppose a role has the "administer blocks" permission, but certain blocks are set to be hidden (for whatever reason) from this role. A user that (only) has this role would see "empty" blocks for those.

The crux of this issue really is that the assumptions of the in-code comment below are not true: Not all blocks that have not been displayed are "empty" blocks: Some may just be set to be not visible for the current user.

      // Find blocks which were not yet displayed on the page (were empty), and
      // add placeholder items in their place for rendering.

Minimally, the block content should not be "(empty)" for the non-empty, but hidden blocks, but rather something like "(you are not authorized to display this block)" or something.

I am willing to try and work on another patch ... though: Could someone tell me what exactly the "empty" blocks are that this code attempts to display? Ideally, does someone have a minimal example (or even better: a test ;) of an "empty" block in the sense used here? I could not find any example in either the dashboard or the block tests ...

Attaching a patch which only introduces a failing test for this issue, just to get us started.

(Moving to 8.x since no patches go into 7.x before the issue is fixed in 8.x)

sven.lauer’s picture

Status: Needs work » Needs review
FileSize
3.9 KB

The attached patch attempts to fix the issue properly. Like #3, it only adds the "(empty)" placeholders in if the user has the "administer blocks" permission, but it also drupal_alter()s the list of (potentially empty) blocks that is pulled from the database. Since block_block_list_alter() is were blocks are removed that a user should not see, this has the desired effect of removing these blocks.

I say 'attempts to fix the issue' as I am still a bit unclear what these "empty" blocks are, and whether they might not be filtered out by some implementation of hook_block_list_alter() ... ideally, we would want to put in a test for this.

sven.lauer’s picture

sven.lauer’s picture

sven.lauer’s picture

Status: Needs review » Needs work

The last submitted patch, 1009872-5-dashboard-invisible-blocks-show-as-empty.patch, failed testing.

sven.lauer’s picture

Status: Needs work » Needs review
FileSize
3.91 KB

Re-rolling against current HEAD.

xjm’s picture

This might be moot in D8 on account of #950956: Remove Dashboard from core ; however, it presumably still needs to be fixed in D7. Here's a D8 reroll just in case.

areikiera’s picture

Would really appreciate a backport to D7. I'm creating several blocks restricted by role and some roles are seeing multiple empty blocks on their dashboard.

*As a side note: Not sure of the proper workflow for status changes, and so wasn't sure if I should change the status to "patch (to be ported)" or maybe "Reviewed and tested by the community". Could the status be holding back the progress? Also, I started a new issue on this (http://drupal.org/node/1430618) so D7 users might be able to find the patch without the need to hijack this thread with a version change, but it was marked as a duplicate. What is the preferred workflow in this situation?

Thanks!

sven.lauer’s picture

@areikiera:

As long as #950956: Remove Dashboard from core is not decided/committed, this issue needs to be resolved in D8 before porting (for assume that Dashboard module does NOT get removed, then we would have a regression if we did not fix this in D8 first). Should that issue be resolved and Dashboard module be removed, THEN we would move the issue here to D7 and set to "patch (to be ported)".

Unless and until that happens, we need to get the current patch into D8, after which we can set it to "patch (to be ported)" to get it into D7. So, to speed up this process, we need reviews/tests (on D8!), so that this patch can be updated (if something is amiss) or set to RTBC (if the patch is good).

areikiera’s picture

@sven.lauer

Understand, and appreciate the explanation. However, since D8 isn't widely used by people other than developers, and D7 users are currently experiencing the problem on live, publicly used sites, it seems like an issue that shouldn't have to wait for a D8 decision. Additionally, the problem seems like it will exist regardless of the D8 decision and will need to be addressed anyway. I understand the need for the workflow here, so with that in mind, would it be appropriate to re-open my other thread (http://drupal.org/node/1430618) so people can work on a usable patch there? Or perhaps a new thread looking for a solution that's not based on a D8 patch is the proper way to go (though it seems like a logical starting point since someone appears to have done much of the research)?

Sorry about the persistence, and I wish I was more familiar with patching these things myself (still working at it). This is a very visible problem to a few of my clients and I feel like they lose a little bit of confidence in Drupal when issues like this arise, and it concerns me a bit when a visible problem like this is postponed for the many users of the live, current release, in favor of a future unreleased version.

sven.lauer’s picture

I understand your frustration.

About the other issue: This has been correctly closed as a duplicate. There should be one issue per bug. D7 issues should be addressed here (think about it this way: The "Version" that is assigned to this issue is the version of the current patch. The issue in itself is not tied to a particular version). It is perfectly acceptable to discuss workarounds/hotfixes for D7 right here (I think). You might even post a D7 patch, but do not expect it to get committed (before the issue is resolved in D8). These are the rules. (I did not make them, but I see the very good reasons for having them.)

And true, D8 is not used by anyone but developers --- but, at least for now, the D8 code is very similar (at least wrt to this issue) to the D7 code. So fixing D8 will directly translate into fixing the bug in D7 (in most cases, this is a simple re-roll of the patch). If this gets into D8, it should get into D7 in a very short time. The way to think about this is that fixing the bug in D8 MEANS fixing the bug in D7.

The real problem here is not that this needs to get into D8 first---but rather that this issue has not seen any attention in a long time. I can't review my own patch, or else I would push it forward ...

sven.lauer’s picture

P. S. My explanation may have suggested that this will not / should not be fixed (in D8 and then D7) before #950956: Remove Dashboard from core is decided. The opposite is true: This should be fixed ASAP, regardless of whether Dashboard ultimately gets removed from D8. The reason the patch above is not committed has nothing to do with the possible removal of Dashboard.

sven.lauer’s picture

Speaking of workarounds: When I first ran into this problem, here is what I did: I created a custom module (let's say, it was named "custommodule") and implemented the following:

/**
 * Implements hook_preprocess_HOOK().
 *
 * Remove "empty" blocks from dashboard.
 */
function custommodule_page_alter(&$page){
  if (isset($page['content']['dashboard'])){
    foreach ($page['content']['dashboard'] as $region => $region_data) {
      if (preg_match('/#/', $region)){
        continue;
      }
      foreach ($region_data as $key => $data) {
        if (is_int($key)) {
          unset($page['content']['dashboard'][$region][$key]);
        }
      }
    }
  }
}

This still works. I could roll a mini-module and post it as a sandbox project to help in case you do not know how to do this yourself.

sven.lauer’s picture

Disclaimer: #17 is a hackish workaround and quite fragile, but it solves the problem of having the "empty" blocks visible, and does so without hacking core. Use at your own risk, though.

saltednut’s picture

Title: Dashboard blocks that are restricted to certain roles are visible for all other roles --- as 'empty' » Dashboard has hardcoded block visibility understanding: hidden blocks are visible for all other roles --- as 'empty'

This is also an issue for functionality that is in D8 for block visibility per Language.
#135464: Add language options to block visibility
dashboard blocks showing as 'empty'

Gábor Hojtsy’s picture

Title: Dashboard has hardcoded block visibility understanding: hidden blocks are visible for all other roles --- as 'empty' » Dashboard does not work with hook_block_list_alter(), exposes all blocks from the database

Looked at @xjm's patch reroll and it looks pretty good. I think it would be pretty great to get this in, since we don't have any clear direction on where to go forward with dashboard, and a D7 fix for this would be very good. Any outstanding issues people see?

xjm’s picture

xjm’s picture

Assigned: Unassigned » xjm

Needs a few cleanups that I'll take care of. Waiting to see if it passes the bot first.

hellolindsay’s picture

Any progress on this? Seems there have been no comments since March. I just ran into the (empty) dashboard blocks problem in Drupal 7...

tim.plunkett’s picture

Status: Needs review » Closed (won't fix)
xjm’s picture

Version: 8.x-dev » 7.x-dev
Assigned: xjm » Unassigned
Status: Closed (won't fix) » Needs review
Issue tags: -Needs backport to D7

Actually, this still needs a fix in D7.

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Novice

So here are the cleanups I apparently intended to do all those months ago:

  1. +++ b/core/modules/dashboard/dashboard.moduleundefined
    @@ -170,25 +170,33 @@ function dashboard_page_build(&$page) {
    +      // Add placeholder items for empty blocks so that users that
    +      // have the right to do so can move them.
    ...
    +        // Collect the blocks into an array keyed by bid--for this is
    +        // what hook_block_list_alter expects.
    

    These comments are wrapping too early. Reference: http://drupal.org/node/1354#general

  2. +++ b/core/modules/dashboard/dashboard.testundefined
    @@ -137,4 +137,22 @@ class DashboardBlocksTestCase extends DrupalWebTestCase {
    +   * Test that blocks that a user should not see are actually not displayed.
    

    This should say "Tests" rather than "Test." Reference: http://drupal.org/node/1354#functions

  3. +++ b/core/modules/dashboard/dashboard.testundefined
    @@ -137,4 +137,22 @@ class DashboardBlocksTestCase extends DrupalWebTestCase {
    +    // Add new custom block that is only visible to administrators
    

    This needs an article and a period: "Add a new custom block that is only visible to administrators."

  4. +++ b/core/modules/dashboard/dashboard.testundefined
    @@ -137,4 +137,22 @@ class DashboardBlocksTestCase extends DrupalWebTestCase {
    +    $this->assertNoRaw('<div id="block-block-1"', t("Block that is set invisible for user's role is not displayed on dashboard."));
    

    The t() should be removed from the assertion message here. Reference: http://drupal.org/simpletest-tutorial-drupal7#t

Tagging Novice to reroll the patch against D7 with these cleanups. Please upload a test-only patch to expose the test coverage, the complete patch, and an interdiff showing your changes.

dan_lennox’s picture

Assigned: Unassigned » dan_lennox

Going to have a go at this for core office hours session.

dan_lennox’s picture

Status: Needs work » Needs review
Issue tags: +sydney2013
FileSize
3.91 KB
1.17 KB

This is the backport to D7 and we've made the changes suggested by xjm in #26.

Also please credit interactivejunky as we worked on this as a team.

jbrown’s picture

Status: Needs review » Reviewed & tested by the community

Great work guys!

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

As an administrator, this patch prevents me from dragging and dropping blocks that I don't see. That was confusing, especially since admin/dashboard/configure and admin/structure/block do not prevent me from doing that. I should be able to drag and drop any block that appears on the dashboard.

Maybe instead of invoking hook_block_list_alter() (which removes those blocks completely) we should just tag the empty ones with a CSS class and hide them with "display: none" (then remove that styling once you enter dashboard customization mode)? Ugly, but it should work. It would also prevent this patch from invoking hook_block_list_alter() with an incomplete set of block regions, which I believe it does otherwise and which some code implementing that hook might not expect.

+    $this->drupalGet('admin/dashboard');
+    $this->assertNoRaw('<div id="block-block-1"', "Block that is set invisible for user's role is not displayed on dashboard.");

Doesn't seem like this is testing much; would be very easy for something in this HTML to change that would cause this test to erroneously pass. Would it be better to assert that the block title isn't present, and then go back, remove the access restrictions on the block, and assert the same block title is present?

+    // Add a new custom block that is only visible to administrators.
...
+    $custom_block['roles[3]'] = TRUE;

That also seems to be assuming a lot.... not sure the test would even pass on all platforms. Maybe just DRUPAL_ANONYMOUS_RID (display the block for anonymous users only) would be better here.

David_Rothstein’s picture

Would it be better to assert that the block title isn't present, and then go back, remove the access restrictions on the block, and assert the same block title is present?

Of course, that wouldn't be possible with my other suggestions above. However, it would be possible to look for the presence/absence of the CSS class instead.

mgifford’s picture

Assigned: dan_lennox » Unassigned
Issue summary: View changes