View widgets access check

Would it be too disruptive to add a generic method on the interface, allowing widgets to define their visibility in any context? This way we solve not only this need, but any need where someone needs to check if a given widget is available or not.

This is a WIP (but functional) patch. If the approach looks good, I can finish it, adding tests, DI, etc.

Also, how can we retrieve the entity browser from inside the selector plugin?

    $browser_id = substr(substr($form_state->getBuildInfo()['form_id'], 15), 0, -5);

looks awful, but I couldn't figure out how to do it otherwise... :)

Let's validate the approach, the tests will probably need to be updated if that's the case

Looks good. Few comments:

1. +++ b/src/Plugin/EntityBrowser/WidgetSelector/Tabs.php
   @@ -21,20 +21,26 @@ class Tabs extends WidgetSelectorBase {
   +    // @TODO Use DI to get $this->entityTypeManager here.
   +    /** @var \Drupal\entity_browser\EntityBrowserInterface $browser */
   +    $browser = \Drupal::entityTypeManager()->getStorage('entity_browser')->load($browser_id);
   

   Let's add this.

2. +++ b/src/WidgetInterface.php
   @@ -109,4 +109,13 @@ interface WidgetInterface extends PluginInspectionInterface, ConfigurablePluginI
   +  /**
   +   * Defines if the widget is visible / accesible in a given context.
   +   *
   +   * @return bool
   +   *   TRUE if the widget is to be considered accesible (visible) by other
   +   *   plugins (such as the selector plugin), FALSE otherwise.
   +   */
   +  public function isVisible();
   +
   

   I'd rename this to access() to be consistent with the rest of Drupal. Also return AccessResult or bool (based on the argument) - another step to be more consistent with the rest of Drupal.

Should \Drupal\entity_browser\Plugin\EntityBrowser\WidgetSelector\DropDown also have this check?

The patch from #9 works fine for me. Thank You, Dan!

1. +++ b/src/Plugin/EntityBrowser/WidgetSelector/DropDown.php
   @@ -14,7 +17,35 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   
   +++ b/src/Plugin/EntityBrowser/WidgetSelector/Tabs.php
   @@ -14,27 +17,60 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   

   Needs docblock with @param entries etc.

2. +++ b/src/Plugin/EntityBrowser/WidgetSelector/DropDown.php
   @@ -14,7 +17,35 @@
   +  public function __construct($configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager) {
   +    parent::__construct($configuration, $plugin_id, $plugin_definition);
   +
   +    $this->entityTypeManager = $entity_type_manager;
   +  }
   
   +++ b/src/Plugin/EntityBrowser/WidgetSelector/Tabs.php
   @@ -14,27 +17,60 @@
   +  public function __construct($configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager) {
   +    parent::__construct($configuration, $plugin_id, $plugin_definition);
   +
   +    $this->entityTypeManager = $entity_type_manager;
   +  }
   

   This breaks BC since it will break any plugins that extend this classes. We'd need to do this in non BC breaking way.

   We could introduce a new interface that would allow plugins to opt-in to this feature.

3. +++ b/src/Plugin/EntityBrowser/WidgetSelector/DropDown.php
   @@ -24,9 +55,17 @@ public function getForm(array &$form = array(), FormStateInterface &$form_state
   +    $browser_id = substr(substr($form_state->getBuildInfo()['form_id'], 15), 0, -5);
   +    /** @var \Drupal\entity_browser\EntityBrowserInterface $browser */
   +    $browser = $this->entityTypeManager->getStorage('entity_browser')->load($browser_id);
   
   +++ b/src/Plugin/EntityBrowser/WidgetSelector/Tabs.php
   @@ -14,27 +17,60 @@
   +    $browser_id = substr(substr($form_state->getBuildInfo()['form_id'], 15), 0, -5);
   +    /** @var \Drupal\entity_browser\EntityBrowserInterface $browser */
   +    $browser = $this->entityTypeManager->getStorage('entity_browser')->load($browser_id);
   

   The entity browser should be available in the form state object.

4. +++ b/src/Plugin/EntityBrowser/WidgetSelector/DropDown.php
   @@ -24,9 +55,17 @@ public function getForm(array &$form = array(), FormStateInterface &$form_state
   +    $widget_ids = array_filter($this->widget_ids, function ($id) use ($browser) {
   +      return $browser->getWidget($id)->access();
   +    }, ARRAY_FILTER_USE_KEY);
   

   ARRAY_FILTER_USE_KEY is PHP 5.6 only. As much as I would like to use it Drupal still supports PHP >= 5.5.9.

5. +++ b/src/Plugin/EntityBrowser/WidgetSelector/Tabs.php
   @@ -14,27 +17,60 @@
   +      if (!empty($browser) && $browser->getWidget($id)->access()) {
   +        $name = 'tab_selector_' . $id;
   +        $element[$name] = array(
   +          '#type' => 'button',
   +          '#attributes' => ['class' => ['tab']],
   +          '#value' => $label,
   +          '#disabled' => $id == $this->getDefaultWidget(),
   +          '#executes_submit_callback' => TRUE,
   +          '#limit_validation_errors' => array(array($id)),
   +          // #limit_validation_errors only takes effect if #submit is present.
   +          '#submit' => array(),
   +          '#name' => $name,
   +          '#widget_id' => $id,
   +        );
   

   It would be nicer to get AccessResult result object and add it into the render array using #access. This will, among other things, ensure that any cache metadata is preserved.

6. We need to add test coverage for this. We didn't manage to get this in before feature freeze. We will start accepting new features after 8.x-1.0 assuming they come with automated tests.

Updated patch which tries to fix the remarks mentioned above.
This patch provides an easier way to get to the entity browser object, based on the remarks by slashrsm.
This fixes the BC problem.

Test coverage still needs to added.

Reroll, review, minor change, tests.

And interdiff.

1. +++ b/src/Plugin/EntityBrowser/Widget/View.php
   @@ -279,4 +280,21 @@ class View extends WidgetBase implements ContainerFactoryPluginInterface {
   +
   +    // Check if the current user has access to this view.
   +    $result = AccessResult::allowedIf($view->access($this->configuration['view_display']));
   +    return $return_as_object ? $result : $result->isAllowed();
   +  }
   

   this could be an issue with caching, because views doesn't seem to have a way to return access cacheablity metadata. permissions/roles are covered by the default user.permissions cache context, but in case of a more complicated access plugin, this might be a problem.

   But not much we can do about it here, except marking it as completely uncacheable. Which in most cases isn't really an issue as it won't be cached anyway.

2. +++ b/src/Plugin/EntityBrowser/WidgetSelector/Tabs.php
   @@ -34,6 +36,7 @@ class Tabs extends WidgetSelectorBase {
            '#widget_id' => $id,
   +        '#access' => $browser->getWidget($id)->access(),
   

   this also loses cacheablity metadata if a plugin actually does add it.

   The whole $return_as_object stuff was mostly added for BC.

   I'd actually vote to just always return an access result object like for example route access checks do. Then implementation are more obviously reminded to apply cacheablity metadata prperly.

3. +++ b/src/WidgetInterface.php
   @@ -118,4 +118,17 @@ interface WidgetInterface extends PluginInspectionInterface, ConfigurablePluginI
   +  /**
   +   * Defines if the widget is visible / accessible in a given context.
   +   *
   +   * @return bool|\Drupal\Core\Access\AccessResultInterface
   +   *   The access result. Returns a boolean if $return_as_object is FALSE (this
   +   *   is the default) and otherwise an AccessResultInterface object.
   +   *   When a boolean is returned, the result of AccessInterface::isAllowed() is
   +   *   returned, i.e. TRUE means access is explicitly allowed, thus the widget
   +   *   is to be considered accessible by other plugins. FALSE means
   +   *   access is either explicitly forbidden or "no opinion".
   +   */
   +  public function access($return_as_object = FALSE);
   +
   

   Alternatively, to make the BC impact minimal, we could optionally support the existing \Drupal\Core\Access\AccessibleInterface, although that won't allow us to do the always-return-access-result-object thing.

4. +++ b/tests/modules/entity_browser_test/src/Plugin/EntityBrowser/Widget/DummyWidget.php
   @@ -55,4 +56,17 @@ class DummyWidget extends WidgetBase {
   +  public function access($return_as_object = FALSE) {
   +    if (\Drupal::state()->get('eb_test_dummy_widget_access', TRUE)) {
   +      $access = AccessResult::allowed();
   +    }
   +    else {
   +      $access = AccessResult::forbidden();
   +    }
   +    return $return_as_object ? $access : $access->isAllowed();
   +  }
   

   this could then add some weird cache context that we can look for in the response. except not sure if phantomjs even allows to look at headers.

I am totally fine with always returning an object. Since this is just being added we do not need to introduce any BC layer. Changed that, added the cache metadata check in the test. Also fixed another problem that I found: if the first widget was not accessible it was still displayed.

Meh...

Implemented access() for the EntityForm widget too.

Reroll.

We've been using this for months now and I reviewed it before, lets commit this :)

Committed. Big thanks!