Upcast named arguments/named parameters in views

+++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
@@ -168,6 +169,7 @@ abstract class PathPluginBase extends DisplayPluginBase implements DisplayRouter
         $bits[$pos] = '{' . $parameter_name . '}';
+        $parameters[$parameter_name]['type'] = 'entity:' . $parameter_name;
       }

yeah that is not really enough, I mean, this will try to upcast things which aren't necessarily entities.

As written in IRC I'm wondering whether it would be a good idea to have some flag, hidden from the UI maybe even, to enable that kind of behaviour.

Modified the patch from #6 so that it would apply to 8.5.

This helped using %user as a named argument and a contextual filter with "User ID from route context".

I am not sure why tests are failing, seems unrelated. Might try to look.

I would ask though, why %user didn't work with this patch. In regular routes on yaml you only need to provide a type hint in case you don't use one of the known named parameters. And as such, I would have expected that %user worked regardless.

Taxonomy edit page is broken, gives fatal error. I guess that's why tests were failing.

Not setting an empty parameters when there's none helps with the fatal issue, likely because taxonomy/term/% is a view.

@hanoii The patch in #18 applies for me on 8.8.5, thanks.

Interesting issue / patch.

1. --- a/core/modules/views/src/Plugin/views/display/PathPluginBase.php
   +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
   

   For this to be RTBC, it'll need test coverage for changes to this class.

2. +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
   @@ -159,11 +160,18 @@ protected function getRoute($view_id, $display_id) {
   +        if (isset($arguments[$argument_position]['specify_validation']) && $arguments[$argument_position]['specify_validation'] == TRUE) {
   

   !empty() instead?

3. +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
   @@ -159,11 +160,18 @@ protected function getRoute($view_id, $display_id) {
   +          if (count($validation_plugin) == 2 && $validation_plugin[0] == 'entity') {
   

   s/==/===/

Thanks,
-Derek

Experiences and risks for this issue are similar to #2730631: Upcast node and node_revision parameters of node revision routes

@dww i am working on it.

I am unable to apply the patch #18 on 9.1.x need to re-roll for 9.1.x

@dww I have created a new patch for this i was unable to apply this patch on 9.1 and 9.0
For #23
#23.1: The test case already exist and run successfully
PHPUnit 8.5.4 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\views\Unit\Plugin\display\PathPluginBaseTest
.............. 14 / 14 (100%)

Time: 309 ms, Memory: 12.00 MB

OK (14 tests, 94 assertions)
#23.2: Changes has been done
#23.3: Changes has been done

Please review

Thanks for the updates, @pavnish!

1. Please provide an interdiff with a new patch to make it easier to see what you changed.

2. Thanks for finding Drupal\Tests\views\Unit\Plugin\display\PathPluginBaseTest -- however, that class has no coverage of this bug report, or we wouldn't have the bug in the first place. ;) The point is, we need to expand that test to cover the behavioral changes introduced with this patch. That's why I added the 'Needs tests' tag -- we need to test *this* bug, not just have any test coverage at all of the class where the bug fix is happening.

3. The best practice is to make such changes to the test, and upload a patch that only includes the changes to the test, without the underlying bug fix. You can call this patch something like "2528166-{comment_number}.test-only.patch". Upload that first, then upload the full patch with both the test changes and the fix. The test-only should fail, and it should fail in a way that makes sense to reveal the bug. Then the full patch should pass all tests (old and new/changed). That way, the core committers can review the test-only to see what it takes to trigger the bug, verify that the test is legitimate, and then see that the new test coverage passes once the fix is applied. This helps verify the bug fix is correct, and ensures we won't re-break the bug with future changes to core.

Thanks!
-Derek

p.s. For extra credit (more quickly see the failed test results, save pointless bot cycles, save the DA $$), your test-only patch can include changes to core/drupalci.yml to tell the bot to only run the new/changed test(s). For a recent example, see https://www.drupal.org/files/issues/2020-05-17/3136668-27.orphaned-and-p... from #3136668-27: Invalid system.schema key_value entry causes fatal on updating to 8.8.5

Enjoy,
-Derek

p.p.s. Upon closer read, that's maybe not the most clear example, since it included *some* of the proposed fix. ;) This is a cleaner example:

#2830326-32: Broken link to 'Put your site into maintenance mode' on update.php results in WSOD
https://www.drupal.org/files/issues/2020-05-15/2830326-32.8_8_x.test-onl...

@dww
RE #28.1 interdiff added
RE ##28.2 Need help.

Need work on #28.2 #28.3

@pavnish: Thanks. The point of #23.2 is that empty() checks for existence and truth in 1 call. So you can replace all of:

if (isset($a['b']) and $a['b'] == TRUE)) {

with:

if (!empty($a['b'])) {

Re: #28.2: Yeah, to write good test coverage for this, you need to understand what "upcast named arguments" means, and what functionality it unlocks. ;) I'll freely admit it's not clear from reading the issue summary. It's basically the plumbing in core that gives you a loaded object in a route Controller callback based on the value of a specific part of a URL, if you specify the route with something like {node} or {user} or whatever. So, we need a test that sets up a view to use an argument that expects some kind of entity, and then make sure that the underlying functionality gets the right kind of entity object. Or something. ;) Honestly, I'm not totally sure myself what "bug" this is fixing, what functionality doesn't work, etc. I know how to make use of a named argument in a Controller callback, but I'm not sure how/why Views itself would care about that. I'd have to dig a lot deeper, and I don't have time for that anytime soon. Hopefully someone else who cares about this can better articulate what functionality is broken as a result of this omission. A summary update would help a lot, so tagging for that, too.

Thanks/sorry,
-Derek

done changes suggested in #35
replace all of:
if (isset($a['b']) and $a['b'] == TRUE)) {
with:
if (!empty($a['b'])) {

Thanks. We still need tests and an issue summary update.

+++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
@@ -156,11 +157,18 @@ protected function getRoute($view_id, $display_id) {
+            $parameters[$parameter_name]['type'] = 'entity:' . $validation_plugin[1];

@@ -201,6 +209,11 @@ protected function getRoute($view_id, $display_id) {
+      $route->setOption('parameters', $parameters);

Unfortunately, this not a single instance where we have to fix this. #2847224: Add views argument_default for getting Entity ID from URL and #2847233: Allow entity field value as argument_default also needs this functionality. It is not just that Views plugins need this, in contrib moderation_sidebar #3047936: Generic support for any entity type and metatags see metatag_get_route_entity need this as well.

I think we need an issue to add a service for the entity system which will reliably provide the parameter from the route with the correct version and access. IOW, convert metatag_get_route_entity to a core service which is the best solution for this problem IMO.

It's not enough to inspect the argument validator plugin ID. There are cases when the plugin ID has a pattern different than entity:{entity_type}, see \Drupal\user\Plugin\views\argument_validator\UserName. In that case we still can upcast the route parameter to an entity.

@jibran, I'm not sure I got the relevance of #45 for this issue. Could you, please, elaborate?

Forgot to add the new param in create() static method.

Fixing broken existing tests. Still needs tests for the issue scope.

Now, I think I like more the idea from #3034919-5: Explore making views routes add parameter options for named entity-type arguments. Avoids using the argument validator plugin ID but also avoids injecting the argument validator plugin manager. However, the test is coupled with the comment & content moderation modules.

Applying the solution from #3034919-5: Explore making views routes add parameter options for named entity-type arguments. Adding credits. Still needs test.

Adding tests. There's also a "test only" patch that acts here also as interdiff. Fixing IS. And i think this is a "Feature request".

Closed #3034919: Explore making views routes add parameter options for named entity-type arguments as duplicate of this.

Fix tests

9.3.x is the development branch and this is a feature request.

Yep, needs a change note.

Let's see a test only run.

Fix coding standards for "test only" patch. The main work has been moved to MR.

Added draft CR https://www.drupal.org/node/3244549

Fixing IS.

I ran into this issue when trying to figure out why adding a _entity_access to a views route failed, it was because parameters were not upcast.

Although the test and fix look good to me I'm hesitant to put this to RTBC. Primarily I'm worried that this may not be a backwards compatible change. Contrib modules may currently have named route parameters that expect to receive the entity ID rather than an upcast entity. Specifically anyone who wrote a views access handler and looks at the route parameter and then naïvely loads an entity based on it would find their code broken now that they receive an entity instead.

The group module currently manually alters the route to enable upcasting in the GroupPermission access handler. In Open Social we also have access checks that deal with views routes although I believe most of these are written to handle both the entity ID and loaded entity case. An example is access to a user event view.

Fix for version 9.3.x

Fix test pipeline

I just ran into this myself.

https://drupal.stackexchange.com/questions/311028/view-with-a-path-of-ap...

It makes it hard to build Views with routes like this without upcasting to take advantage of other chained arguments. All that argument will receive is an ID.

Opened a new MR !2979 against 10.1.x.

@Kingdutch, I understand your point but we already did this for node revisions in #2730631: Upcast node and node_revision parameters of node revision routes. How to move on wit this?

I think this is mostly up to core maintainers, going to move this to RTBC to get their visibility.
I've slightly changed my point of view since my last message in that I think currently a lot of code dealing with routes already needs to handle both cases to be resilient (I found this after trying to simplify some code after my last message and all of my assertions failing).

For example /user/:id is currently upcast but /user/%user/custom-view is not. So a block that wants to work on both pages will need to take both instances into account.

With that in mind I think a move towards standardisation is much more likely to allow people to simplify their code base in the future than it is to break code that currently works sometimes, but is silently broken other times. So I for one would be very happy if we can get this in.

Tests for this issue are currently fataling:
https://www.drupal.org/pift-ci-job/2524962

Meanwhile, re:

Contrib modules may currently have named route parameters that expect to receive the entity ID rather than an upcast entity. Specifically anyone who wrote a views access handler and looks at the route parameter and then naïvely loads an entity based on it would find their code broken now that they receive an entity instead.

Could we add a BC layer with a __toString() method or the equivalent so that the caller would get the deprecation?

Could we add a test for the current behavior with the entity ID as the parameter so that we can analyze the BC break?

I think it's a good point to explore, and we might need to handle it with a deprecation process for D11 otherwise.

Thank you @Kingdutch for your feedback on this point; I think it's important to understand the potential disruption of this issue.

Any news on this? Thanks

Fixed the tests but nothing has changed in substance, only adapted to new PHPCS, PHPStan, Drupal core changes. This was once RTBCed by @kingdutch, in #84

Thanks for reviving this one!

Could we add a BC layer with a __toString() method or the equivalent so that the caller would get the deprecation?

Could we add a test for the current behavior with the entity ID as the parameter so that we can analyze the BC break?

Seems this feedback from @xjm is still needed.

The latest changes from the main branch have been merged.