Updated: Comment #0
Follow-up from #1921544-82: Create a Current Path Condition.
@catch said

Users won't have access to the route names - but the whole point of having a router + generator is being able to move routes around without path-specific code breaking. I think this could use more discussion - i.e. should we open a follow-up to add a route condition?

Problem/Motivation

We should be moving towards matching the route and params instead of the path. The problem is that users maybe will not have access to the route names.

Proposed resolution

Create route and params condition and replace current path condition.

Remaining tasks

Are we only going to create route and params condition? or are we going to replace path condition with route and params condition? This needs more discussion.

User interface changes

Route and params condition is going to replace current path condition.

API changes

Route and params condition is going to replace current path condition.

#1921544: Create a Current Path Condition
#1743686: Condition Plugin System

CommentFileSizeAuthor
#20 2071553-route-condition-20.patch3.56 KBjian he
#20 interdiff.txt3.09 KBjian he
#18 2071553-route-condition-18.patch3.09 KBjian he
#18 interdiff.txt1.01 KBjian he
#16 2071553-route-condition-16.patch3.09 KBjian he
#5 interdiff.txt541 byteskim.pepper
#5 2071553-route-condition-5.patch4.47 KBkim.pepper
#4 interdiff.txt3.55 KBkim.pepper
#4 2071553-route-condition-4.patch4.48 KBkim.pepper
#3 2071553-route-condition-3.patch4.03 KBkim.pepper
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Issue summary: View changes
Issue tags: -conditions, -Stalking Crell +condition plugins
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
Related issues: +#1921544: Create a Current Path Condition
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
Status: Active » Needs review
FileSize
2071553-route-condition-3.patch4.03 KB

Here's a very simple implementation that only works with route names, and not route parameters.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
FileSize
2071553-route-condition-4.patch4.48 KB
interdiff.txt3.55 KB

Discussed with @tim.plunkett in IRC and agreed that we should only put one route.

I've also added a form for said route.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
FileSize
2071553-route-condition-5.patch4.47 KB
interdiff.txt541 bytes

Oops. Copy and paste error.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Can't we do something similar as menu links do: store route names/parameters internally but use paths for the UI?

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented

Can't we do something similar as menu links do: store route names/parameters internally but use paths for the UI?

I guess so? The menu link code looks pretty complicated, and I assume we'd be duplicating some of this.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

Do we really want to do that?
If someone alters the path used by a given route name, should the condition follow that? Or begin to fail because the path no longer matches?
Also that would overlap with the path condition...

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Well, we do follow changes to the path for example on menu links already.
I could though see usecases for both to be honest. IN d7 people often used path aliases for this which lead to a couple of problems when removing them or changing them.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
Related issues: +#2321745: Add #type => 'path' that accepts path but optionally stores URL object or route name and parameters

jibran queued 5: 2071553-route-condition-5.patch for re-testing.

jibran queued 5: 2071553-route-condition-5.patch for re-testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs review » Needs work 
+++ b/core/modules/system/lib/Drupal/system/Plugin/Condition/RouteCondition.php
@@ -0,0 +1,73 @@
+    $route_name = $request->attributes->get('_route');

We should use the route match now.

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented
Project: Drupal core » Ctools
Version: 8.0.x-dev »
Component: other » Code

This seems like it should probably become CTools domain for the forseeable future.

Eclipse

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented
Project: Ctools » Chaos Tool Suite (ctools)
Version: » 8.x-3.x-dev

wrong ctools...

jian he’s picture

jian he CreditAttribution: jian he commented
Status: Needs work » Needs review
FileSize
2071553-route-condition-16.patch3.09 KB

This one will working :)

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented 
+++ b/src/Plugin/Condition/RouteCondition.php
@@ -0,0 +1,105 @@
+    $form['pages'] = array(

$form['route']

jian he’s picture

jian he CreditAttribution: jian he commented
FileSize
interdiff.txt1.01 KB
2071553-route-condition-18.patch3.09 KB

Fixed configuration form.

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 commented
Status: Needs review » Needs work 
+++ b/src/Plugin/Condition/RouteCondition.php
@@ -0,0 +1,105 @@
+   * @param \Symfony\Component\HttpFoundation\RequestStack $request_stack
+   *   The request stack.
+   */

I guess it is request matcher

jian he’s picture

jian he CreditAttribution: jian he commented
Status: Needs work » Needs review
FileSize
interdiff.txt3.09 KB
2071553-route-condition-20.patch3.56 KB

1. Fixed the document mentioned at #19
2. Added the schema definition
3. Rename the route configuration param into route_name

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented

Anyone opposed to an autocomplete for the route name? My concern would be around security.

Eclipse

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented
Status: Needs review » Needs work

Also, looking at this again, I'm not sure a route by name condition is all that useful. Maybe it is, but it needs to be multivalue because multiple routes can exist on the same path. Also you might be interested in more than just parameters. I could see a use case for request method just off the top of my head. Whatever the case this will definitely need some serious tests and I'd like to hear more about the benefits/detriments of path vs route in our new world of multiple routes per path.

Eclipse

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented
Status: Needs work » Closed (won't fix)

On further thought I've decided to close this issue. If someone feels strongly that this should be a thing, re-open it and let's discuss.

Eclipse

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Issue tags: -Blocks-Layouts
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented

I think we are going in a wrong direction here. We should change the scope of the issue. \Drupal\system\Plugin\Condition\RequestPath is still matching the path. The UI should remain the same but we should stores routes instead of paths and we should match route name instead of a path.