Allow views contextual filters to expose the context using argument validation plugins

In general I guess we have to also change the UI in views to explain that. Previously the UI told the user that blocks don't get arguments but just use "provide default argument from X".

1. +++ b/core/modules/views/src/Plugin/Block/ViewsBlock.php
   @@ -28,7 +27,25 @@ class ViewsBlock extends ViewsBlockBase {
   +          $value = $this->context[$argument_name]->getContextValue();
   +          if ($value instanceof EntityInterface) {
   +            $value = $value->id();
   +          }
   +          $args[] = $value;
   

   The code should at least explain why we need to special case entities.

2. +++ b/core/modules/views/src/Plugin/Derivative/ViewsBlock.php
   @@ -105,9 +105,22 @@ class ViewsBlock implements ContainerDerivativeInterface {
   +                if (strpos($argument['validate']['type'], 'entity:') !== FALSE) {
   

   Oh, but we just expose entities here?

+++ b/core/modules/views/src/Plugin/Block/ViewsBlock.php
@@ -28,7 +27,25 @@ class ViewsBlock extends ViewsBlockBase {
+            $value = $value->id();

In views_content in drupal 7 you can specify which property of the entity you want to use.

You might have an contextual filter on the username column of the user table and then state that the required context is user and the property to use is the username.

@Berdir, surely it's part of the Block Plugin? (proven by the fact that the ID is being extracted from the context in the block plugin). In drupal 7 the view decides what property it takes from the context using an Argument Input setting in the Display Settings section.

here's a re-roll

$ patch -p1 < views-block-context-2287073-10.patch 
patching file core/modules/views/src/Plugin/Block/ViewsBlock.php
patching file core/modules/views/src/Plugin/Derivative/ViewsBlock.php
Hunk #2 succeeded at 106 with fuzz 2.

the interdiff is a bit weird to me, but i guess its alright? :)

$ diff views-block-context-2287073-10.patch views-block-context-2287073-11.patch 
44c44
< @@ -114,4 +132,23 @@ class ViewsBlock extends ViewsBlockBase {
---
> @@ -114,4 +132,23 @@ public function getMachineNameSuggestion() {
69c69
< index cec9895..9164688 100644
---
> index 3ebfc5f..134e269 100644
80c80
< @@ -105,9 +106,27 @@ class ViewsBlock implements ContainerDeriverInterface {
---
> @@ -105,9 +106,27 @@ public function getDerivativeDefinitions($base_plugin_definition) {
82c82
<                'entity' => array(
---
>                'config' => array(

i just tested this with the latest dev of page_manager and #2377757: Expose Block Context mapping in the UI

when you create a view with a node argument and set validation to check for the entity type

then the argument / contextual filter context will be exposed in page manager

pretty neat!

If there is no value in the context, the argument passed to the view should not be NULL but rather the exception value ('all').

#2375695-22: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed pointed out that it's not clear how to make this set the right cache contexts. Could you explain in a bit more detail the obstacles you're seeing?

AFAICT this is the problem: Views determines which cache contexts to set when rendering a view display by looking at the cache contexts stored in the view's config entity. Those were calculated at save time. They're calculated at save time because it's extremely expensive to initialize all handlers & plugins to just calculate the cache contexts necessary: that'd make the cost of things you have to do *before* you can use the render cache so very expensive that the impact of render caching is relatively low.
But with this feature, that assumption is no longer correct, because we can only know all the cache contexts involved at run time, since it is a dynamic (because conditional) argument.

Assigning to dawehner to get his thoughts.

If there is no value in the context, the argument passed to the view should not be NULL but rather the exception value ('all').

I think it should be NULL ... you can still configure your view to show every entry, in case there is no argument specified, or do I see something wrong?

Regarding #15
As we have seen in #2381277: Make Views use render caching and remove Views' own "output caching" we don't need render array based caching, for blocks, because all what is needed there is to bubble up the cache metadata from the rendered array, so it can be leveraged the next time you render the view, using cache redirects.

Isn't all we need on runtime for context contexts to say which cache contexts they have and add that to the render array? This then would not have to involve views land at all, and all the expensiveness would not matter much?

I think it should be NULL ... you can still configure your view to show every entry, in case there is no argument specified, or do I see something wrong?

Ah, yes, you might be right. But then something else in the argument handling is wrong. For a view with two arguments, if I only pass something to the second, it will be NULL for the first (without #14). Then in ViewExecutable::_buildArguments(), the if (isset($arg) || $argument->hasDefaultArgument()) condition is false (hasDefaultArgument() returns FALSE for the option "Display all results for the specified field" that you refer to) and then the second argument is not even considered. I am not familiar enough with the internals of Views to see what action is appropriate here, but one suggestion would be that the corresponding else { break; } should really be else { continue; }.

Broken blocks is a major bug in context / plugin system, not a featutre request.

#20: That depends on the brokenness of things ;). We should not ship with unusable things that half-work; that only frustrates developers.

If this doesn't get fixed in core, we'll do it in CTools, for sure, because this was possible with the 'Content pane' display in CTools in D7 and we'll need to continue having that functionality.

Working on tests

Re-rolled the patch. Uploading an updated patch.

Reuploading patch

Great work! I was always amused how shamelessly blocks ignore view arguments.

1. +++ b/core/modules/views/src/Plugin/Block/ViewsBlock.php
   @@ -30,7 +30,25 @@ class ViewsBlock extends ViewsBlockBase {
   +    $this->view->display_handler->getOption('arguments');
   

   Do we really need to call this here? It seems to do nothing useful or am I missing something?

2. +++ b/core/modules/views/src/Tests/Plugin/ContextualFiltersBlockContextTest.php
   @@ -0,0 +1,65 @@
   + * Contains \Drupal\views\Tests\plugins\ContextualFiltersBlockContextTest.
   

   s/plugins/Plugins

3. +++ b/core/modules/views/src/Tests/Plugin/ContextualFiltersBlockContextTest.php
   @@ -0,0 +1,65 @@
   +  public function testBlockContext() {
   +
   +    $this->drupalLogin($this->drupalCreateUser(array('administer views', 'administer blocks')));
   +    $this->drupalGet('admin/structure/block');
   +    $this->clickLinkPartialName('Place block');
   +    $this->drupalPlaceBlock('views_block:test_view_block3-block_1', array('label' => 'test_view_block3-block_1:1'));
   +
   +    $definition = \Drupal::service('plugin.manager.block')->getDefinition('views_block:test_view_block3-block_1');
   +
   +    $this->assertTrue($definition['context']['nid']);
   +
   +  }
   

   Should we remove first and last empty lines?

36.1 Removed doesn't seem to do anything
36.2 Good eyes
36.3 We should

Tested it on the latest core code and it applies.

So there is a case when you can "check" specify validation criteria but the validator is left at -basic validator-

Just one nit-pik.

+++ b/core/modules/views/src/Plugin/Derivative/ViewsBlock.php
@@ -111,9 +112,33 @@ public function getDerivativeDefinitions($base_plugin_definition) {
+                if (!isset($argument['validate'])) {
+                  continue;
+                }

Let's do empty check on $argument['validate']['type'] as we actually use that in the code below.

#43 reroll

with the tiny fix from #44

Corrected fix for #44

Yay for RC!

Looks good to me. Great work everyone!

Last patch needs to be re-rolled. I'm on it.

Re-rolled #51. In conflict with #2533800: Remove all unused use statements from core

... and back to RTBC.

#59 Fixed

8.1.x it is then.

+++ b/core/modules/views/src/Plugin/Derivative/ViewsBlock.php
@@ -111,9 +112,33 @@ public function getDerivativeDefinitions($base_plugin_definition) {
+                elseif ($argument['validate']['type'] == 'numeric') {
+                  $this->derivatives[$delta]['context'][$argument_name] = new ContextDefinition('integer', $argument_name, FALSE);
...
+                  case 'numeric':
+                    $this->derivatives[$delta]['context'][$argument_name] = new ContextDefinition('integer', $argument_name, FALSE);

Can we assume that integer contexts are correct for numeric arguments?

Also the test seems a bit light for all the conditions contained in the patch. The issue summary suggests that only entity support is added but the patch seems to add more.

1. +++ b/core/modules/views/src/Plugin/Derivative/ViewsBlock.php
   @@ -111,9 +112,33 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   +          if ($arguments = $display->getOption('arguments')) {
   +            foreach ($arguments as $argument_name => $argument) {
   +              if (!empty($argument['specify_validation'])) {
   

   Is there a reason why we not use $display->getHandler('arguments')

2. +++ b/core/modules/views/src/Plugin/Derivative/ViewsBlock.php
   @@ -111,9 +112,33 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   +                  $this->derivatives[$delta]['context'][$argument_name] = new ContextDefinition('integer', $argument_name, FALSE);
   ...
   +                    $this->derivatives[$delta]['context'][$argument_name] = new ContextDefinition('integer', $argument_name, FALSE);
   

   So the second parameter is some form of label. Should we rather use $argument->options['title'] instead?

3. +++ b/core/modules/views/src/Tests/Plugin/ContextualFiltersBlockContextTest.php
   @@ -0,0 +1,63 @@
   +  public static $modules = array('block',
   +    'block_test_views', 'views_ui', 'node'
   +  );
   

   This looks super weird to be honest :)

4. +++ b/core/modules/views/src/Tests/Plugin/ContextualFiltersBlockContextTest.php
   @@ -0,0 +1,63 @@
   +  public static $testViews = array('test_view_block3');
   

   Any reason we don't use something descriptive like 'test_view_block_with_context'?

Addressed #65.

+++ b/core/modules/views/src/Plugin/Block/ViewsBlock.php
@@ -24,19 +24,17 @@ class ViewsBlock extends ViewsBlockBase {
+      if ($argument = $this->view->display_handler->getHandler('argument', $context_name) && $value = $context->getContextValue()) {

Be careful with the assignment here, AFAIK you need to wrap that first one in parens.

Is it just me or does this patch have kind of a week test coverage? For example we don't test that the actual context passing actually works.

Be careful with the assignment here, AFAIK you need to wrap that first one in parens.

Tim is right about that, see https://3v4l.org/OdUV8

Added some test coverage.

That interdiff is incomplete. You fixed my point in #68 but it's not in there...
Did that test fail without the fix for #68?

Your point from #68 was fixed in #69 and is also in the interdiff there.

I do get some notices when running test on pre #69 code locally. Uploading patch for testbot to check it.

#71 is strange as some .orig files sneaked in. Re-uploading.

Expected fail.

+++ b/core/modules/views/src/Plugin/Block/ViewsBlock.php
@@ -23,10 +24,24 @@ class ViewsBlock extends ViewsBlockBase {
+    $args = [];
+    foreach ($this->getContexts() as $context_name => $context) {
+      /** @var \Drupal\views\Plugin\views\argument\ArgumentPluginBase $argument */
+      if (($argument = $this->view->display_handler->getHandler('argument', $context_name)) && ($value = $context->getContextValue())) {
+        if ($value instanceof EntityInterface) {
+          $value = $value->id();
+        }
+        $args[] = $value;
+      }
+      elseif ($argument->options['default_action'] == 'ignore') {
+        $args[] = $argument->options['exception']['value'];
+      }
+    }
+

What happens if you have a non context argument first ... then $args have no values at all for those, maybe setting a NULL for those would be nice

Makes sense. Added and updated the test.

I think we no longer needs additional tests ...

+++ b/core/modules/views/src/Plugin/Block/ViewsBlock.php
@@ -25,16 +25,24 @@ public function build() {
     $args = [];
-    foreach ($this->getContexts() as $context_name => $context) {
+    foreach ($this->view->display_handler->getHandlers('argument') as $argument_name => $argument) {
       /** @var \Drupal\views\Plugin\views\argument\ArgumentPluginBase $argument */
-      if (($argument = $this->view->display_handler->getHandler('argument', $context_name)) && ($value = $context->getContextValue())) {
-        if ($value instanceof EntityInterface) {
-          $value = $value->id();
+      if (!empty($this->context[$argument_name])) {
+        if (($value = $this->context[$argument_name]->getContextValue())) {
+          if ($value instanceof EntityInterface) {
+            $value = $value->id();
+          }
+          $args[] = $value;
+        }
+        elseif ($argument->options['default_action'] == 'ignore') {
+          $args[] = $argument->options['exception']['value'];
+        }
+        else {
+          $args[] = NULL;
         }
-        $args[] = $value;
       }
-      elseif ($argument->options['default_action'] == 'ignore') {
-        $args[] = $argument->options['exception']['value'];
+      else {
+        $args[] = NULL;
       }
     }

I'm wondering whether it would be worth to replace this crazy nesting with setting $args[$argument_name] = NULL and then override it just for the cases we know?

Nice, less crap to maintain

This should fix the failing test.

Thank you!

Some minor issues.

1. +++ b/core/modules/views/src/Plugin/Block/ViewsBlock.php
   @@ -23,10 +24,28 @@ class ViewsBlock extends ViewsBlockBase {
   +          } elseif ($argument->options['default_action'] == 'ignore') {
   

   CS is wrong here.

2. +++ b/core/modules/views/src/Plugin/Derivative/ViewsBlock.php
   @@ -106,9 +109,31 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   +                $this->derivatives[$delta]['context'][$argument_name] = new ContextDefinition('integer', $argument->adminLabel(), FALSE);
   ...
   +                  $this->derivatives[$delta]['context'][$argument_name] = new ContextDefinition('integer', $argument->adminLabel(), FALSE);
   

   Maybe combine these two.

FILE: ...sk/dev/drupal/core/modules/views/src/Plugin/Block/ViewsBlock.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 38 | ERROR | [x] Expected newline after closing brace
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 37ms; Memory: 5Mb


FILE: ...v/drupal/core/modules/views/src/Plugin/Derivative/ViewsBlock.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 9 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 41ms; Memory: 5.25Mb


FILE: ...les/views/src/Tests/Plugin/ContextualFiltersBlockContextTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 5 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 34ms; Memory: 4.75Mb

Other small things to fix.

Fix cs
#87.2 not sure it makes sense to combine them

Agree about #87.2. It doesn't seem that we can easily do that.

back to rtbc cos there's only cs changes

I have a field of type "List (text)" in content type and it has 2 values. I can make a contextual filter in views and make it get the value from url but is it also possible to set some variable from page manager with this patch? Just that the url might change during the development and then the value wouldn't be ok anymore.

I can confirm this works.
Tested it in combination with #2709235: Block fields don't make use of available contexts

So I need 83 and 89 to get this working? Can it be applied to 8.1.x ?

Blocks with required context will not be visible anymore on the core block layout UI, but they're already not working right now..

I think that this should be tested - there doesn't appear to be a test for this.

While I can confirm this patch works it is of limited use currently until NodeRouteContext is refactored into a EntityRouteContext . Also some words on the UI that the argument needs to be validated would be nice.

I cannot agree more with #23
@xjm @alexpott @cilefen @tim.plunkett agreed that this is not a bug, but a major task, of course.

Actually, it's relatively difficult to change what the deriver does without altering every plugin in the system generated by the deriver. You can't get in front of that because of where and how it happens in plugin discovery currently. Just as an FYI, yes we COULD do something along these lines in CTools, but it would be super heavy handed. Prefer to see it in core if possible.

Eclipse

What's left here? Tests are here and generic coverage for contexts in core

Rerolled the patch

The patch in #107 is wrong.
New rerolled patch file.

So let's be honest the status here :)

Having interface is good idea.
But that will require to update all contrib
OTOH that never work before so better to get into 8.3

1. +++ b/core/modules/views/src/Plugin/Block/ViewsBlock.php
   @@ -23,10 +24,31 @@ class ViewsBlock extends ViewsBlockBase {
   +    if ($this->view->display_handler->getHandlers('argument')) {
   

   IMHO we can skip the if. getHandlers will always return an array of values.

2. +++ b/core/modules/views/src/Plugin/Block/ViewsBlock.php
   @@ -23,10 +24,31 @@ class ViewsBlock extends ViewsBlockBase {
   +          }
   +          elseif ($argument_default = $argument->getPlugin('argument_default')) {
   +            if ($argument_default_value = $argument_default->getArgument()) {
   +              $args[$argument_name] = $argument_default->getArgument();
   +            }
   

   I'm wondering whether this has to be part of this patch. The default argument values are handled somewhere in views at the moment, so I don't see why we strictly need it. I see the point of this change though, and I believe its actually a good one, but you now, its all about lowering the surface density.

3. +++ b/core/modules/views/src/Plugin/views/argument_validator/NumericArgumentValidator.php
   @@ -12,10 +15,17 @@
   +   * {@inheritdoc}
   +   */
   +  public function getContextDefinition() {
   +    return new ContextDefinition('integer', $this->argument->adminLabel(), FALSE);
   +  }
   +
    }
   

   It is a bit confusing to be honest that we don't have similar kind of code for strings etc ...

+++ b/core/modules/views/src/Plugin/views/argument/ArgumentPluginBase.php
@@ -1326,6 +1326,17 @@ public function calculateDependencies() {
+   * Returns a context definition for this argument.
+   *
+   * @return \Drupal\Core\Plugin\Context\ContextDefinitionInterface|null
+   *   A context definition that represents the argument or NULL if that is
+   *   not possible.
+   */
+  public function getContextDefinition() {
+    return $this->getPlugin('argument_validator')->getContextDefinition();
+  }

I'm wondering whether there is some way of empty context definition, empty as in providing nothing. In this case we could skip returning NULL. I just assume there isn't :)

Ok, I've looked through this a bit and REALLY like what's sitting here. (For whatever that's worth).

A few quick validating questions:

1. +++ b/core/modules/views/src/Plugin/Block/ViewsBlock.php
   @@ -23,10 +24,28 @@ class ViewsBlock extends ViewsBlockBase {
   +      if (!empty($this->context[$argument_name])) {
   +        if ($value = $this->context[$argument_name]->getContextValue()) {
   +          if ($value instanceof EntityInterface) {
   +            $value = $value->id();
   +          }
   +          $args[$argument_name] = $value;
   +        }
   +      }
   

   I've attached a minor interdiff that I THINK is an improvement over this both in intent and readability. I wouldn't un-rtbc this over it, but it might be worth incorporating.

2. +++ b/core/modules/views/src/Plugin/views/argument/NumericArgument.php
   @@ -124,4 +125,17 @@ public function getSortName() {
   +    if ($context_definition = parent::getContextDefinition()) {
   +      return $context_definition;
   +    }
   +
   +    // If the parent does not provide a context definition through the
   +    // validation plugin, fall back to the integer type.
   +    return new ContextDefinition('integer', $this->adminLabel(), FALSE);
   

   Delegates to the Validator if there is one, and if not, returns an optional context definition for (in this case) numeric argument plugins.

   That means that if there is not a corresponding value for this, AND it's not a required argument, then your block code will use "all" by default for the argument value. Right? If so, ++

3. +++ b/core/modules/views/src/Plugin/views/argument_validator/Entity.php
   @@ -228,4 +229,11 @@ public function calculateDependencies() {
   +    return new ContextDefinition('entity:' . $this->definition['entity_type'], $this->argument->adminLabel(), FALSE);
   

   So the argument validator here returns an optional context definition. Is that what we actually want to do? Shouldn't this ONLY be the case if we allow "all" to be passed for this argument? Same question for all the other validators in this patch.

Hope this seems sensible.

Eclipse

Ok, sounds generally good, and certainly lightyears better than what we have today.

As for my interdiff, it checks if a particular context has a value object associated with it. If it does, of course you'd delegate to that value object. But since it's checking to see if the value object is there, if it is not there, it won't override the "all" you set just previous to this if statement.

TL:DR; The if statement only succeeds if there was a value object in the context object.

Eclipse

What are the odds of this making it into 8.3.0? I can't underscore how great this is from a contrib perspective enough. It should have been part of 8.0.0.

Eclipse

Back to RTBC

+++ b/core/modules/views/src/Plugin/Block/ViewsBlock.php
@@ -23,10 +24,28 @@ class ViewsBlock extends ViewsBlockBase {
+        if ($value = $this->context[$argument_name]->getContextValue()) {
+          if ($value instanceof EntityInterface) {
+            $value = $value->id();

I think it needs a comment about why there's EntityInterface instead of is_scalar() for example?

+++ b/core/modules/views/src/Plugin/views/argument/ArgumentPluginBase.php
@@ -1326,6 +1326,17 @@ public function calculateDependencies() {
+  public function getContextDefinition() {

+++ b/core/modules/views/src/Plugin/views/argument_validator/ArgumentValidatorPluginBase.php
@@ -104,6 +104,15 @@ public function validateArgument($arg) { return TRUE; }
+  public function getContextDefinition() { }

Shouldn't we be using some kind of interface?

@Berdir please add this comment, it explains the check for EI, it makes clear what context value expected by views

A couple of PHPCS fixes.

+++ b/core/modules/views/src/Plugin/views/argument/ArgumentPluginBase.php
@@ -105,7 +105,7 @@ public function exceptionTitle() {
-   * @return TRUE/FALSE
+   * @return TRUE or FALSE

Just use @return bool

Here we go.

+++ b/core/modules/views/src/Plugin/Derivative/ViewsBlock.php
@@ -106,9 +107,18 @@ public function getDerivativeDefinitions($base_plugin_definition) {
+          foreach ($display->getHandlers('argument') as $argument_name => $argument) {
+            /** @var \Drupal\views\Plugin\views\argument\ArgumentPluginBase $argument */
+            if ($context_definition = $argument->getContextDefinition()) {

+++ b/core/modules/views/src/Plugin/views/argument/StringArgument.php
--- a/core/modules/views/src/Plugin/views/argument_validator/ArgumentValidatorPluginBase.php
+++ b/core/modules/views/src/Plugin/views/argument_validator/ArgumentValidatorPluginBase.php

As far as I know there is no requirement to implement the base class. And since getContextDefinition() is not part of any interface - I think this has a chance of breaking. Shouldn't we have an optional interface that we can test for here?

How can we tackle multiple values arguments? AFAIK argument_validator doesn't play nice with multiple values.

1. +++ b/core/modules/views/src/Plugin/views/argument/ArgumentPluginBase.php
   @@ -1326,6 +1326,17 @@ public function calculateDependencies() {
   +  public function getContextDefinition() {
   
   +++ b/core/modules/views/src/Plugin/views/argument_validator/ArgumentValidatorPluginBase.php
   @@ -104,6 +104,15 @@ public function validateArgument($arg) { return TRUE; }
   +  public function getContextDefinition() { }
   

   These both should have @see to each other.

2. +++ b/core/modules/views/src/Plugin/views/argument/ArgumentPluginBase.php
   @@ -1326,6 +1326,17 @@ public function calculateDependencies() {
   +    return $this->getPlugin('argument_validator')->getContextDefinition();
   

   I think argument_default plugin should also set the context.