Updated: Comment #0
Suggested commit message
Issue #1918768 by larowlan, tim.plunkett, chx: Refactor tour module to use routes instead of paths.

Problem/Motivation

Tour module relies on paths to decide when to display each tour.
In the new world order we should be using route names.

Proposed resolution

Switch out paths for route names

Remaining tasks

Reviews

User interface changes

None

API changes

Change to tour yml format

Original report by @username

Over in #1809352: Write tour.module and add it to core we need config entity queries where we can do path matching, this patch provides pattern matching on single value properties.

Would also nice to be able to have pattern matching on array properties too (tour config entity has a path array property on the entity and contains wildcards - ala the 'pages' textarea on the block placement configuration form).

Here's an example patch cobbled together after conversations with @chx.

Commit credit for @chx too as he rightly pointed out that I should be using preq_quote() and some inconsistencies in my tests.

Comments

larowlan’s picture

Status:Active» Needs review
StatusFileSize
new7.23 KB
PASSED: [[SimpleTest]]: [MySQL] 50,590 pass(es).
[ View ]

Sample implementation

chx’s picture

Status:Needs review» Needs work
+++ b/core/lib/Drupal/Core/Config/Entity/Query/Condition.phpundefined
@@ -176,6 +176,9 @@ protected function match(array $condition, $value) {
+          $regex = '@^' . str_replace('\\*', '(.*)', preg_quote($value, '@')) . '$@i';
+          return preg_match($regex, $condition['value']);

You dont need backslashes. And I am confused by the goals here, isn't it $condition['value'] that needs to be converted into a regex and preg_matched against value? But then again, your 'label' is 'foo_bar_*' -- this is really weird. I would've expected the label to be foo_bar_baz and the condition('label', 'foo_bar_*', 'MATCH').

damiankloip’s picture

Yes, I'm confused too. I'm with chx; I was expecting to match 'foo_bar_baz' with condition('label', 'foo_bar_*'). You need to use wildcards in actual properties and not for querying them?

EDIT: I see now, you want to essentially replace the drupal_match_path() usage. These tests should test against the array values too? Still seems weird :)

larowlan’s picture

Yeah, we did have wildcards in our properties - ala paths in block config eg node/add/*

larowlan’s picture

So Tour entity has a paths property which is an array.
I was hoping we could use the Conditions API for this but I've not seen any movement on those issues.
#1921540: Create a User Role Condition
#1921546: Create a PHP Filter Condition
#1921550: Create a Language check Condition
and especially
#1921544: Create a Current Path Condition

The paths property contains wildcards.
Eg

paths
  -node/add/*
  -node/*/edit

We want to match entities where a known condition (eg current_path()) matches any of that entity's property.

So in this case we're comparing current_path() to Drupal\tour\Plugin\Core\Entity\Tour->paths

Hope that helps

dawehner’s picture

I am wondering whether we could convert the paths to actual used route names, which then potentially would be way faster to load.

larowlan’s picture

Yes I think that would work but it would entail

  • An api change, I think changing the tour yml schema now is an api change
  • All pages with tours would need to be converted to the new routing system before a tour could be used
  • Some extra work arounds in Tour UI (contrib) to create a widget for selecting a route, as expecting users to enter a list of route names, one per line is a big ask, instead we could provide an autocomplete widget that looked up route names based on patterns
  • Longer term we'd hoped to use conditions api for the path so we could also include things like role, content type, language etc. This would be a break from the 'path' condition which still uses paths but there is no reason that we couldn't write a 'route name' condition plugin.
clemens.tolboom’s picture

I don't think ask Tour writers to enter route names as that knowledge is not available for a writer. It's no big deal to write a path to route lookup I guess.

But one blocker against this issue is writing a tour for very particular paths like

  1. node/1 i.e. explaining the login procedure containing some screenshots
  2. product/13 i.e. explaining parts of the product
  3. admin/structure/views/view/complicated_view having a tour documenting particular difficult parts of the view)

Anyway I filed Tour UI #2099305: Check for router path and prepare for change notice to come.

larowlan’s picture

Title:Add match/match any support for ConfigEntityQuery» Refactor tour module to use routes instead of paths
Component:configuration entity system» tour.module
Issue summary:View changes
Issue tags:+Tour, +API change
larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new4.67 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch tour-route-name..patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Like so

Status:Needs review» Needs work

The last submitted patch, 10: tour-route-name..patch, failed testing.

larowlan’s picture

10: tour-route-name..patch queued for re-testing.

andypost’s picture

I'm agree with #8 - that "tour-writers" have no clue about route names so tour should work with both, probably tour_ui module could try to convert path to route name

damiankloip’s picture

+++ b/core/modules/tour/tour.module
@@ -111,25 +112,17 @@ function tour_preprocess_page(&$variables) {
+    $tours = entity_load_multiple('tour', array_keys($results));
...
+      $variables['page']['help']['tour'] = entity_view_multiple($tours, 'full');

We should use the controller directly here instead?

alexpott’s picture

@damiankloip - this is in procedural code, therefore looks fine to me

kim.pepper’s picture

10: tour-route-name..patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 10: tour-route-name..patch, failed testing.

tim.plunkett’s picture

+++ b/core/modules/tour/tour.module
@@ -111,25 +112,17 @@ function tour_preprocess_page(&$variables) {
+  $query = \Drupal::service('entity.query')->get('tour');

There is \Drupal::entityQuery(), a bit shorter. But why not use entity_load_multiple_by_properties() here?

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new4.64 KB
PASSED: [[SimpleTest]]: [MySQL] 59,226 pass(es).
[ View ]

entity_load_multiple_by_properties() performs an equality test, what we want is an in_array() test
re-roll

tim.plunkett’s picture

StatusFileSize
new1.39 KB
new4.53 KB
PASSED: [[SimpleTest]]: [MySQL] 59,375 pass(es).
[ View ]

Ugh, I forgot how bad ConfigStorageController::loadByProperties() is. We should fix that (not here).

Here's a fix for the entityQuery part, and the Views UI.

nick_schuch’s picture

20: tour-1918768-20.patch queued for re-testing.

dawehner’s picture

I wonder whether we also should provide some support for route parameters.

kim.pepper’s picture

dawehner, so that we could have different tours depending on a dynamic parameter? Not sure what the use case for this would be.

larowlan’s picture

@kim see comment at #8

nick_schuch’s picture

Status:Needs review» Needs work

node/1 vs node/2? Sounds like this one needs work.

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new8.87 KB
new10.4 KB
PASSED: [[SimpleTest]]: [MySQL] 59,811 pass(es).
[ View ]

thanks to @chx for helping me sort the EFQ syntax here.

tim.plunkett’s picture

  1. +++ b/core/modules/tour/lib/Drupal/tour/Entity/Tour.php
    @@ -130,4 +133,32 @@ public function getExportProperties() {
    +  public function matchRoute($route_name, ParameterBag $route_params) {

    The methods I know of that are prefixed with match* don't return Booleans. I think hasRoute() would be a better name.

  2. +++ b/core/modules/tour/lib/Drupal/tour/TourInterface.php
    @@ -15,12 +16,25 @@
    +  public function matchRoute($route_name, ParameterBag $route_params);
    ...
    +   * @param \Symfony\Component\HttpFoundation\ParameterBag $route_params

    +++ b/core/modules/tour/tour.module
    @@ -108,23 +109,23 @@ function tour_preprocess_page(&$variables) {
    +      if (!$tour->matchRoute($route_name, $request->attributes->get('_raw_variables'))) {

    While the ->has() method is nice, it is weird to see ParameterBag here. It makes it more cumbersome to call this method, and couples it to HttpFoundation. I think this should just call ->all() on the bag before passing it in, and use an array typehint.

    This would also make it easier to unit test

larowlan’s picture

discussed on irc ::hasMatchingRoute() is best name

larowlan’s picture

Also needs an update to tour.schema.yml

vijaycs85’s picture

Had a word with @larowlan: config can be something like https://gist.github.com/vijaycs85/8550098

larowlan’s picture

StatusFileSize
new5.62 KB
new10.86 KB
PASSED: [[SimpleTest]]: [MySQL] 59,750 pass(es).
[ View ]

Fixes 27-30

tim.plunkett’s picture

+++ b/core/modules/tour/config/schema/tour.schema.yml
@@ -69,4 +69,18 @@ tour.tip.text:
+route:
+  type: mapping
+  label: 'Route'
+  mapping:
+    route_name:
+      type: text
+      label: 'Route Name'
+    route_params:
+      type: sequence
+      label: 'Route Params'
+      sequence:
+        - type: string
+          label: 'Param'

This seems like it could be useful to other modules. Maybe move it to system?

larowlan’s picture

StatusFileSize
new1.27 KB
new11.31 KB
PASSED: [[SimpleTest]]: [MySQL] 59,754 pass(es).
[ View ]

damn, had it there first (got to stop second guessing myself)
moved

Status:Needs review» Needs work

The last submitted patch, 33: tour-route-name.33.patch, failed testing.

larowlan’s picture

Status:Needs work» Needs review

33: tour-route-name.33.patch queued for re-testing.

tim.plunkett’s picture

Status:Needs review» Needs work
Issue tags:+Need tests
+++ b/core/modules/tour/lib/Drupal/tour/Entity/Tour.php
@@ -130,4 +132,32 @@ public function getExportProperties() {
+  public function hasMatchingRoute($route_name, $route_params) {

Can we get a unit test for this?

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new6.15 KB
new15.32 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Adds unit test, had to switch from accessing the protected to using getRoutes in the hasMatchingRoute method so I could mock the getRoutes method.
Also moved the static into a protected property and added a reset method cause un-resettable statics are pants.

tim.plunkett’s picture

  1. +++ b/core/modules/tour/tests/Drupal/tour/Tests/Entity/TourTest.php
    @@ -0,0 +1,143 @@
    +   * @group Tour
    ...
    + * Tests the Tour entity.
    + */
    +class TourTest extends UnitTestCase {

    We usually put the group on the class.

  2. +++ b/core/modules/tour/tests/Drupal/tour/Tests/Entity/TourTest.php
    @@ -0,0 +1,143 @@
    +    $tour = $this->getMockBuilder('\Drupal\tour\Entity\Tour')
    +      ->disableOriginalConstructor()
    +      ->setMethods(array('getRoutes'))
    +      ->getMock();

    Why not getMock('Drupal\tour\TourInterface')? Just curious here.

  3. +++ b/core/modules/tour/tests/Drupal/tour/Tests/Entity/TourTest.php
    @@ -0,0 +1,143 @@
    +    $this->assertEquals($result, $tour->hasMatchingRoute($route_name, $route_params));

    We should always use assertSame() (strict comparison)

  4. +++ b/core/modules/tour/lib/Drupal/tour/Entity/Tour.php
    @@ -136,22 +143,21 @@ public function getExportProperties() {
    -    static $keyed_routes;
    -    if (!isset($keyed_routes)) {
    -      $keyed_routes = array();
    -      foreach ($this->routes as $route) {
    -        $keyed_routes[$route['route_name']] = isset($route['route_params']) ? $route['route_params'] : array();
    +    if (!isset($this->keyedRoutes)) {
    +      $this->keyedRoutes = array();
    +      foreach ($this->getRoutes() as $route) {
    +        $this->keyedRoutes[$route['route_name']] = isset($route['route_params']) ? $route['route_params'] : array();

    I love when unit tests lead to better code!

larowlan’s picture

1. ok
2. cause we're testing the getMatchedRoutes implementation, which isn't on the interface
3. ok

larowlan’s picture

StatusFileSize
new1.08 KB
new15.32 KB
PASSED: [[SimpleTest]]: [MySQL] 59,942 pass(es).
[ View ]

The last submitted patch, 37: tour-route-name.36.patch, failed testing.

larowlan’s picture

Issue summary:View changes
larowlan’s picture

2. cause we're testing the hasMatchingRoute implementation, which isn't on the interface

larowlan’s picture

Issue tags:-Need tests

Tests added

tim.plunkett’s picture

cause we're testing the hasMatchingRoute implementation, which isn't on the interface

To clarify, that method does live on the interface.
I forgot about the setMethods() functionality of getMockBuilder, which allows you to use the actual methods in \Drupal\tour\Entity\Tour except the ones you choose to mock. So we're good here.

  1. +++ b/core/modules/tour/tests/Drupal/tour/Tests/Entity/TourTest.php
    @@ -0,0 +1,144 @@
    +          array('route_name' => 'some.route')
    ...
    +        TRUE
    ...
    +        FALSE
    ...
    +        TRUE
    ...
    +        FALSE
    ...
    +        TRUE
    ...
    +        TRUE

    These are missing commas.

  2. +++ b/core/modules/tour/lib/Drupal/tour/Entity/Tour.php
    @@ -130,4 +139,38 @@ public function getExportProperties() {
    +    if (!isset($this->keyedRoutes[$route_name])) {
    +      // We don't know about this route.
    +      return FALSE;

    The unit test does not cover this case. Just need to add a provider case for all non-matching route names

larowlan’s picture

StatusFileSize
new1.92 KB
new15.51 KB
PASSED: [[SimpleTest]]: [MySQL] 60,026 pass(es).
[ View ]

done

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new809 bytes
new15.55 KB
PASSED: [[SimpleTest]]: [MySQL] 59,955 pass(es).
[ View ]

The trailing period on your @covers prevents that from working.
I've switched it to use @coversDefaultClass as we've done elsewhere. That will make any other methods we add later even easier.

Since I'm just changing some PHPUnit directives, I'm still going to RTBC this. Awesome work @larowlan!

vijaycs85’s picture

StatusFileSize
new99.2 KB

Validated with config_inspector + sample views ui config to verify config schema and seems good. Ref: attachement. +1 for RTBC.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed
Issue tags:+Approved API change

Committed 1560601 and pushed to 8.x. Thanks!

Handbook page https://drupal.org/node/1934442 needs updating.

diff --git a/core/modules/tour/lib/Drupal/tour/Entity/Tour.php b/core/modules/tour/lib/Drupal/tour/Entity/Tour.php
index 2821995..1f553c8 100644
--- a/core/modules/tour/lib/Drupal/tour/Entity/Tour.php
+++ b/core/modules/tour/lib/Drupal/tour/Entity/Tour.php
@@ -96,8 +96,6 @@ public function getRoutes() {
     return $this->routes;
   }

-
-
   /**
    * {@inheritdoc}
    */

Removed this empty space during commit.

larowlan’s picture

Updated handbook page

Status:Fixed» Closed (fixed)

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