Add a generic entity argument validation plugin.

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

Just tracking some work.

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

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.

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

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.

change title.

+++ 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.

.

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?

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

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.

+++ 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?

XREF #2095235: The argument handler is broken for argument_validators for path like clone/taxonomy/term/1+2

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?

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 :)

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

Nice and important 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)

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.

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

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.

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

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

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.

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

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

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.

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

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!

Committed/pushed to 8.x, thanks!

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

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