Add support for OR in block visibility conditions

Created on 27 September 2010, about 14 years ago
Updated 2 April 2024, 8 months ago

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.

Feature request
Status

Needs work

Version

11.0 🔥

Component
Block 

Last updated about 13 hours ago

Created by

🇭🇰Hong Kong mertskeli

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇩🇪Germany Daniel Kulbe Berlin

    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.

  • 🇩🇪Germany Daniel Kulbe Berlin

    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.

  • 🇩🇪Germany Daniel Kulbe Berlin

    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.

  • 🇮🇳India _utsavsharma

    Fixed CCF for #81 in 9.5.x.

  • 🇮🇹Italy trickfun

    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

  • 🇮🇹Italy kopeboy Milan

    @ #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
  • 🇮🇹Italy trickfun

    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

  • 🇩🇪Germany flefle

    Attached bellow is the adopted patch for 10.2.x.

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.6
    last update 11 months ago
    Patch Failed to Apply
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.2 & sqlite-3.34
    last update 11 months ago
    Patch Failed to Apply
  • Status changed to Needs review 11 months ago
  • 🇮🇳India gauravvvv Delhi, India

    Added patch for 11.x

  • last update 11 months ago
    Custom Commands Failed
  • Status changed to Needs work 11 months ago
  • Status changed to Needs review 11 months ago
  • 🇮🇳India gauravvvv Delhi, India

    Tried fixing CCF, attached interdiff for same.

  • last update 11 months ago
    25,956 pass, 1,830 fail
  • Status changed to Needs work 11 months ago
  • 🇺🇸United States smustgrave

    Caused a large number of failures.

  • 🇩🇪Germany onfire84

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

  • 🇺🇸United States johnlutz

    Fixed the patch that applies against 10.2.x.

  • 🇵🇭Philippines adriancruz

    Patch for 10.2.x

  • Merge request !7270Resolve #923934 "Block visibility or" → (Closed) created by smustgrave
  • Pipeline finished with Failed
    8 months ago
    Total: 659s
    #134736
  • Pipeline finished with Success
    8 months ago
    Total: 642s
    #134751
  • Pipeline finished with Failed
    8 months ago
    Total: 166s
    #134785
  • Status changed to Needs review 8 months ago
  • 🇺🇸United States smustgrave

    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

  • Pipeline finished with Success
    8 months ago
    Total: 1042s
    #134786
  • Status changed to Needs work 8 months ago
  • 🇺🇸United States smustgrave

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

  • Pipeline finished with Canceled
    8 months ago
    #134898
  • Pipeline finished with Failed
    8 months ago
    Total: 198s
    #134903
  • Pipeline finished with Failed
    8 months ago
    Total: 754s
    #134908
  • 🇺🇸United States smustgrave

    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.

  • Pipeline finished with Failed
    8 months ago
    Total: 725s
    #134922
  • Pipeline finished with Canceled
    8 months ago
    Total: 125s
    #134934
  • Pipeline finished with Failed
    8 months ago
    Total: 752s
    #134936
  • Pipeline finished with Failed
    8 months ago
    Total: 314s
    #134946
  • Pipeline finished with Failed
    8 months ago
    #134948
  • Pipeline finished with Canceled
    8 months ago
    Total: 157s
    #134953
  • Pipeline finished with Failed
    8 months ago
    Total: 691s
    #134954
  • Pipeline finished with Success
    8 months ago
    Total: 568s
    #134961
  • Status changed to Needs review 8 months ago
  • 🇺🇸United States smustgrave

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

  • Status changed to Needs work 8 months ago
  • 🇦🇺Australia acbramley

    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.

  • Status changed to Needs review 8 months ago
  • 🇺🇸United States smustgrave

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

    Addressed feedback

  • Pipeline finished with Failed
    8 months ago
    Total: 722s
    #135796
  • Status changed to Needs work 8 months ago
  • 🇺🇸United States smustgrave

    Forgot the upgrade path test

Production build 0.71.5 2024