Allow for block visibility rules

This is something that would make Page Manager in D8 considerably more useful to me. It's one of the reasons that I love D7 Panels.

Some use cases beyond the one supplied above:

• Show a block based on a user's role.
• Show or hide a block based on the value of a boolean checkbox, to allow the content editor to control if a block should be visible for a given node.
• Show a different Views block based on the value of a field. Similar to the last point, but in this case I might use a select box with multiple options. There would be a views block for each option. An example is using a different text overlay block on an image based on a selection made by the content editor. One option might be "Two line overlay" and another might be "Three line overlay with More button"

Here's a first pass at that, feedback welcome.

Hopefully fixed errors with existing tests (but I haven't added new ones). Btw I don't know if it's ok to change the signature for \Drupal\page_manager\Plugin\DisplayVariant\PageBlockDisplayVariant::__construct() while it's in beta?

So the tests are fixed on D8.4.x but there's a single error for 8.5.x. Sadly I don't have time right now to look into that.

1. +++ b/page_manager_ui/src/Form/ConditionSubform.php
   @@ -0,0 +1,85 @@
   +class ConditionSubform {
   

   Its super nice to have a piece of well written and even more important, battle tested code here.

   Maybe we could add a @todo to move this somewhere else in core, so its actually reusable :)

2. +++ b/page_manager_ui/src/Form/ConditionSubform.php
   @@ -0,0 +1,85 @@
   +   * @see \Drupal\block\BlockForm::validateVisibility()
   ...
   +   * @see \Drupal\block\BlockForm::submitVisibility()
   

   Let's ensure this also points to the page_manager equivalent

3. +++ b/page_manager_ui/src/Form/VariantPluginAddBlockForm.php
   @@ -29,11 +31,17 @@ class VariantPluginAddBlockForm extends VariantPluginConfigureBlockFormBase {
   +  public function __construct(SharedTempStoreFactory $tempstore, PluginManagerInterface $condition_manager, LanguageManagerInterface $language_manager, ContextRepositoryInterface $context_repository, PluginManagerInterface $block_manager) {
   +    parent::__construct($tempstore, $condition_manager, $language_manager, $context_repository);
   

   A general tip: If you add new dependencies you can make them optional, as this ensures 100% that none of other people's code is broken:

   
   class Example {
     /**
      * @var \Drupal\...\+++\
      */
     protected $dep;
   
     public function __construct(Dependency $dep = NULL) {
       $this->dep  = $dep ?: \Drupal::service('example.service');
     }
   }
   
   class Subclass extends Example {
     public function __construct(Dependency $dep = NULL) {
       parent::__construct($dep);
     }
   }
   
   

   This is particular useful for any kind of baseclasses.

4. +++ b/page_manager_ui/src/Form/VariantPluginConfigureBlockFormBase.php
   @@ -188,6 +228,31 @@ abstract class VariantPluginConfigureBlockFormBase extends FormBase {
   +      $visibility = empty($configuration['visibility']) ? [] : $configuration['visibility'];
   ...
   +    return empty($configuration['visibility']) ? [] : $configuration['visibility'];
   

   Some general coding style question: $visibility = empty($configuration['visibility']) ? [] : $configuration['visibility']; vs.

   $configuration +=
    ['visibility' => []]

   Do you have any particular preference?

5. +++ b/page_manager_ui/src/Form/VariantPluginConfigureBlockFormBase.php
   @@ -188,6 +228,31 @@ abstract class VariantPluginConfigureBlockFormBase extends FormBase {
   +      $this->visibilityCollection = new ConditionPluginCollection($this->conditionManager, $visibility);
   

   Note: This code could use $this->getVisibility()

6. +++ b/page_manager_ui/src/Form/VisibilitySubform.php
   @@ -0,0 +1,130 @@
   +class VisibilitySubform extends ConditionSubform {
   

   ... block module has its own UX "optimizations", so node_type, user_role etc .... I'm wondering whether this really makes sense in the context of page_manager, where you have ctools installed and as such a LOT of conditions. I am strongly wondering whether the UI should look more like the selection criterias.

Thanks for the thoughtful feedback @dawehner!

1.

Maybe we could add a @todo to move this somewhere else in core, so its actually reusable :)

Yeah, good idea! Done.

2.

Let's ensure this also points to the page_manager equivalent

This really is the page manager equivalent - I'm referencing where I stole the code from, and ideally what should be refactored to use this if it's moved into core.

3.

If you add new dependencies you can make them optional

Thanks! Done. PhpStorm's Change Signature FTW!

4.

Some general coding style question

Haha! Yeah, I wasn't overly happy with that little bit. I think your way looks better (:

5.

This code could use $this->getVisibility()

Doh! Thanks, done.

6.

I am strongly wondering whether the UI should look more like the selection criterias.

Yeah, that's a really good point - never thought about it. I've added a @todo, but for now I think what we have makes some kind of sense (: Also it looks like there's work being done on changing how core works, so it might be worth seeing how that pans out: #2284687: Redesign UI for better management of block visibility.

Thanks again!

Ok, well, PhpStorm's Change Signature does have some limitations (: I also needed to reorder the parameters passed to the mock builder. Interdiff is against #6.

It would make this module much better, same that in Drupal 7.
I will test it as soon as i can.
Thanks

Failures due to #2904887: Symfony 3 is incompatible with Page Manager

Add DependencySerializationTrait to ConditionSubform so it can be used in an AJAX form.

Thanks. I've applied #17 to a clean copy of Page Manager 8.x-4.0-beta2 but I'm not sure what I'm supposed to be seeing.

I have one page in Page Manager which uses a path of /node/%.

Within that, I have one variant which is selected if the node type is Service.

Within that, I have two blocks, Body and Webform.

I cannot see any UI that would allow me to set visibility rules on these blocks.

I imagine I'm missing something ...

@millionleaves Not sure, sorry, it works for me. When you add/edit a block using page manager you should see the same kind of block visibility tabs you see on the core block form. You're not using a Panels page are you? If so you'll also need the patch in #2769099: Block visibility conditions.

The following simplytest.me link works for me: https://simplytest.me/project/page_manager?patch[]=https://www.drupal.or...

(You'll need to enable page_manager and page_manager_ui first.)

Ah yes - that was the secret. I'm using Panels, and can now set the visibility of blocks based on node field values.

However, when viewing a node using a Panels variant containing a block where visibility of the block is based on a specified ID, I am now getting this error:

The website encountered an unexpected error. Please try again later.

The server log contains this:

Uncaught PHP Exception InvalidArgumentException: "You must provide the context IDs in the @{service_id}:{unqualified_context_id} format." at /path/to/mysite/core/lib/Drupal/Core/Plugin/Context/LazyContextRepository.php line 71

Removing the block visibility rule removes this error and displays the page as it was.

I've tried with the current and -dev versions of both Page Manager and Panels.

Thanks @millionleaves.

when viewing a node using a Panels variant containing a block where visibility of the block is based on a specified ID

I'm not entirely sure what kind of condition you've set up. Would you be able to provide an export of the page?

Thank!

Thanks. See attached. Is this what you're looking for?

@millionleaves Perfect, thanks! I'm not familiar with the node_field condition and I can't seem to find it in my current codebase... Do you know what provides it? What's its name in the UI?

Thanks

@AndyF

In this example, the node_field condition is ensuring that the webform block should only be displayed on a node with NID 5. It's from the Panels block visibility settings patch that you referenced in #19, where you can use node fields to determine the visibility of a block within a Panel. To recreate, you add a block to a (panels) variant. Within that you can choose Node Field to control visibility of the block.

@millionleaves Thanks. I hate to say it, but I have no idea where that condition comes from! It's not from the patches themselves (they just expose existing conditions via the UI). It doesn't seem to be part of panels or page_manager or node, I can't get any results searching on its machine name or label, and I can't find any solutions to enabling a block based on NID that don't use the request path condition.

Would you be able to type the following command from the document root of your codebase?

grep -Fr 'id = "node_field"' .

Hopefully that will show what defines the condition.

Thanks!

Here it is:

modules/entity_field_condition/src/Plugin/Condition/NodeField.php

https://www.drupal.org/project/entity_field_condition

Thanks @millionleaves, that's helped me fix a problem with using conditions with context mappings (which includes the node field) for page manager. I haven't tested it with Panels but fingers crossed it will work there too (please confirm!).

Thanks!

I've applied the patch to a fresh download of the dev version of Page Manager. Patch applies cleanly, and my site no longer blows up when I visit a page using a panels variant with block visibility conditions.

Unfortunately ....

Panels Pages are loading without the block that has visibility conditions, regardless of whether the conditions are met. I don't get this since another block in the same variant is working fine. May be the block type (the missing block is a Webform block, the other is the Body field).

and ...

I then tried applying the Panels Block Visibility patch to the latest -dev of Panels to see if that helped and found that I could not longer edit either of the blocks at all. I actually ran into this problem the other day and resolved it by reverting to the current stable version of Panels, so I doubt it's related to this latest patch.

So, I need to do some more testing of different conditions to pinpoint whether it's my setup or some combination of the Panels and Page Manager patches causing the problems. I'll try and do that in the next day or so.

If I see it correctly then this uses the core context repository. But page manager has its own context system, only then contexts for contexts from the url/static contexts will work.

Thanks @berdir! Forgive me, I'm not sure what you mean exactly by "url/static contexts". Do you have any suggestions for how I might trigger a failure? Just FYI the test I did before posting was creating a variant at /node/{nid} and making a block display only on node 1 using the node_field condition from #26.

I'm new to contexts by the way, so any and all help appreciated, thanks again!

Yes, sorry for the short drive-by comment, didn't have much time.

Yes, node probably works because core provides the context for that. But try the same with a page that has a taxonomy_term argument, either replacing /taxonomy/term/{taxonomy_term} or a custom route. Then configure that path argument to be a taxonomy_term.

You won't be able add a visibility condition now for a specific vocabulary. But if you have a block which requires a taxonomy_term context (for example a view with a term id argument and term validation), then you will be able to select and use the term provided by the path.

I don't have a full overview of how that works these days, but the contexts are stored in tempstore when editing, see \Drupal\page_manager_ui\Controller\PageManagerController::selectBlock() which selects the available blocks and provides the contexts for that and at runtime, you can get the contexts from the page variant, \Drupal\page_manager\Entity\PageVariantViewBuilder::view() sets them on the display variant, where you can access them with $this->getContexts().

Perfect, thanks @berdir!

I'd say https://www.drupal.org/node/2769099 is related to this.

Just wanted to give thanks to ppl above me; This worked for me as well. As a side note, be sure that you have the most updated version of both panels and page_manager modules (not the latest stable version) otherwise the patches won't work.

This in combination with the Entity_field_condition module, gives us the desired functionality, albeit less user friendly.

Thanks for all the work!

I've got some free time: I'm going to work on this a bit more, starting with some tests.

I've added some unit and functional tests, let's see what testbot has to say. Feedback welcome! I haven't looked into @berdir's comment in #31 at all.

Thanks!

There seems to be a little flakiness in the JS functional tests. I'm thinking it might be because I assumed that when the block title field appeared in the add block modal that the condition vertical tabs had also been set up. I've fixed it so that I wait on the element I'm actually clicking (doh!).

And fixed a few stragglers, fingers crossed...

Not sure why, but there's still some flakiness in the JS tests. It's not consistent, but tends to be around waiting for the condition's vertical tab and summary. It frequently occurs in Drupal\Tests\page_manager_ui\FunctionalJavascript\VisibilitySubformTest::testContentTypeCondition() where it asserts the content type link.

<?php
// Add the block.
$add_block_link = $assert->waitForLink('Add new block');
$this->assertNotEmpty($add_block_link);
$add_block_link->click();

$powered_by_link = $assert->waitForLink('Powered by Drupal');
$this->assertNotEmpty($powered_by_link);
$powered_by_link->click();

// Configure the condition.
$content_type_link = $assert->waitForLink('Content typesNot restricted');
$this->assertNotEmpty($content_type_link); // THIS INTERMITTENTLY FAILS FOR SOME REASON.
$content_type_link->click();
?>

Just a minor addition that converts the visibility subform helper into a service for easier injection.

trigger testbot and summary update

Access plugins in the panels world in Drupal 7 could be added to a pane/region/page etc. on the fly, so there were no plugins active until you added one. From that point on you could add two access plugins to a pane and give them different values. That's what made the access system in panels superior to the core block visibility in D7.

Additionally these access plugins are applied and available for a pane right away (pane config screen), so there is not way to just say "I don't want this one" - this should be done in code... Also I guess if having lots of plugins on a site, it will be very difficult to track which ones are available and really used.

I am playing around with the patch here and in https://www.drupal.org/project/panels/issues/2769099 and while it seems to work (I am still discovering some issues with a custom condition, but I'll update on that later) the solution seems to be as the one we had in D7.

Over at 2769099 there was a proposal to introduce a panels access plugin, see https://www.drupal.org/project/panels/issues/2769099#comment-12049274 - I am wondering if we should consider building on that?

See this screenshot for hopefully better understanding:
https://ibb.co/fgFq5J

Ok, I see there is https://www.drupal.org/project/block_visibility_groups

Hopefully this can be managed better with it

Thanks @drupov,

Access plugins in the panels world in Drupal 7 could be added to a pane/region/page etc. on the fly, so there were no plugins active until you added one.

From a backend POV that's what we're doing/simulating, but the UI arguably doesn't match that very well. (Drupal\Core\Condition\ConditionPluginCollection::getConfiguration() detects whether the condition config is the same as the default config and only saves conditions which are actually configured.)

From that point on you could add two access plugins to a pane and give them different values.

I'm not sure exactly what you're getting at. It's possible to have two conditions on a block, but I guess that's not what you mean?

I guess if having lots of plugins on a site, it will be very difficult to track which ones are available and really used.

Thanks, yeah that's a really good point. @dawehner mentioned the UX issues with it in #2858877-9: Allow for block visibility rules point 6 which led to a @todo being added and there's a core block issue #2284687: Redesign UI for better management of block visibility. At the moment we're just copying the core UI which I guess makes sense at least to try and get the initial functionality in? When #2284687: Redesign UI for better management of block visibility lands we can refactor it.

I am still discovering some issues with a custom condition, but I'll update on that later

If you do find any issues, it would be great if you could see if the issue manifests only with panels, or both with page manager and panels, and report back on the appropriate thread (thanks!).

Over at 2769099 there was a proposal to introduce a panels access plugin

The difference in approach there is much more than the UI. The patch in #2769099-10: Block visibility conditions as I understand it creates a new kind of plugin, which means you can't use existing (including contrib) conditions. I wanted to be able to use the same condition for a variant as for a block.

I am wondering if we should consider building on that?

Just for the UI, or is there something else?

Visibility rules available on a page manager page, where not even a node context exists.

I'm far from an expert here, but as I understand it you're not seeing all conditions. From what I can gather (ie take with a pinch of salt) the reason you see a node condition is because the Node module has a context provider service (Drupal\node\ContextProvider\NodeRouteContext) which kind of makes the context globally available (even if not always populated with an actual node entity). But there isn't a similar context provider for taxonomy terms (no idea why not), so as @berdir mentioned in #31 to make the vocabulary condition work on an overridden taxonomy term page the patch needs to show conditions for the specific contexts associated with the page as well as the global ones. As I say, I'm still feeling my way around this area a bit!

Thanks again!

Thanks!

> From that point on you could add two access plugins to a pane and give them different values.

I'm not sure exactly what you're getting at. It's possible to have two conditions on a block, but I guess that's not what you mean?

I mean two same access plugins, sorry

Thanks, yeah that's a really good point. @dawehner mentioned the UX issues with it in #2858877-9: Allow for block visibility rules point 6 which led to a @todo being added and there's a core block issue #2284687: Design a GUI for managing block visibility. At the moment we're just copying the core UI which I guess makes sense at least to try and get the initial functionality in? When #2284687: Design a GUI for managing block visibility lands we can refactor it.

Totally agree. It will be best to move forward.

If you do find any issues, it would be great if you could see if the issue manifests only with panels, or both with page manager and panels, and report back on the appropriate thread (thanks!).

trying :)

The difference in approach there is much more than the UI. The patch in #2769099-10: Block visibility conditions as I understand it creates a new kind of plugin, which means you can't use existing (including contrib) conditions. I wanted to be able to use the same condition for a variant as for a block.

Absolutely. We also have the mentioned #2284687: Design a GUI for managing block visibility.

Just for the UI, or is there something else?

UI, but as said, that will be tackled anyway.

Thanks again for these contributions, really appreciate it!

I'm going to take a look at integrating page manager contexts.

Here's a first attempt at integrating page manager contexts.

As @drupov mentioned we can't add the same condition more than once, so for example if there are two term contexts then we can't have a condition on both of them. (I hadn't originally realised that the UI change would impact functionality.) I'm not sure if this is something to address immediately or in a follow-up issue? Also I could really do with someone who knows something about contexts casting an eye over the patch if at all possible (:

Thanks!

Here's a first attempt at integrating page manager contexts.

Works for me! If there is a context you can apply it to the condition:
https://ibb.co/fc1LUo

One thing is a little bit messy maybe: for example if this demo context is not there, the condition for it would still be displayed:
https://ibb.co/df1Upo
The best seems to be conditions to be added when needed (kind of what "Block Visibility Group" does: https://ibb.co/jHB7aT), so start with an empty set and add those available through context when needed.

Maybe it's really worth thinking about the whole "we can't add the same condition more than once"-thing to be done here. This was one of the main advantages of the access system for Panels in Drupal 7.

Anyway, feels like real progress here, thanks @AndyF!

Thanks for the testing and feedback @drupov!

One thing is a little bit messy maybe: for example if this demo context is not there, the condition for it would still be displayed

This boils down to the context providers mentioned at the end of #49. What I've done is include contexts both from the "global" providers (which you get by calling getAvailableContexts() on the context.repository service) and the contexts from the page variant. It's true that if you try to add a condition under a variant's selection criteria it only shows conditions for the page contexts, not the ones from context.repository. Maybe we should do the same? I guess any context that would be provided by the repository can be added explicitly via page manager, so you should still be able to access all the same conditions...

Maybe it's really worth thinking about the whole "we can't add the same condition more than once"-thing to be done here.

It definitely seems a higher priority now it's more than just a UX improvement, it actually limits functionality. Also I guess it would affect the config schema and might involve a hook_update_N() if we were to make the change later. OTOH it was kinda nice to be relying on code stolen from core, we might be duplicating to some extent work done in #2284687: Redesign UI for better management of block visibility and there's always the risk this issue will linger! That said, I'm not sure how much more work it would be: we can maybe (mostly) copy the UI for page variant selection criteria.

That said, I'm not sure how much more work it would be: we can maybe (mostly) copy the UI for page variant selection criteria.

You're right and also that part (Add selection criteria functionality for panes) would fit better as a Panels issue?

I think with the work you've done here we've made good progress into solving this issue, so maybe it's really better to try to make it RTBC and handle the rest separately.

Hi all, thanks so much for your work on this feature!

We've applied the patch in #55, and it appears to have applied cleanly, but we're not seeing any of the UI elements shown in the screenshots, which we would use to apply selection rules/conditions.

Are there any other steps needed to test the patch, other than applying it?

It no longer applies to the latest dev (april 2019)

Patch ported with no additional review except some CS. Interdiff fails. After update, a cache rebuild seems to be required.

I tried the patch from #60. It applied cleanly against "8.x-4.0-beta4, released 29 May 2019". I cleared the cache after applying the patch.
Nothing changed... There were no visibility rules available.

Which kind of page manager variant, you are using? Blocks?

From the config file: "variant: panels_variant"

Using the standard page from total_control dashboard module. I expected the blocks in the content to have visibility rules.

Sorry, can't confirm what #62 is saying actually, but at first i could (edited this post).

I first updated all my modules, then applied patch @ #60, but forgot for this to work, i also have to patch the panels module.
Seems to work after i did that.

So for anyone who comes across this, try that :)

Re-roll of #60, no additional changes. Interdiff fails due to changes in base code.

When trying this patch I got the following so I think this patch needs a bit more work. Also there is a test failure as seen above.

 dropsolid8    www/html$ drush updb
The following updates are pending:

page_manager module :
  Updating page variants with new current_user context name.

page_manager_ui module :
  Force cache clear to ensure aggregated JavaScript files are regenerated.   @see https:www.drupal.orgprojectpage_managerissues2921404

Do you wish to run all pending updates? (y/n): y
Invalid argument supplied for foreach() page_manager.post_update.php:18                                                                                                                                                                                            [warning]
Invalid argument supplied for foreach() page_manager.post_update.php:18                                                                                                                                                                                            [warning]
... (many more)
Werkt page_manager bij                                                                                                                                                                                                                                             [ok]
Werkt page_manager_ui bij

Somehow this was a duplicate

#66 applied without problems and works perfectly after applying the patch here as well: #2769099: Block visibility conditions. Thank you!

The patch on #66 needed a re-roll:

Checking patch config/schema/page_manager.schema.yml...
Checking patch page_manager.services.yml...
Checking patch page_manager_ui/page_manager_ui.services.yml...
Checking patch page_manager_ui/src/Form/ConditionSubform.php...
Checking patch page_manager_ui/src/Form/VariantPluginAddBlockForm.php...
Checking patch page_manager_ui/src/Form/VariantPluginConfigureBlockFormBase.php...
Checking patch page_manager_ui/src/Form/VisibilitySubform.php...
Checking patch page_manager_ui/src/Form/VisibilitySubformInterface.php...
Checking patch page_manager_ui/tests/src/FunctionalJavascript/VisibilitySubformTest.php...
Checking patch page_manager_ui/tests/src/Unit/ConditionsSubformTest.php...
Checking patch page_manager_ui/tests/src/Unit/VisibilitySubformTest.php...
Checking patch src/ConditionAccessResolver.php...
Checking patch src/ConditionAccessResolverInterface.php...
Checking patch src/Plugin/DisplayVariant/PageBlockDisplayVariant.php...
Checking patch tests/src/Unit/ConditionAccessResolverTest.php...
Checking patch tests/src/Unit/PageBlockDisplayVariantTest.php...
Hunk #1 succeeded at 9 (offset 1 line).
Hunk #2 succeeded at 17 (offset 1 line).
Hunk #3 succeeded at 50 (offset 2 lines).
Hunk #4 succeeded at 144 (offset 3 lines).
error: while searching for:
    $block_manager = $this->prophesize(BlockManager::class);
    $condition_manager = $this->prophesize(ConditionManager::class);
    $variant_plugin = $this->getMockBuilder(PageBlockDisplayVariant::class)
      ->setConstructorArgs([['page_title' => $page_title, 'uuid' => 'UUID'], 'test', [], $context_handler->reveal(), $account->reveal(), $uuid_generator->reveal(), $token, $block_manager->reveal(), $condition_manager->reveal(), $module_handler->reveal()])
      ->setMethods(['renderPageTitle'])
      ->getMock();

I have re-rolled the attached patch.

error: patch failed: tests/src/Unit/PageBlockDisplayVariantTest.php:148
error: tests/src/Unit/PageBlockDisplayVariantTest.php: patch does not apply

cc - https://www.drupal.org/project/panels/issues/2769099#comment-14792705

Got working. Thanks for the patch NWOM!

I have applied below but conditions are still not available as before, reference #72.

Panels: Custom attributes in panels blocks and variants
https://www.drupal.org/project/panels/issues/2849867#comment-14791140

Page Manager: Allow for block visibility rules
https://www.drupal.org/project/page_manager/issues/2858877#comment-14787260

Hi Sseto, what recipe of module versions and patches did you use to get it working again? For some reason, I'm not able to get the visibility conditions back. Thank you, Crutch

Hi NWOM,

Patch( page_manager-block_visibility_conditions-2858877-71.patch ) not working for drupal 9.5.9 and page manager 8.x-4.0-rc2.

Hi,
I have created and added the patch. that is working for drupal 9.5.9 and page manager 8.x-4.0-rc2.
Kindly review it.

#76 works on 9.5.11 as well! Thanks for the updated patch

Can https://www.drupal.org/project/context_stack be used with in page_manager to help determine the context?

Is there any reason why the ConditionAccessResolver has been removed from available services?

It can be useful for other modules, in my case here: #2769099: Block visibility conditions

Here's a patch that brings the resolver back as a service, it must be used on top of the issue's merge request.

I'd be happy to include it in the MR.