Refactor tour module to use routes instead of paths

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

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

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

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.

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

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

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

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

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

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.

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

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

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

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

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

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

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

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

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!

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

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!

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

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.