Problem/Motivation
Block visibility settings allow to show a block under certain conditions but they have to pass all conditions to be valid. Sometimes it's needed for just a single condition to pass, one of the main features of https://www.drupal.org/project/block_visibility_groups
Steps to reproduce
Place a block on all Articles but also basic page with URL "/test"
Currently the block will never appear, with the change though it can appear under both conditions.
Proposed resolution
Add setting to block form for condition logic
Remaining tasks
Review
User interface changes
API changes
NA
Data model changes
NA
Release notes snippet
New block setting for condition logic to change visibility from an "and" condition to also an "or" condition.
Comment | File | Size | Author |
---|---|---|---|
#99 | Screenshot 2024-04-01 at 8.31.15 PM.png | 69.6 KB | smustgrave |
Issue fork drupal-923934
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
mdupontWhat is wrong in the logic? The examples you give are all behaving correctly.
In your first example, you set the block to display only on nodes of content type "Blog entry". Blogs page is not of content type "blog entry" so the block won't show.
In your second example, the block will only display on the page with the path "blog", therefore the Blogs page. To display on several pages, enter "blog/*" under the first line, so the block will be displayed on any page with the path blog/[anything].
Your third example is the same as the second one, where you added a restriction on the content type. As the page with the path "blog" is not of content type "Blog entry", the block will never be displayed.
I assume that your Blogs page is accessible under "blog" and your blog entries under "blog/[something]". In this case, what you'd want to do is to configure your block this way :
- Tick "Only the listed pages"
- Write "blog" and "blog/*" in the textarea
- Make sure "Content types" is empty
Comment #2
mertskeli CreditAttribution: mertskeli commentedSo, there is no option which allows to show a block on both a page and a node type.
No concerns regarding 1 and 2. But option 3 makes it a bug.
A page and a node are two different things, and visibility per page and visibility per node (type) are supposed to be of no relation to each other, until we consider example.com/node "a page" or a node has a URL alias.
Every blog entry is a node, not a page, and is a node/[something], not a blog/[something].
Comment #3
mdupontIt works as designed. Visibility filters add up. If the current page is not a node, it won't have a node type, so you won't want to show the block on it if you have set up a filter to show it only on nodes of a certain type.
For a more flexible way to set the block visibility, you can look for contrib modules (like Panels or Context) or write a PHP snippet which will give you all the flexibility you need (like in your case: display the block on the Blogs page AND on each node of type blog entry). A quick search should give you some ready-to-use snippets.
Comment #4
mertskeli CreditAttribution: mertskeli commentedmdupont, you misunderstood this issue, but I'm not going to re-open it.
Blog IS a page, and it includes a list of recent/all posts.
True for D6. But this issue is per D7 which does not have php option in block settings.
Comment #5
mdupontI understand your frustration with the current block visibility settings, but it works in D7 the same way than in D6. I think I understood the issue.
In D7 you are still able to use a PHP snippet to control block visibility (as unfriendly as it is), if you have enabled the PHP filter module (in core).
Blog is a page, indeed, but it's not a node. It's a custom page created for the blog module. Therefore it is not a blog entry node. If you restrict block visibility to blog entry nodes, the block won't show up on any custom (non node) pages nor on any node of a different type than blog entry.
So in the end it works as designed, but that doesn't mean than the way it was designed is good. What your issue is really about is a feature request (not a bug report) for introducing a kind of "OR filter".
Explanation: in addition to being able to filter block visibility by path AND by node type, as it is currently, you want to be able to filter block visibility by path OR by node type. In the first case, you are effectively restricting visibility only to nodes with the corresponding type and path. In the second case, the block will show up as soon as one of the conditions is met (is a node of the corresponding type OR is any page that matches the path).
The issue you mentions would be fixed if the block system would work like in the second case. It is not because the block system works like in the first case, it is an AND filter. It is not wrong logic, but for you it is not the logic you need.
Quoting you:
This is where you're wrong. Visibility settings are in relation and add up. Usually, if you tick a content type under "Show block for specific content types" (and you apply no other visibility restriction), you don't expect the block to show elsewhere than on node/[nid] pages where the node has the given content type, did you?
In the end, a solution would be a new feature like an additional radio button in the block visibility settings:
"Show block if:
[ ] One of these conditions is met
[ ] All of these conditions are met
"
Of course, as this stage, such a feature would not be added to Drupal core, it belongs to a contrib module to be developed, and maybe, if it is good enough, it could be integrated into D8.
You still have other options : you could use Pathauto to generate URL aliases like "blog/[node:title]" for blog entry nodes and restrict block visibility by path only, or you could enable the PHP filter module and write a code snippet.
Comment #6
mertskeli CreditAttribution: mertskeli commented(I appreciate your calm and detailed answer.)
D7 makes it only OR logic, comparing two incomparable terms. It is wrong. Maybe it is not a functional bug (although it looks like a functional perversion) but it is a UI bug for sure.
The visibility setting says: Page and Node (type). As if a node can never be on a page, and a page is unable to contain nodes.
So for your list of "Recent blog nodes" you have to choose either a blog page, OR a blog node. Or you have to download 2-3 more modules to make it work.
Comment #7
mdupontRight, as you said the current UI doesn't provide a straightforward way to display a block both on the blog listing page and blog nodes. Drupal 7 is not the only version affected, former versions had the same block visibility UI logic and limitations.
The bug therefore lies in the fact that the UI doesn't make it clear that different "categories" of filters add up and aren't independent from each other. So if you take D7 filters, you have "Pages", "Content types", "Roles" and "Users" and each of them must return TRUE for the block to be displayed :
- "Pages": should it be displayed at this URL?
- "Content types": are we on a node page and should it be displayed for a node of this content type?
- "Roles": does the current user belong to one of the roles for which we should display the block?
- "Users": Can users decide to show or hide this block in their preferences, and is the current user's setting to show this block?
The block will only be displayed if you can answer "yes" to all these questions. The UI doesn't make it clear for users.
As this issue already contains quite a few posts, I suggest to open a new issue stating that the block visibility settings UI is confusing, and linking back to this one.
Comment #8
mertskeli CreditAttribution: mertskeli commentedI suggest to forget about it. If too many people complain, it will be fixed. For now it seems to be acceptable.
Comment #9
mdupontFollow up in #960864: Block visibility settings UI is confusing - unclear about how settings will be applied.
Comment #10
edurenye CreditAttribution: edurenye at ENDPHASYS Technologies commentedI'm reopening this issue because in the exposed case was working as designed but there is another situation (at least in d8) that does not follow the rules, if you set visible in /blog and in /node/* and then in content type you select all except blog post and negate the condition, then it's supposed to show the block in blog and in the blog posts but it's just shown in the blog posts.
Comment #11
cilefen CreditAttribution: cilefen as a volunteer commentedCheck out https://www.drupal.org/project/block_visibility_groups
Comment #12
drugan CreditAttribution: drugan as a volunteer commentedFirst, Negate the condition checkbox is returned into place.
Second, added AND / OR conditions conjunction operator option. The logic is simple: in the case of AND all conditions (negated included) should pass. Actually, it is the current state without the patch applied. In the case of OR at least one of the conditions set should pass. If no one is passed then the block will be hidden. If no one condition is set then the AND / OR setting has no effect and block will be visible everywhere.
Third, removed Show for the listed pages / Hide for the listed pages options from the Pages condition which is confusing (at least for me). The Negate the condition is quite easy to understand and it's enough as I think. Also, if user will try to negate the empty Pages value or * (asterisk, which is basically has the same effect as empty) then the negate option will be forcibly set to FALSE when saving the block.
The caveat is that if for example you have only two content types Article and Basic page and they are all checked and negated then the block will disappear everywhere in the case of conjunction operator AND or, if conjunction operator is OR and no other conditions are set. Logically, it is insane setting and I don't think that any of admins will try to do it.
You may see working example of the patch on the Commerce Quick Purchase module. Note that when using the module all the blocks on the system will get AND / OR conditions conjunction operator not only the block provided by the module.
Comment #13
drugan CreditAttribution: drugan as a volunteer commentedComment #15
drugan CreditAttribution: drugan as a volunteer commentedThe reason for the test fail was in Show for the listed pages / Hide for the listed pages options. Just because it has two radio buttons one of which is always returns TRUE. Actually, I remember was stumbled on some issue where the default Negate the condition option (single checkbox) was changed for radios because of "usability" reasons. As a consequence the tests were adjusted for the change. Which are now returned to the initial state to work with the single FALSE or TRUE value of the checkbox.
Comment #17
drugan CreditAttribution: drugan as a volunteer commentedComment #19
acrosmanHaving struggled with the block UI and writing block condition plugins due to the current behavior of assuming AND at all times, I strongly support the notion of adding support for ORs.
Comment #20
drugan CreditAttribution: drugan as a volunteer commented@acrosman
Actually, this feature was always in the core. I've just implemented it. Look at the line 39:
https://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Condition/C...
Comment #21
jwilson3Patch in #15 does exactly what is needed. Block Visibility Groups is a very nice module that should be included in core at some point!
Comment #22
tim.plunkettThis needs automated test coverage before it can go in.
Comment #23
arunkumarkI have manually tested the patch and tested. It seems to be working as expected.
Comment #24
BerdirThis still doesn't have automated test coverage so it can't be RTBC.
Comment #26
preksha CreditAttribution: preksha commentedHave manually applied this patch and tested, seems to be working fine.
Comment #27
mr.york CreditAttribution: mr.york at Agence Inovae commentedReroll last patch.
Comment #28
ccjjmartin CreditAttribution: ccjjmartin at Four Kitchens commentedI can confirm the last patch applied to 8.6.2. The author didn't mention tests so we likely still need those.
Comment #29
ccjjmartin CreditAttribution: ccjjmartin at Four Kitchens commentedThe last reroll applied properly but missed a chunk a code. Reroll 2 attached.
Comment #32
B-Prod CreditAttribution: B-Prod commentedIn the last 2 patches, it seems there are some mistakes.
Patch 27 : the OR condition is not taken into account, as there is still a
'and'
in the code:$this->resolveConditions($conditions, 'and')
It needs to use the
$and_or
variable to evaluate correctly.Patch 29 : the
gathered_contexts
variable is not defined, as it has been replace by the$definitions
one.The current patch is a reroll for the current published release (Drupal 8.8.2).
Comment #35
ptmkenny CreditAttribution: ptmkenny commentedReferencing another issue related to block visibility conditions.
Comment #36
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded reroll for Drupal-9.2.x.
Comment #37
AndrewTur CreditAttribution: AndrewTur commentedReroll against dev-9.2.x
Comment #38
ankithashettyFixed Custom failure errors in #37 in the following patch, thanks!
Comment #39
ankithashettyUpdated patch in #38, thanks!
Comment #41
AndrewTur CreditAttribution: AndrewTur commentedRerolled the patch to apply against 9.1.10
Comment #42
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedRe-rolled against 9.3. Please review.
Comment #43
vikashsoni CreditAttribution: vikashsoni as a volunteer and at Zyxware Technologies commentedPatch apply successfully for reference sharing screenshot
Comment #44
jonnyhocks CreditAttribution: jonnyhocks at Welsh Government commentedUnfortunately the patch in #39 didn't apply cleanly for me on 9.2.0, so I have re-rolled for 9.2.0.
Comment #45
wanjee CreditAttribution: wanjee commentedPatch 923934-40.patch in #41 is missing condition class AndOrCondition declaration.
Applying 923934-44.patch instead against 9.1.10 works.
Comment #46
larowlanThis issue was triaged by the bug smash initiative @lendude, @darvanen, @quietone and myself.
We agreed that this is no longer a bug-report, but now a feature request to add OR support to block visibility conditions.
Reclassifying as such, and updating the issue title.
Also updating the version as features go into 9.3.x
Adding template to the issue summary and adding remaining tasks
Comment #47
Daniel KulbeRerolled against latest 9.2.x and 9.3.x branches.
Comment #48
nicrodgersThe patches in #47 are missing the main plugin condition file core/modules/system/src/Plugin/Condition/AndOrCondition.php
Comment #49
nicrodgersComment #50
nicrodgersHere's a re-roll of 44 for 9.2.x.
Still needs a re-roll for 9.3.x.
Comment #51
Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commentedRe-rolled patch #47, against 9.3.x. Please review. attached interdiff for same.
Comment #52
Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commentedComment #53
nicrodgersAs mentioned in #48, the patches in #47 are missing the AndOr plugin condition and thus should not be used. The patch in #51 is also missing the plugin because it was based on #47.
For 9.3, I'd suggest rerolling #44.
Comment #54
ankithashettyRerolled the patch in #44 against 9.3.x as suggested in #53, thanks!
Comment #55
quietone CreditAttribution: quietone as a volunteer commentedLooks like commit-code-check failed.
Comment #56
mohit_aghera CreditAttribution: mohit_aghera at QED42 commentedFixing commit-code-check.sh failures.
coding standards tests are passing on local now. Should be good to go.
Comment #58
mohit_aghera CreditAttribution: mohit_aghera at QED42 commentedAttempting to fix the test cases.
This change looks a little confusing to me. I had to do this because we've altered the titles in the issue https://www.drupal.org/project/drupal/issues/1932810
I think someone else should look into this one as well.
Comment #59
Berdirthe change to old deprecated content types is fine, but it's just that, the old deprecated version. The new entity bundle condition is not being adjusted now, that code is still unchanged, so you nee to update that too.
shouldn't that be in the plugin itself?
I don't really understand why this test changes. why should changes to the UI affect caching?
The approach seems strange here, that plugin will also show up in other places that use condition plugins. Why isn't the AND/OR setting just a fixed UI element in the block UI and saved in a separate setting?
Comment #60
quietone CreditAttribution: quietone as a volunteer commentedNW for #59.
Comment #62
kazah CreditAttribution: kazah commentedAny update?
Comment #64
mmbkpatch #58 does not apply to core 9.4.x any more.
Comment #65
Serhii Shandaliuk CreditAttribution: Serhii Shandaliuk as a volunteer and at Drupfan for Drupal Ukraine Community commentedComment #66
Serhii Shandaliuk CreditAttribution: Serhii Shandaliuk as a volunteer and at Drupfan for Drupal Ukraine Community commentedComment #67
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedComment #68
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedVerified the patch for drupal 9.5.x version.
https://www.drupal.org/files/issues/2022-07-14/923934-67.patch
After applying patch we can see that and and or option available for the respective block under configuration section.
Where we can choose And or Or condition. Based on this block will be visible. By default AND is selected.
For OR :
If we have applied filter to the content type to blog and in page restriction page given url of another content type such as basic page/artical/ custom then it will display on both blog content type as well as that mentioned path too.
Reference SS
Test Result pass
Can be move to RTBC
Comment #69
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedComment #70
quietone CreditAttribution: quietone at PreviousNext commentedThe issue summary is out of date and this issue is tagged for an issue summary update. Lets see that was asked for. It was 11 months ago, in #46, by a committer. An image has been added to the Issue Summary but that is all. Setting back to NW for an issue summary update.
Comment #59 asked questions about the patch and recommended changes. There has been no change to the patch or any discussion of what @Berdir is saying. That comment has, well, been ignored. That means that the reroll and all the following work are not helping to resolve this issue.
Remember to read all the comments before working on an issue as well as the Issue tags.
Comment #72
jonnyhocks CreditAttribution: jonnyhocks at Welsh Government commentedUnderstand there are still issues to be addressed as noted in comment 70 but I needed to reroll for a project. I have uploaded here if anyone else were to find it helpful in the interim.
Comment #73
jonnyhocks CreditAttribution: jonnyhocks at Welsh Government commentedComment #74
jonnyhocks CreditAttribution: jonnyhocks at Welsh Government commentedApologies for uploading a corrupted patch in 72 (contained a merge conflict) - I attach a working re-roll for 9.5.x.
Comment #75
Daniel KulbeAdding an other issue this is somewhat related to (#3264843).
Comment #76
Daniel KulbeAdding an other update here respecting #3264843, which should somehow also address #59 question 1.
Comment #77
Daniel KulbeTook me a moment, but now I can tell - the changed conditions logic does not work properly:
The following case applies:
I have in the block 'entity_bundle:node' one of the options checked, also the negated checkbox. So this block should show everywhere, where the page is not a node of this bundle.
Visiting a page different page than a node (e.g. the node list), an
MissingValueContextException
will be thrown by the plugin, with this being the last of the visibility option or even when the condition logic is 'AND',$missing_context
will beTRUE
.This does not meet with the expectation, the block should be shown everywhere else but on a node of that bundle.
On a sidenote, the
$missing_value
is never set to anything else butFALSE
. The case should be dropped, the variable removed when not required anymore.Comment #78
Daniel KulbePlease also note, I fixed in this Version :
One further opinion on this ... the check regarding the empty request_path plugin - shouldn't it be happen in the plugin form validation method instead. It is called a few lines below where the check was added.
Comment #79
Daniel KulbeSorry, still found a tiny JavaScript selector issue. I will not start the failed tests in 78 again, as nothing has changed.
Can please sb. take care of the functional test, who has some strong suites there.
Comment #80
Daniel KulbeOne final thought on this - the original code is right in one further thing: The Condition should be added to the conditions array in every case, even if there is an exception, so when collecting cachability, every condition is applied to forbidden for missing value, not just only the valid ones. Adding another version here, which fixes this.
Comment #81
_utsavsharma CreditAttribution: _utsavsharma at OpenSense Labs for DrupalFit commentedFixed CCF for #81 in 9.5.x.
Comment #82
trickfun CreditAttribution: trickfun commentedI think that having one condition logic for all visibility types is wrong.
"user roles" condition should be always in AND mode.
user roles is different from "content type" or "path".
i suggest to separate "user role" from other conditions.
I think the better solution is to have
Visibility (AND or OR for all conditions)
AND
User Visibility (AND or OR for all conditions relative to user)
Thank you
Comment #83
kopeboy CreditAttribution: kopeboy commented@ #82: This is a restriction imho: why?
I could say we should leave it more general, as you can't exclude in advance an OR condition on user role.
Here is an example usecase for a block showing diff/debug/editorial information:
AND
OR
Comment #84
trickfun CreditAttribution: trickfun commentedIs it possible to have this use case?
1. show A block on taxonomy term
OR
2. show A block on /my-custom-url
3. show in the previous page only if user is admin
thank you
Comment #86
flefle CreditAttribution: flefle as a volunteer commentedAttached bellow is the adopted patch for 10.2.x.
Comment #87
Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commentedAdded patch for 11.x
Comment #88
shweta__sharma CreditAttribution: shweta__sharma at OpenSense Labs commentedCCF error
Comment #89
Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commentedTried fixing CCF, attached interdiff for same.
Comment #90
smustgrave CreditAttribution: smustgrave at Mobomo commentedCaused a large number of failures.
Comment #91
onfire84 CreditAttribution: onfire84 at erdfisch commentedTryed Patch #86 on a 10.2.x Drupal Page. Patch does apply, but when i try to configure a block on the blocklayout page, it fails with "Error: Undefined constant "Drupal\block\form_state" in Drupal\block\BlockForm->buildVisibilityInterface() (line 242 of core/modules/block/src/BlockForm.php)."
Comment #92
johnlutzFixed the patch that applies against 10.2.x.
Comment #93
adriancruz CreditAttribution: adriancruz commentedPatch for 10.2.x
Comment #95
smustgrave CreditAttribution: smustgrave at Mobomo commentedHiding patches for clarity
Cleaned up issue summary so removing that tag.
Ran test-only feature and got https://git.drupalcode.org/issue/drupal-923934/-/jobs/1210671 but that shows the schema update
This shows the block placement test. https://git.drupalcode.org/issue/drupal-923934/-/jobs/1211130
So removing that tag as well
Turned patch #93 into an MR with a few changes
Removed some changes to the tests that seemed out of scope.
Turned the new condition into an attribute vs annotation usage.
Added new test coverage.
Suggestions appreciated
Comment #96
smustgrave CreditAttribution: smustgrave at Mobomo commentedPosted in slack and @berdir made a recommendation to take a deeper look at #59
Comment #99
smustgrave CreditAttribution: smustgrave at Mobomo commentedUpdated issue summary with new solution.
I've pushed changes to the new MR but there is a test failure I can't figure out why, with the default config.
Comment #100
smustgrave CreditAttribution: smustgrave at Mobomo commentedAll tests green, think ready for review of new approach.
Comment #101
acbramley CreditAttribution: acbramley at PreviousNext commentedLeft some comments on the MR, IMO the summary could use a rewording - it took me a few goes to understand what "hit" meant in this context.
Comment #102
smustgrave CreditAttribution: smustgrave at Mobomo commentedUpdated and cleaned out more to just include a single example.
Addressed feedback
Comment #103
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #104
smustgrave CreditAttribution: smustgrave at Mobomo commentedForgot the upgrade path test