Add support for OR in block visibility conditions

What 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

It 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.

I 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:

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

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.

Right, 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.

Follow up in #960864: Block visibility settings UI is confusing - unclear about how settings will be applied.

I'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.

Check out https://www.drupal.org/project/block_visibility_groups

First, 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.

The 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.

Having 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.

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

Patch in #15 does exactly what is needed. Block Visibility Groups is a very nice module that should be included in core at some point!

This needs automated test coverage before it can go in.

I have manually tested the patch and tested. It seems to be working as expected.

This still doesn't have automated test coverage so it can't be RTBC.

Have manually applied this patch and tested, seems to be working fine.

Reroll last patch.

I can confirm the last patch applied to 8.6.2. The author didn't mention tests so we likely still need those.

The last reroll applied properly but missed a chunk a code. Reroll 2 attached.

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

Referencing another issue related to block visibility conditions.

Added reroll for Drupal-9.2.x.

Reroll against dev-9.2.x

Fixed Custom failure errors in #37 in the following patch, thanks!

Updated patch in #38, thanks!

Rerolled the patch to apply against 9.1.10

Re-rolled against 9.3. Please review.

Patch apply successfully for reference sharing screenshot

Unfortunately the patch in #39 didn't apply cleanly for me on 9.2.0, so I have re-rolled for 9.2.0.

Patch 923934-40.patch in #41 is missing condition class AndOrCondition declaration.
Applying 923934-44.patch instead against 9.1.10 works.

This 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

Rerolled against latest 9.2.x and 9.3.x branches.

The patches in #47 are missing the main plugin condition file core/modules/system/src/Plugin/Condition/AndOrCondition.php

Here's a re-roll of 44 for 9.2.x.

Still needs a re-roll for 9.3.x.

Re-rolled patch #47, against 9.3.x. Please review. attached interdiff for same.

As 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.

Rerolled the patch in #44 against 9.3.x as suggested in #53, thanks!

Looks like commit-code-check failed.

Fixing commit-code-check.sh failures.
coding standards tests are passing on local now. Should be good to go.

----------------------------------------------------------------------------------------------------
Checking core/modules/block/js/block.es6.js

yarn run v1.22.10
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/mohit/Experiments/dc/drupal8/core/modules/block/js/block.es6.js
[10:07:23] '/Users/mohit/Experiments/dc/drupal8/core/modules/block/js/block.es6.js' is being checked.
✨  Done in 0.92s.
core/modules/block/js/block.es6.js passed

Attempting to fix the test cases.

+++ b/core/modules/block/src/BlockForm.php
@@ -262,7 +262,7 @@ protected function buildVisibilityInterface(array $form, FormStateInterface $for
-        $form['node_type']['#title'] = $this->t('Content types');
+        $form['node_type']['#title'] = $this->t('Content types (Deprecated)');
       }

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.

1. +++ b/core/modules/block/src/BlockForm.php
   @@ -255,36 +260,23 @@ protected function buildVisibilityInterface(array $form, FormStateInterface $for
   -      $form['node_type']['bundles']['#title'] = $this->t('Content types');
   -      $form['node_type']['negate']['#type'] = 'value';
   -      $form['node_type']['negate']['#title_display'] = 'invisible';
   -      $form['node_type']['negate']['#value'] = $form['node_type']['negate']['#default_value'];
   +      if (isset($form['node_type'])) {
   +        $form['node_type']['#title'] = $this->t('Content types (Deprecated)');
   +      }
   +      if (isset($form['user_role'])) {
   +        $form['user_role']['#title'] = $this->t('Roles');
   +      }
   +      if (isset($form['request_path'])) {
   +        $form['request_path']['#title'] = $this->t('Pages');
   +      }
        }
   +
        if (isset($form['entity_bundle:node'])) {
          $form['entity_bundle:node']['negate']['#type'] = 'value';
   

   the 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.

2. +++ b/core/modules/block/src/BlockForm.php
   @@ -325,6 +317,16 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
        // Validate visibility condition settings.
   +    if ($path = $form_state->getValue(['visibility', 'request_path'])) {
   +      // Forcibly set FALSE for negate if there is no pages defined,
   +      // otherwise the block will not be displayed even there is no any other
   +      // restrictions set.
   +      // @see \Drupal\Component\Plugin\Exception\ContextException\ConditionAccessResolverTrait
   +      if ((empty($path['pages']) || $path['pages'] === '*') && !empty($path['negate'])) {
   +        $form_state->setValue(['visibility', 'request_path', 'negate'], FALSE);
   +      }
   +    }
   

   shouldn't that be in the plugin itself?

3. +++ b/core/modules/node/tests/src/Functional/NodeBlockFunctionalTest.php
   @@ -167,11 +167,16 @@ public function testRecentNodeBlock() {
        $this->assertSession()->pageTextNotContains($label);
   -    $this->assertCacheContexts(['languages:language_content', 'languages:language_interface', 'theme', 'url.query_args:' . MainContentViewSubscriber::WRAPPER_FORMAT, 'url.site', 'user', 'route']);
   +    $this->assertCacheContexts(['languages:language_content', 'languages:language_interface', 'theme', 'url.query_args:' . MainContentViewSubscriber::WRAPPER_FORMAT, 'url.site', 'user']);
   +
   +    // Front-page will be un-cacheable due to limitation added in block configuration.
   +    $this->assertSession()->responseHeaderEquals('X-Drupal-Dynamic-Cache', 'UNCACHEABLE');
    
   -    // Ensure that a page that does not have a node context can still be cached,
   -    // the front page is the user page which is already cached from the login
   -    // request above.
   +    // Create an article node and visit it.
   +    $node7 = $this->drupalCreateNode(['uid' => $this->adminUser->id(), 'type' => 'article']);
   +    $this->drupalGet('node/' . $node7->id());
   +    $this->assertSession()->responseHeaderEquals('X-Drupal-Dynamic-Cache', 'MISS');
   +    $this->drupalGet('node/' . $node7->id());
        $this->assertSession()->responseHeaderEquals('X-Drupal-Dynamic-Cache', 'HIT');
   

   I don't really understand why this test changes. why should changes to the UI affect caching?

4. +++ b/core/modules/system/src/Plugin/Condition/AndOrCondition.php
   @@ -0,0 +1,93 @@
   + * Provides an 'And Or' condition.
   + *
   + * @Condition(
   + *   id = "and_or",
   + *   label = @Translation("Conditions logic"),
   + * )
   + */
   +class AndOrCondition extends ConditionPluginBase implements ContainerFactoryPluginInterface {
   +
   

   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?

NW for #59.

Any update?

patch #58 does not apply to core 9.4.x any more.

Verified 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

The 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.

Understand 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.

Apologies for uploading a corrupted patch in 72 (contained a merge conflict) - I attach a working re-roll for 9.5.x.

Adding an other issue this is somewhat related to (#3264843).

Adding an other update here respecting #3264843, which should somehow also address #59 question 1.

Took me a moment, but now I can tell - the changed conditions logic does not work properly:

--- a/core/modules/block/src/BlockAccessControlHandler.php
+++ b/core/modules/block/src/BlockAccessControlHandler.php
@@ -85,20 +85,35 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
       $conditions = [];
       $missing_context = FALSE;
       $missing_value = FALSE;
-      foreach ($entity->getVisibilityConditions() as $condition_id => $condition) {
+      $visibility = $entity->getVisibilityConditions();
+      $available_contexts = count($visibility);
+      $and_or = 'and';
+      if ($visibility->has('and_or')) {
+        $and_or = $visibility->get('and_or')->getConfiguration()['all'];
+        // This condition was need just to fetch and/or config setting.
+        $visibility->removeInstanceId('and_or');
+      }
+      foreach ($visibility as $condition_id => $condition) {
         if ($condition instanceof ContextAwarePluginInterface) {
           try {
             $contexts = $this->contextRepository->getRuntimeContexts(array_values($condition->getContextMapping()));
             $this->contextHandler->applyContextMapping($condition, $contexts);
+            $conditions[$condition_id] = $condition;
           }
           catch (MissingValueContextException $e) {
-            $missing_value = TRUE;
+            $available_contexts--;
+            // The required or last condition checked has no context.
+            if (($and_or == 'and') || !$available_contexts) {
+              $missing_context = TRUE;
+            }
           }
           catch (ContextException $e) {
             $missing_context = TRUE;
           }
         }
-        $conditions[$condition_id] = $condition;
+        else {
+          $conditions[$condition_id] = $condition;
+        }
       }
 
       if ($missing_context) {

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 be TRUE.
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 but FALSE. The case should be dropped, the variable removed when not required anymore.

Please also note, I fixed in this Version :

• Missing context vs. context value, a cache issue, as stated in the comment in code.
• The conditions count was created prior the and-or plugin was removed.
• The form
• I applied the same logic in the radio negate switch form to all considerable elements in the form.
• I choose to stick to jQuery in block.js

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.

Sorry, 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.

One 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.

Fixed CCF for #81 in 9.5.x.

I 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

@ #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:

1. Show this block on taxonomy term pages only
   
   AND
2. Show this block on admin or edit pages only
   
   OR
3. Show this block if user has taxonomy editor role

Is 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

Attached bellow is the adopted patch for 10.2.x.

Added patch for 11.x

CCF error

Tried fixing CCF, attached interdiff for same.

Caused a large number of failures.

Tryed 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)."

Fixed the patch that applies against 10.2.x.

Patch for 10.2.x

Hiding 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

Posted in slack and @berdir made a recommendation to take a deeper look at #59

Updated 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.

All tests green, think ready for review of new approach.

Left 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.

Updated and cleaned out more to just include a single example.

Addressed feedback

Forgot the upgrade path test