An additional AccessPlugin that checks if the user has access to (selected) entity/ies for the route.

A solution suggested for #1857256: Convert the taxonomy listing and feed at /taxonomy/term/%term to Views. Presently taxonomy/term/%taxonomy_term checks entity->access view replacing it with a view requires doing the same. A generic solution to check access to entities in route.

Files: 
CommentFileSizeAuthor
#62 2029509-62.patch784 bytesdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 59,090 pass(es), 16 fail(s), and 2 exception(s).
[ View ]
#59 vdc-2029509.patch36.19 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,113 pass(es).
[ View ]
#54 vdc-2029509-54.patch36.23 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,894 pass(es).
[ View ]
#50 vdc-2029509-38-50.interdiff.txt1.38 KBekes
#50 vdc-2029509-50.patch36.65 KBekes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch vdc-2029509-50_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#47 vdc-2029509-46.patch36.58 KBekes
PASSED: [[SimpleTest]]: [MySQL] 58,900 pass(es).
[ View ]
#47 vdc-2029509-38-46.interdiff.txt1.31 KBekes
#46 vdc-2029509-38.patch36.53 KBekes
FAILED: [[SimpleTest]]: [MySQL] 58,825 pass(es), 12 fail(s), and 8 exception(s).
[ View ]
#46 vdc-2029509-35-38.interdiff.txt1.58 KBekes
#38 vdc-2029509-38.patch36.53 KBekes
PASSED: [[SimpleTest]]: [MySQL] 58,684 pass(es).
[ View ]
#38 vdc-2029509-35-38.interdiff.txt1.58 KBekes
#35 vdc-2029509-35.patch36.63 KBekes
PASSED: [[SimpleTest]]: [MySQL] 58,970 pass(es).
[ View ]
#35 vdc-2029509-27-35.interdiff.txt7.99 KBekes
#31 vdc-2029509-31.patch36.65 KBekes
FAILED: [[SimpleTest]]: [MySQL] 58,485 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
#31 vdc-2029509-27-31.interdiff.txt4.15 KBekes
#27 vdc-2029509-27.patch36.69 KBekes
PASSED: [[SimpleTest]]: [MySQL] 58,026 pass(es).
[ View ]
#27 vdc-2029509-23-27.interdiff.txt5.19 KBekes
#23 vdc-2029509-23.patch36.47 KBekes
PASSED: [[SimpleTest]]: [MySQL] 57,764 pass(es).
[ View ]
#23 vdc-2029509-20-23.interdiff.txt7.78 KBekes
#20 vdc-2029509-20.patch34.95 KBekes
PASSED: [[SimpleTest]]: [MySQL] 57,979 pass(es).
[ View ]
#20 vdc-2029509-15-20.interdiff.txt3.63 KBekes
#19 vdc-2029509-19.patch32.98 KBekes
PASSED: [[SimpleTest]]: [MySQL] 58,103 pass(es).
[ View ]
#15 vdc-2029509-15.patch32.95 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 56,894 pass(es).
[ View ]
#15 interdiff.txt7.04 KBdawehner
#13 vdc-2029509-13.patch33.13 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,033 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#12 vdc-2029509-12.patch26.36 KBekes
PASSED: [[SimpleTest]]: [MySQL] 56,400 pass(es).
[ View ]
#12 interdiff.txt5.79 KBekes
#8 vdc-2029509-8.patch26.58 KBekes
PASSED: [[SimpleTest]]: [MySQL] 57,017 pass(es).
[ View ]
#8 interdiff.txt1.46 KBekes
#5 vdc-2029509-5.patch26.35 KBekes
FAILED: [[SimpleTest]]: [MySQL] 56,657 pass(es), 0 fail(s), and 12 exception(s).
[ View ]
#5 interdiff.txt2.02 KBekes
#4 vdc-2029509-4.patch24.33 KBekes
FAILED: [[SimpleTest]]: [MySQL] 56,433 pass(es), 0 fail(s), and 13 exception(s).
[ View ]
#4 interdiff.txt16.2 KBekes
#3 vdc-2029509-3.patch11.78 KBekes
FAILED: [[SimpleTest]]: [MySQL] 56,460 pass(es), 1 fail(s), and 12 exception(s).
[ View ]
#2 vdc-2029509-2.patch5.16 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 56,404 pass(es).
[ View ]

Comments

dawehner’s picture

Additional to that we also need a argument validator plugin which checks access to an entity.

dawehner’s picture

StatusFileSize
new5.16 KB
PASSED: [[SimpleTest]]: [MySQL] 56,404 pass(es).
[ View ]

Just tracking some work.

ekes’s picture

StatusFileSize
new11.78 KB
FAILED: [[SimpleTest]]: [MySQL] 56,460 pass(es), 1 fail(s), and 12 exception(s).
[ View ]

A part-of-the-way point patch. Knowns:

Working: A generic entity argument handler with access to entity check; and where appropriate bundle filter option.
Needs work: Extending for user and taxonomy cases which allow more options. Needs tests.
Looks ugly: The 'entity:derivative' name given caused both the '#visible' form handling, and the ajax save to break. str_replace(':', '-' ... and vice versa is a pretty ugly work-around.

ekes’s picture

StatusFileSize
new16.2 KB
new24.33 KB
FAILED: [[SimpleTest]]: [MySQL] 56,433 pass(es), 0 fail(s), and 13 exception(s).
[ View ]

Adding: multiple ID handling; and porting taxonomy term ID and creating a taxonomy term name.
Remaining: user and node original argument handling. Node should be a swap out replacement. Presently user validator has some direct database accessing code; but looks like it could (should) be replaced with entity loading methods by the same pattern as Taxonomy Term.

ekes’s picture

Status:Active» Needs review
StatusFileSize
new2.02 KB
new26.35 KB
FAILED: [[SimpleTest]]: [MySQL] 56,657 pass(es), 0 fail(s), and 12 exception(s).
[ View ]

Changed so taxonomy term view uses new filter. So existing tests on view should pass.

Status:Needs review» Needs work

The last submitted patch, vdc-2029509-5.patch, failed testing.

ekes’s picture

The exceptions causing the fail above are entity type menu_item. Which has bundles; but seems to have no field definition (so no bundel_label etc.)

The ::baseFieldDefinitions() returns an empty array for ::getStorageController('menu_link'). Is it that the information is being called incorrectly; or as it seems that the information is not there?

[Follow-up asking question on issue making the bundles]

ekes’s picture

StatusFileSize
new1.46 KB
new26.58 KB
PASSED: [[SimpleTest]]: [MySQL] 57,017 pass(es).
[ View ]

Addition of a empty check, and default text, for any cases like this where there are bundles to select from but they don't have a general bundle type name.

ekes’s picture

Status:Needs work» Needs review
Berdir’s picture

menu links have not yet been converted to NG, that's why they have no field definitions.

dawehner’s picture

Status:Needs review» Needs work
Issue tags:+Needs tests, +VDC

Impressive work! I guess tests for the test entity would be helpful.

+++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsEntityArgumentValidator.phpundefined
@@ -0,0 +1,57 @@
+class ViewsEntityArgumentValidator implements DerivativeInterface {

The DerivativeBase which just landed this week really helps to simplify a lot of the code.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument/ArgumentPluginBase.phpundefined
@@ -294,7 +294,7 @@ public function buildOptionsForm(&$form, &$form_state) {
+      '#default_value' => str_replace(':', '-', $this->options['validate']['type']),

@@ -327,8 +327,9 @@ public function buildOptionsForm(&$form, &$form_state) {
+            $sanitized_id = str_replace(':', '-', $id);
+            $form['validate']['options'][$sanitized_id] = array(
+              '#prefix' => '<div id="edit-options-validate-options-' . $sanitized_id . '-wrapper">',

There should be a comment why this is needed.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument_validator/Entity.phpundefined
@@ -0,0 +1,206 @@
+    $entities = entity_load_multiple($entity_type, $ids);

What about using the entity manager to load the entities?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument_validator/Entity.phpundefined
@@ -0,0 +1,206 @@
+   * @param Entity $entity
...
+  protected function validateEntity(\Drupal\Core\Entity\Entity $entity) {

The best thing is probably to use the entity interface. Here the documentation should contain the full namespace, but the method just the "EntityInterface", which then is added as use statement.

ekes’s picture

Status:Needs work» Needs review
StatusFileSize
new5.79 KB
new26.36 KB
PASSED: [[SimpleTest]]: [MySQL] 56,400 pass(es).
[ View ]

Changes for suggestions above. Plus missed t().

Still to do: tests for the test entity.

dawehner’s picture

StatusFileSize
new33.13 KB
FAILED: [[SimpleTest]]: [MySQL] 57,033 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Here are some unit tests... I will continue with a review tomorrow.

Status:Needs review» Needs work

The last submitted patch, vdc-2029509-13.patch, failed testing.

dawehner’s picture

Priority:Normal» Major
Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new7.04 KB
new32.95 KB
PASSED: [[SimpleTest]]: [MySQL] 56,894 pass(es).
[ View ]

Opened a new follow up for the ':': #2035345: Reconsider whether to use ':' as separator for derivative plugins and fixed some minor stuff.

I consider this as major as it pretty much blocks all major conversions.

dawehner’s picture

Title:Views AccessPlugin entity access» Add a generic entity argument validation plugin.
Issue tags:+Needs tests

change title.

damiankloip’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/argument_validator/TermName.phpundefined
@@ -0,0 +1,93 @@
+ * Validate whether a term name is a valid term argument.

Validates

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/argument_validator/TermName.phpundefined
@@ -0,0 +1,93 @@
+ *   module = "taxonomy",

We can remove this; provider will get added during discovery.

+++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsEntityArgumentValidator.phpundefined
@@ -0,0 +1,40 @@
+    $entity_info = \Drupal::entityManager()->getDefinitions();
...
+        'help' => t('Validate @label', array('@label' => $entity_info['label'])),

We should use the ContainerDerivativeInterface and inject the entity manager and translation manager. See \Drupal\views\Plugin\Derivative\ViewsEntityRow or something.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument/ArgumentPluginBase.phpundefined
@@ -291,10 +291,12 @@ public function buildOptionsForm(&$form, &$form_state) {
+      '#default_value' => str_replace(':', '-', $this->options['validate']['type']),

Seems like this is liable to break pretty easily. I know we are talking about chnaging the separator, but do you think we should change this to a double '--' or something atleast?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument/ArgumentPluginBase.phpundefined
@@ -327,8 +329,10 @@ public function buildOptionsForm(&$form, &$form_state) {
+            $sanitized_id = str_replace(':', '-', $id);

@@ -385,10 +389,12 @@ public function validateOptionsForm(&$form, &$form_state) {
+    $validate_id = str_replace('-', ':', $sanitized_id);

@@ -418,11 +424,13 @@ public function submitOptionsForm(&$form, &$form_state) {
+    $form_state['values']['options']['validate']['type'] = $validate_id = str_replace('-', ':', $sanitized_id);

We are using these similar things a few times, worth encode/decode type methods?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument_validator/Entity.phpundefined
@@ -0,0 +1,214 @@
+      // @todo Should we support both ANY and ALL?

Yes, I think we should :)

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument_validator/Entity.phpundefined
@@ -0,0 +1,214 @@
+   * Validate an individual entity against class access settings.

Validates

+++ b/core/modules/views/tests/Drupal/views/Tests/Plugin/argument_validator/EntityTest.phpundefined
@@ -0,0 +1,201 @@
+    $this->assertFalse($this->argumentValidator->validateArgument(mt_rand(100, 1000)));

Seems overkill, can't we just try 1000, or even just 3 or something?

+++ b/core/modules/views/tests/Drupal/views/Tests/Plugin/argument_validator/EntityTest.phpundefined
@@ -0,0 +1,201 @@
+    $options = array();
+    $options['access'] = TRUE;
+    $options['bundles'] = array();
+    $options['operation'] = 'test_op';

These tests could make use a data provider? I guess the result of validation varies too much for each case?

+++ b/core/modules/views/tests/Drupal/views/Tests/Plugin/argument_validator/EntityTest.phpundefined
@@ -0,0 +1,201 @@
+    $this->assertFalse($this->argumentValidator->validateArgument('1,2'));
+    $this->assertFalse($this->argumentValidator->validateArgument('1+2'));

Would be good if a case for TRUE was tested too.

dawehner’s picture

Issue tags:+Needs reroll

.

ekes’s picture

Issue tags:-Needs reroll
StatusFileSize
new32.98 KB
PASSED: [[SimpleTest]]: [MySQL] 58,103 pass(es).
[ View ]
ekes’s picture

Status:Needs review» Needs work
StatusFileSize
new3.63 KB
new34.95 KB
PASSED: [[SimpleTest]]: [MySQL] 57,979 pass(es).
[ View ]

Starting on #17. Patch moves to ContainerDerivativeInterface. Posted just for check point.

Other issues raised make sense. Although I seem to recall some reason str_replace() wasn't put into it's own function; but probably was nothing. Shall get there next.

ekes’s picture

We are using these similar things a few times, worth encode/decode type methods?

It's also done in two classes several times in Drupal\views\Plugin\views\argument\ArgumentPluginBase, but also once in Drupal\views\Plugin\views\argument_validator\Entity. So then static method(s) on ArgumentPluginBase?

ekes’s picture

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument_validator/Entity.phpundefined
@@ -0,0 +1,214 @@
+      // @todo Should we support both ANY and ALL?

Yes, I think we should :)

Turns out we don't need to. If the argument fits all the entity validation is never called.

ekes’s picture

Status:Needs work» Needs review
StatusFileSize
new7.78 KB
new36.47 KB
PASSED: [[SimpleTest]]: [MySQL] 57,764 pass(es).
[ View ]

Changes from #17 with notes as above and:

These tests could make use a data provider? I guess the result of validation varies too much for each case?

The validation would be different each time, so it would be another test anyway if I understand the way that dataProvider works. As can be seen by what is done now within the test to check for when access is true for 1,2 and 1+2.

Seems overkill, can't we just try 1000, or even just 3 or something?

Here it's not working to hard, it's only one: just between 100 and 1000, which aren't valid entity IDs.

dawehner’s picture

Issue tags:-Needs tests

Removed the 'needs tests' tag.

@@ -296,7 +296,7 @@ public function buildOptionsForm(&$form, &$form_state) {

-      '#default_value' => str_replace(':', '-', $this->options['validate']['type']),
+      '#default_value' => self::sanitizeValidatorId($this->options['validate']['type']),

@@ -330,7 +330,7 @@ public function buildOptionsForm(&$form, &$form_state) {
-            $sanitized_id = str_replace(':', '-', $id);
+            $sanitized_id = self::sanitizeValidatorId($id);

What about using static:: instead so you can override it in a sub-class if needed

@@ -1125,6 +1125,23 @@ public static function processContainerRadios($element) {
+  public static function sanitizeValidatorId($id) {
...
+  public static function revertSanitizedValidatorId($id) {

Let's add @param's here.

@@ -1125,6 +1125,23 @@ public static function processContainerRadios($element) {
+    return str_replace(':', '---', $id);
...
+    return str_replace('---', ':', $id);

I try to understand why we need three '-' here, is this done to be 100% sure?

dawehner’s picture

btw. Thank you for working so hard on that issue!.

damiankloip’s picture

Issue tags:+Needs tests

Here it's not working to hard, it's only one: just between 100 and 1000, which aren't valid entity IDs.

Yep, I get its just giving one random value. It just seems we could just pick a random value instead? We don't gain from randomly generating, except maybe a random failure in the future ;)

I thought as much with the data provider, it doesn't make sense to use them here.

ekes’s picture

StatusFileSize
new5.19 KB
new36.69 KB
PASSED: [[SimpleTest]]: [MySQL] 58,026 pass(es).
[ View ]

Yep, I get its just giving one random value. It just seems we could just pick a random value instead?

Ah, now I understand. Yes sure, swapping to a known not set.

I try to understand why we need three '-' here, is this done to be 100% sure?

Yes, only figuring if - can actually be in the string already such that double makes sense, then either another different character or an extra-extra -.

damiankloip’s picture

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument/ArgumentPluginBase.php
@@ -1117,6 +1125,35 @@ public static function processContainerRadios($element) {
+  public static function sanitizeValidatorId($id) {
...
+  public static function revertSanitizedValidatorId($id) {

I will review properly on monday, but for now I think that sanitize/revert is not the best naming choice. I think we should do with encodeValidatorId and decodeValidatorId instead, as that is more inline with other core methods that do similar things. What do you think?

ekes’s picture

encode/decode sounds better; revertSanitized is kind of clunky anyway just the best I could think of at the time.

ekes’s picture

StatusFileSize
new4.15 KB
new36.65 KB
FAILED: [[SimpleTest]]: [MySQL] 58,485 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

Re-roll, plus changed method names suggested in #28.

Status:Needs review» Needs work

The last submitted patch, vdc-2029509-31.patch, failed testing.

clemens.tolboom’s picture

I get "Drupal\Component\Plugin\Exception\PluginException: The plugin (taxonomy_term) did not specify an instance class. in Drupal\Component\Plugin\Factory\DefaultFactory::getPluginClass() (line 62 of /core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.php). "

when calling a clone version of taxonomy_term view. Taken from steps to reproduce on #2095235: The argument handler is broken for argument_validators for path like clone/taxonomy/term/1+2

Should I reinstall drupal to make sure?

ekes’s picture

There are some API changes. I've updated. I'll post an updated patch for that.

However, for that, there is still one point where the taxonomy term page is broken. The preview works, the page itself has some path/route issues. Question as posed in IRC is

15:10 < ekes> What's the situation with views/menu-route? Just once I've fixed above, and enabled view: taxonomy_term_is_page() calls menu_get_object() however
$request->attributes->get(RouteObjectInterface::ROUTE_OBJECT)->getPath(); returns NULL in there. Something need adding?

I'll post the other changes in a moment though.

ekes’s picture

StatusFileSize
new7.99 KB
new36.63 KB
PASSED: [[SimpleTest]]: [MySQL] 58,970 pass(es).
[ View ]

Reroll as #31 plus API changes: uses Drupal\views\Annotation\ViewsArgumentValidator rather than core Plugin Annotation; changes plugin/derivative module to provider; and gets Field definitions from EntityController, not the StorageController.

Issue with taxonomy/term/% page view as per #34 exists.

ekes’s picture

Status:Needs work» Needs review

The issue in #34 is independent of this patch and exists in the core view at the moment [1]. So changing to needs review.

[1] Issue #2091399: [META] Remove menu_get_object() comment #2

dawehner’s picture

Issue tags:-Needs tests

I don't think we need tests anymore, just a couple of final nitpicks before setting it to RTBC. This is really great work.

  1. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/argument_validator/TermName.php
    @@ -0,0 +1,92 @@
    +use Drupal\Core\Annotation\Translation;
    ...
    +use Drupal\views\Annotation\ViewsArgumentValidator;
    ...
    + * @ViewsArgumentValidator(

    +++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument_validator/Entity.php
    @@ -0,0 +1,213 @@
    +use Drupal\views\Annotation\ViewsArgumentValidator;

    You can now skip to specify this annotation classes, see #2090353: Don't require to put the use statement into plugin classes

  2. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/argument_validator/TermName.php
    @@ -0,0 +1,92 @@
    +  /**
    +   * The taxonomy term storage controller.
    +   *
    +   * @var \Drupal\taxonomy\TermStorageControllerInterface
    +   */
    +

    Nice documentation but for you missed the variable itself :)

ekes’s picture

StatusFileSize
new1.58 KB
new36.53 KB
PASSED: [[SimpleTest]]: [MySQL] 58,684 pass(es).
[ View ]

Changes for #37.

jibran’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:+phpunit

Yay!! Thanks @ekes for killer tests RTBC as per #37.

dawehner’s picture

Nice and important work!!

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

I manually tested this by:

  1. Applying patch
  2. Installing standard profile
  3. admin/structure/views
  4. enabling the taxonomy view
  5. editing the taxonomy view
  6. clicking on Advanced to expose the contextual filters
  7. clicking on "Content: Has taxonomy term ID (with depth)"

This results in:

Uncaught [object Object] ajax.js?v=8.0-dev:551
Drupal.ajax.error ajax.js?v=8.0-dev:551
ajax.options.complete ajax.js?v=8.0-dev:246
fire jquery.js?v=2.0.3:2913
self.fireWith jquery.js?v=2.0.3:3025
done jquery.js?v=2.0.3:7415
(anonymous function)
ekes’s picture

The issue with menu_link not implementing (EntityNG) fields comment-#7 has returned with the API change #2024963: Move baseFieldDefinitions from storage to entity classes. But I'm not sure if it's in the scope of this issue/patch this time.

In Drupal\views\Plugin\views\argument_validator\Entity we call $this->entityManager->getFieldDefinitions($entity_type). Previously, the storage controller getFieldDefinitions method returned an empty array for the menu entities. Now the method Drupal\Core\Entity\EntityManager::getFieldDefinitions calls $class::baseFieldDefinitions($entity_type),. As the menu entities do not implement the method baseFieldDefinitions the function dies with a fatal error: PHP Fatal error:  Call to undefined method Drupal\menu_link\Entity\MenuLink::baseFieldDefinitions() in ..../core/lib/Drupal/Core/Entity/EntityManager.php on line 482. Just putting return if not method_exists call in EntityManager after the $class loaded solves this for this patch.

Bug against EntityManager::getFieldDefinitions(); or should it be called differently?

Berdir’s picture

It's not related to that issue, this is about #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface.

As the documentation of that function says, it's not valid to call it with a non content entity type. There could be nicer error handling, but the result would be the same.

ekes’s picture

As the documentation of that function says, it's not valid to call it with a non content entity type.

.

What's the best way for me to know when I've got the entity type name?

Berdir’s picture

You need to check if the $entity_definition['class'] implements ContentEntityInterface.

ekes’s picture

Status:Needs work» Needs review
StatusFileSize
new1.58 KB
new36.53 KB
FAILED: [[SimpleTest]]: [MySQL] 58,825 pass(es), 12 fail(s), and 8 exception(s).
[ View ]

Or possibly better, check if the entity definition says that the entity type is fieldable?

diff --git a/core/modules/views/lib/Drupal/views/Plugin/views/argument_validator/Entity.php b/core/modules/views/lib/Drupal/views/Plugin/views/argument_validator/Entity.php
index 49a8420..dca9fbb 100644
--- a/core/modules/views/lib/Drupal/views/Plugin/views/argument_validator/Entity.php
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument_validator/Entity.php
@@ -100,9 +100,11 @@ public function buildOptionsForm(&$form, &$form_state) {
       foreach ($bundles as $bundle_id => $bundle_info) {
         $bundle_options[$bundle_id] = $bundle_info['label'];
       }
-      $fields = $this->entityManager->getFieldDefinitions($entity_type);
       $bundles_title = empty($entity_definitions[$entity_type]['bundle_label']) ? t('Bundles') : $entity_definitions[$entity_type]['bundle_label'];
-      $bundle_name = empty($fields[$bundle_type]) || empty($fields[$bundle_type]['label']) ? t('bundles') : $fields[$bundle_type]['label'];
+      if ($entity_definitions[$entity_type]['fieldable']) {
+        $fields = $this->entityManager->getFieldDefinitions($entity_type);
+      }
+      $bundle_name = empty($fields) || empty($fields[$bundle_type]['label']) ? t('bundles') : $fields[$bundle_type]['label'];
       $form['bundles'] = array(
         '#title' => $bundles_title,
         '#default_value' => $this->options['bundles'],
ekes’s picture

StatusFileSize
new1.31 KB
new36.58 KB
PASSED: [[SimpleTest]]: [MySQL] 58,900 pass(es).
[ View ]

Wrong patch.

dawehner’s picture

  1. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument_validator/Entity.php
    @@ -100,9 +100,11 @@ public function buildOptionsForm(&$form, &$form_state) {
    +      if ($entity_definitions[$entity_type]['fieldable']) {
    +        $fields = $this->entityManager->getFieldDefinitions($entity_type);
    +      }

    I really like this solution.

  2. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument_validator/Entity.php
    @@ -100,9 +100,11 @@ public function buildOptionsForm(&$form, &$form_state) {
    +      $bundle_name = empty($fields) || empty($fields[$bundle_type]['label']) ? t('bundles') : $fields[$bundle_type]['label'];

    It would be great to have one additional sets of brackets here, just to make it clear what runs first.

Berdir’s picture

Status:Needs review» Needs work
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument_validator/Entity.php
@@ -100,9 +100,11 @@ public function buildOptionsForm(&$form, &$form_state) {
+      if ($entity_definitions[$entity_type]['fieldable']) {
+        $fields = $this->entityManager->getFieldDefinitions($entity_type);
+      }
+      $bundle_name = empty($fields) || empty($fields[$bundle_type]['label']) ? t('bundles') : $fields[$bundle_type]['label'];

No, that is not correct.

The fieldable key is misleading (that's why there's an issue to change it). File for example has field definitions (base fields) but fieldable = FALSE (because fieldable means configurable fields).

ekes’s picture

StatusFileSize
new36.65 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch vdc-2029509-50_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.38 KB

Ah, shame. It seems to make sense to be able to ascertain from EntityManager if the entity has field definitions/is a content type.

But anyway to move this on. Swapping to just:

-      $fields = $this->entityManager->getFieldDefinitions($entity_type);
-      $bundle_name = empty($fields[$bundle_type]) || empty($fields[$bundle_type]['label']) ? t('bundles') : $fields[$bundle_type]['label'];
+      if (in_array('Drupal\Core\Entity\ContentEntityInterface', class_implements($entity_definitions[$entity_type]['class']))) {
+        $fields = $this->entityManager->getFieldDefinitions($entity_type);
+      }
+      $bundle_name = (empty($fields) || empty($fields[$bundle_type]['label'])) ? t('bundles') : $fields[$bundle_type]['label'];
ekes’s picture

Status:Needs work» Needs review
dawehner’s picture

Issue tags:-phpunit, -VDC

#50: vdc-2029509-50.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+phpunit, +VDC

The last submitted patch, vdc-2029509-50.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new36.23 KB
PASSED: [[SimpleTest]]: [MySQL] 59,894 pass(es).
[ View ]

Rerolled the patch and marked #2110845: Allow overriding views to override paths with parameters critical due to the fact that taxonomy/term don't work at the moment.

Status:Needs review» Needs work
Issue tags:-phpunit, -VDC

The last submitted patch, vdc-2029509-54.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review

54: vdc-2029509-54.patch queued for re-testing.

dawehner’s picture

Issue summary:View changes
Status:Needs review» Reviewed & tested by the community

I manually tested the patch using simplytest.me

The curreny URL looks like the following, though this is not caused by that patch, certainly.
The patch just cares about the "1", not the next argument.
taxonomy/term/1/%7Btaxonomy_term_modifier%7D

catch’s picture

Status:Reviewed & tested by the community» Needs work
  1. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/argument_validator/TermName.php
    @@ -0,0 +1,91 @@
    +    if (!count($terms)) {

    if (!$terms) is plenty.

  2. +++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsEntityArgumentValidator.php
    @@ -0,0 +1,108 @@
    +  /**
    +   * The entity manager.
    +   *
    +   * @var \Drupal\Core\StringTranslation\TranslationManager
    +   */
    +  protected $translationManager;
    +

    Comment should say translation manager.

dawehner’s picture

Status:Needs work» Needs review
Issue tags:+VDC
StatusFileSize
new36.19 KB
PASSED: [[SimpleTest]]: [MySQL] 59,113 pass(es).
[ View ]

This patch uses a $this->t() method in the derivative class as well as extends DerivativeBase

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

This was already RTBC, and despite no interdiff the last patch addresses #58.

  1. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument/ArgumentPluginBase.php
    @@ -1100,6 +1108,35 @@ public static function preRenderMoveArgumentOptions($form) {
    +   * Reason and alternative: http://drupal.org/node/2035345
    ...
    +    return str_replace(':', '---', $id);

    Triple dash! Woo. Glad this has a d.o link

  2. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument_validator/Entity.php
    @@ -0,0 +1,214 @@
    +      if (in_array('Drupal\Core\Entity\ContentEntityInterface', class_implements($entity_definitions[$entity_type]['class']))) {

    Some day we'll have a method for this...

  3. +++ b/core/modules/views/tests/Drupal/views/Tests/Plugin/argument_validator/EntityTest.php
    @@ -0,0 +1,222 @@
    + * @group Drupal
    + * @group Views
    + *
    + * @see \Drupal\views\Plugin\views\argument_validator\Entity
    + */
    +class EntityTest extends UnitTestCase {

    Beautiful tests!

catch’s picture

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

damiankloip’s picture

Title:Add a generic entity argument validation plugin.» FAILING HEAD: Add a generic entity argument validation plugin.
Status:Fixed» Needs review
StatusFileSize
new784 bytes
FAILED: [[SimpleTest]]: [MySQL] 59,090 pass(es), 16 fail(s), and 2 exception(s).
[ View ]

plugin.manager.entity doesn't exist anymore as a service.

catch’s picture

Status:Needs review» Fixed

Whoops. I wonder who committed that patch removing the service then committed this one a day later without noticing...

amateescu’s picture

Title:FAILING HEAD: Add a generic entity argument validation plugin.» Add a generic entity argument validation plugin.

Status:Fixed» Needs work

The last submitted patch, 62: 2029509-62.patch, failed testing.

damiankloip’s picture

Status:Needs work» Fixed

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.