Support from Acquia helps fund testing for Drupal Acquia logo

Comments

martins.bertins’s picture

Here's the patch.

martins.bertins’s picture

Fixed some typing errors in the patch.

Bernsch’s picture

Status: Active » Needs review
swentel’s picture

Category: bug » feature
Status: Needs review » Needs work

There's spaces in the patch, and this was intented by design, not a bug report.

martins.bertins’s picture

Status: Needs work » Needs review
FileSize
2 KB

Removed the spaces.

kclarkson’s picture

@swentel,

I need this functionality as well. Basically I have an event that utilizes the registration form. Only logged in users can register for my events so I wanted to create a simple block that says: "you must be logged in to register" and use the block default visibility settings.

Because I am using Display suite for my layout I would like to position that custom block field but only want it to show for anonymous users.

kclarkson’s picture

Title: Block field respects block visibility settings » Block field Doesn't respect block visibility settings
kclarkson’s picture

Status: Needs review » Reviewed & tested by the community

@ martins.bertins

I just applied your most recent patch. Applied cleanly and now the blocks respect the default settings THANKS !!!!!

ps- at first I didn't think it was working but realized I was logged in as User 1. After I logged in with a different user it worked.

Shane Birley’s picture

I can confirm that the patch is working as expected. This should find its way into the commit list soonish. Quite an important and powerful change.

*UPDATE*

I spoke too soon. It worked for about five minutes and then (once the cache was cleared) I started getting:

Warning: include() [function.include]: Failed opening '.tpl.php' for inclusion (include_path='.:/usr/share/pear:/usr/share/php') in theme_render_template() (line 1495 of /includes/theme.inc).

It seems to open up a hole and "forgets" what theme bits are being loaded. Unsure if it is related but, if I return to the original ds.module file, the problem goes away.

bmodesign’s picture

Patch #5 worked for me, installed on commerce kickstart.

markosef’s picture

Doesn't work in my case. I use "Show block on specific pages" and "Content type" combined and when I combine the two, block is not displayed in those cases. Thinking of giving up on using DS for views as it just makes a mess.

swentel’s picture

Status: Reviewed & tested by the community » Closed (works as designed)

I'm sorry, but the visibility doesn't fly here for these kind of blocks.

kclarkson’s picture

@Swentel,

Does the patch in #5 not fix the solution ? Is there a reason you do not want to include this patch ?

swentel’s picture

Status: Closed (works as designed) » Needs work

Talked to Mārtiņš at DrupalCamp Baltics.

The problem with the current patch is following: committing it like this would mean that suddenly visibility settings will kick on existing sites in case a block is also used on other places. This is not desired behavior, so it means we need an extra setting on the block field. Something like 'respect visiblity settings.

Another better option also than the current patch is probably just calling drupal_alter('block_list); which then would mean all kind of visibility settings would work automatically out of the box (pages, content types, php, language etc).

jwa’s picture

Exactly the same code as provided in #5, but updated line numbers so it can patch with DS 2.6

nicktr’s picture

Thank you for this patch. Works nicely. Using DS 2.6.

aspilicious’s picture

I agree with #14 so a committable patch would go in that direction.
I fixed this for the drupal 8 version, so we don't have any issues there.

Leeteq’s picture

Version: 7.x-2.2 » 7.x-2.x-dev
skribbz14’s picture

Just applied the patch from comment #15 and it doesn't appear to be affecting block visibility settings for restricting anonymous users.

What I am trying to do is hide particular blocks from anonymous users. I created the blocks and set their role visibility settings to "authenticated user" only, by checking only that roles' checkbox. I then created Display Suite custom block fields for each block and placed them on their view mode and pages. When logged out I can still see these blocks.

What's weird is that if I do the opposite and restrict a particular block from people who are logged in that works just fine, so that part is working, but I really need to hide things from anonymous users as well.

Thanks for any help in advanced!

Update:

I was able to able to configure role permissions for individual fields once I enabled "Field permissions", using the Display Suite Extras module.

khanz’s picture

I am using a block to show adsense ads on nodes, but the block are also shown on node and comment preview pages and is a undesirable function for me. Trying to restrict the block in specific paths doesn't works with ds 2.6.

mducharme’s picture

This feature request has been marked as "won't fix" several times already. I really needed this functionality as well though and the solution in the meantime is Field Formatter Conditions.

It replicates block visibility settings on the block field (or any field when managing display) at the cost of two extra modules.

bousley’s picture

This is a pretty serious issue. I don't see why they continue to mark this issue as "won't fix". It runs directly contrary to how blocks function as far as context goes...

mducharme’s picture

I have an idea. What about adding a new checkbox within the extras settings called "Respect Block Visibility" that would be checked before executing the code in this patch? That would resolve the issue raised in #14 since it would be off by default.

Thoughts?

mparker17’s picture

+1 to @mducharme's idea in #23.

mducharme’s picture

Re #14

Another better option also than the current patch is probably just calling drupal_alter('block_list);

I thought this was a good idea since it leverages a block function but it's not going to work for this case. ```block_list``` depends on checking the theme's regions to determine whether or not the block should be displayed. Since we can use disabled blocks as DS block fields, those will be never checked. Even if block_list allowed us to check for "disabled" blocks, it's a memory hog and caused timeouts throughout my testing.

It would be great if there was an easy way to say "**IF** this block were to be rendered **anywhere** on this page, would the value of $block->visibility be 0 or 1?".

daggerhart’s picture

The patch from #15 is good except one thing. It attempts to detect if the user is admin with ($user->uid > 1). This causes the patch to ignore role visibility for anonymous users.

Attached is a re-roll of the #15 patch with this issue corrected.

nickBumgarner’s picture

Just tested #26 and it seems to be working perfectly. Even anonymously it works! Thanks.

bousley’s picture

Thanks, daggerhart!

swentel’s picture

#23 sounds sane yes

DYdave’s picture

Following-up with #29 and @swentel's confirmation of #23's approach, I went ahead and tried to add this feature on top of the patch from #26, which seems to have been tested successfully at #27 and #28.

Please find attached to this comment an updated patch against ds-7.x-2.x at 1640acd, which does everything #26's did, plus adds a new checkbox on the DS block field edit/add form, called Respect Block Visibility, to allow users to enforce block's visibility settings for the field (approach from #23).
File attached as: ds-7.x-2.x-respect-block-visibility-settings-1936128-30.patch.

See attached screenshot of the resulting DS block field edit/add form:
Screenshot of the resulting DS block field edit/add form after applying the patch: added a checkbox to allow users to enforce block's visibility settings for the field.

Switching back issue to needs review for testing, feedback and reviews.

Feel free to let me know if you would have any comments, questions, issues, suggestions, objections or ideas on this updated patch, I would certainly be glad to provide more information.
Thanks very much in advance for everyone's testing/reporting, reviews and comments.

swentel’s picture

Looks good on a first view - haven't tested it manually yet, will try this weekend. Others can too of course.

Two small code things

  1. +++ b/ds.module
    @@ -879,6 +879,52 @@ function ds_render_block_field($field) {
    +    if($data->visibility < 2) {
    

    extreme nitpick: needs a space between 'if' and '('.

  2. +++ b/modules/ds_ui/includes/ds.fields.inc
    @@ -377,6 +377,12 @@ function ds_edit_block_field_form($form, &$form_state, $custom_block = '') {
    +    '#default_value' => isset($custom_block->properties['block_visibility']) ? $custom_block->properties['block_visibility'] : '',
    

    Default value should probably be FALSE instead of an empty string.

DYdave’s picture

Thanks a lot @swentel for this detailed review.
I wish I could have received nitpicked reviews like that more often.

Please find attached to this comment a re-rolled patch from #30 against ds-7.x-2.x at 1640acd, including all the code comments/changes suggested at #31.
File attached as: ds-7.x-2.x-respect-block-visibility-settings-1936128-32.patch.

Any comments, feedback, testing/reporting, questions, reviews or suggestions on the re-rolled patch or the ticket in general would be greatly appreciated.
Thanks very much in advance for everyone's testing/reporting, reviews and comments.

mducharme’s picture

I've tested this and it works as expected.

I'm on the fence about having a "per-block field" vs a global setting in ds extras though. My concern is that you're left with a mixed mode where, down the road, a user configuring the core block visibility for two blocks might see the changes for one and not the other. I definitely see the added flexibility per-block though so I'm happy to see this method move forward.

aspilicious’s picture

Looks sane to me.

[little hijack of the issue]
One more thing, related to drupal 8. Do we want the same flexibility or do we prefer to always follow the restrictions put on the block?

swentel’s picture

Gave this more thought. This won't work if someone uses i18n to respect language visibility. So the trick here is that we need to mimic hook_block_list_alter(). like block module does, then any block visibility will work out of the box.

@see _block_load_blocks()

This will also reduce the code immensively!

mducharme’s picture

Here's a rewrite of the patch based on your notes @swentel. It definitely reduces the code by offloading all of the block processing. I've tested a lot of scenarios and this seems to hold up.

swentel’s picture

Looks amazing, will test this over the weekend unless aspilicious beats me to it - could probably use a couple of verifications anyway to make sure we really don't break existing installations :)

stephenpar’s picture

I am using modules Organic Groups, Event and Entity Registration to allow group members to register to events. Users can only join Events organised for Groups that they are a member of. Everything worked fine except that Entity Registration doesn't show a link to Cancel a registration once a user is registered and I also wanted to display a list of participants (to logged in users only) when viewing an Event.

So Display Suite is used to display a block of Registrations that shows the username, company name and a Delete link of all people who have resgistered to attend. The delete link only shows at the user's own row as expected. Admins will see the delete link at all rows. It was almost perfect except that the Registrations block was also visible to anonymous users.

Then I applied patch #1936128-36 to 7.x-2.6 and here are the results.

  • Anonymous users don't see the Registrations block anywhere, which solves the issue that I had.
  • When an Event has registrations, logged in users can see the list of people attending and the delete link is still correctly only showing for the user's own registration.
  • However, when an Event doesn't have any registrations, the following warnings appear at the top of the page:
    • Notice: Undefined property: stdClass::$subject in ds_render_block_field()
    • Notice: Undefined property: stdClass::$content in ds_render_block_field()

Can anyone tell me how to fix this please?

Edited:
Basically the problem is with an empty block. Before applying the patch an empty block didn't cause any Notices to appear. The patch doesn't handle empty blocks well.

For now and until this is fixed I'll just make sure that the block is not empty even if there are no results by using the No Results Behaviour in the View.

mducharme’s picture

Assigned: Unassigned » mducharme
Status: Needs review » Needs work

Thanks for the detailed explanation. I know what the problem is and will post an updated patch shortly (hopefully within 24 hrs).

mducharme’s picture

Status: Needs work » Needs review
FileSize
6.19 KB

Fixed. I realized while reviewing the code that I had accidentally removed the check to ensure that the core block module was enabled. That's fixed too.

mducharme’s picture

Assigned: mducharme » Unassigned
aguilara’s picture

I applied this patch and for some reason it seems to not be doing what I thought it was supposed to. From my understanding, this patch would allow one to add a block field using Display Suite and then be able to control the pages where the block would appear using the Visibility settings of the block and still be displaying in the section that it was added to in Display Suite. However, what ended up happening is that the Block settings ended up overwriting completely the Display Suite block field.

The situation I was trying to fix is that I have a two column display that I use for only two Basic Pages by using the "View mode per node" feature in the Extras of Display Suite. These two basic pages are supposed to have two different blocks in them (one in each column). So I was hoping that using this patch, I would be able to have only one "Two Column" view mode for these pages and add all four different blocks using Display Suite's block fields and place the blocks in the correct column using the Manage Display UI. Then use the Block settings to specify in which pages the blocks will appear. Now checking the "Respect Block Visibility" does indeed allow the Block settings to affect Display Suite. What happens though is that when I enter the page where I want the block to appear, the Block gets added to the Content section of the page. Which does cause the block to appear in the page the settings listed, but it is displayed in a one Column View. Maybe this has to do with the fact that I am using the "View per node feature" and by default, the block is displayed using the Default view for the content type? Hopefully my explanation makes sense and someone has an idea as to what is going on.

Thanks!

mducharme’s picture

I just ran through several tests and couldn't replicate your issue @aguilara. Everything worked as expected so I don't think it's related to this patch. I had a bit of trouble understanding your case/workflow though so I suggest posting a new issue or support request since I think it's display mode per node related. Post a link to your issue here and I'll head over and paste my step-by-step process for you to compare.

asad.hasan’s picture

#40 works great, however native menu blocks do not work, I have to use some other modules menu block.

Please advise.

mducharme’s picture

@asaduiux I just went through testing both the main menu and user menu "native" blocks and they work as expected with this patch. Can you provide the steps to reproduce the problem that you're seeing?

zmove’s picture

Status: Needs review » Reviewed & tested by the community

#40 perfectly work (don't forget to edit your field and check the option "Respect block visibility").

+1 for a quick commit as it sounds more like a bug report than a feature request to me.

kerrycurtain’s picture

Confirm also that #40 works a treat, thank you.

kerrycurtain’s picture

Comment removed

bdanin’s picture

patch in #40 works very well for me, huge help, thanks!!

hansrossel’s picture

patch #40 works here too

hansrossel’s picture

After applying patch #40 there is a caching issue when I create a new views block field; to reproduce
- make a views block
- create a block field for that view, leave everything default and do not select the respect visibility settings
- enable the block field: nothing is visible
To fix it:
- go to the block config in the block overview and save the block => the block field is visible now

hansrossel’s picture

Status: Reviewed & tested by the community » Needs work
bdanin’s picture

@hansrossel, before going to the block config admin screen and saving the block, did you try to clear all Drupal caches to see if that alone fixes the problem? Just wanting to make sure that before we switch back to "needs work" that something is actually breaking

dshields’s picture

It'd be great to hear back from @hansrossel (who set it to "Needs work") on this.

hansrossel’s picture

Unfortunately I don't remember anymore for which site I used it. If I'm the only one that had cache issues it might also have been something with that specific site. I did not test it with a vanilla Drupal installation. So set back to reviewed and tested if you feel like.

mducharme’s picture

The patch still applies cleanly to 7.x-2.13 and 7.x-2.x.

dshields’s picture

Status: Needs work » Reviewed & tested by the community
ron_s’s picture

Patch still applies cleanly to 7.x-2.14.

We've been using this for almost 2 years now... can we finally get patch #40 committed? Thanks.

lanceh1412’s picture

This patch (#40) works fine for me on 7.x-2.14

dshields’s picture

Priority: Normal » Major

  • swentel committed f539fe1 on 7.x-2.x authored by mducharme
    Issue #1936128 by martins.bertins, DYdave, mducharme, daggerhart, jwa:...
swentel’s picture

Status: Reviewed & tested by the community » Fixed

Ok. I went ahead and committed this to dev. Will let it like this for a while to make sure enough people can test it.

rubendello’s picture

rubendello’s picture

Did, by any chance, somebody tried to make this work in D8?

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

MariaIoann’s picture

I updated DS from 7.x-2.14 to 7.x-2.15 which includes patch #40 and my DS block fields (Quick Tabs blocks) are not being rendered.
Debugging it, seems that $block->content['#markup'] is not set. Using the Default DS Block Layout it works, but then I have to manually hide the block title.

Maybe, it is related to this: https://www.drupal.org/project/ds/issues/2962309

Is it a DS issue or a Quick Tabs issue?

maartendeblock’s picture

Updating from 7.x-2.14 to 7.x-2.15 made block fields no longer visible. The checkbox to respect block visibility is not set.

Applying patch https://www.drupal.org/files/issues/2018-04-23/block-content-fix-2962309... to 2.15 solved the issue.