Problem/Motivation

Using the same tour on multiple routes is generally allowed through "routes" (plural).

But entering multiple routes:

entity.node.canonical
- node:333
entity.node.canonical
- node:357

which in the tour.XYZ.yml the results in:

routes:
  -
    route_name: entity.node.canonical
    route_params:
      node: '333'
  -
    route_name: entity.node.canonical
    route_params:
      node: '357'

is not working correctly, as I could verify step-debugging through the code!

In TourHelper.php the loop over the route parameters needs to break; once a match is found, instead it continues and so only the last entry "wins"! So technically this needs to be an "or", but is not even an "and".

Only the last entry is really taken into account and so the tour only shows up on the last route in the list.

Steps to reproduce

See above

Proposed resolution

  1. Add an example to the field #descrption for an entry with multiple routes (UI)
  2. Write a test showing this issue
  3. Fix the implementation to ANY match

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork tour-3553841

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

anybody created an issue. See original summary.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Issue summary: View changes
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

I'm on it and will provide details or if possible a fix!

smustgrave’s picture

smustgrave commented

Will request tests for this too please.

anybody opened merge request !124

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@smustgrave here we go. Tests are on the way. Reading through the code I also found #3554030: Wrong comparable parameter "bundle" is used for taxonomy_term / node ids.

The key issue seems to be that multiple routes don't work, if they use the same route name, which may be needed like in the example above (entity.node.canonical).

In that case the lines in Tour::hasMatchingRoute() discard the routes when building the $this->keyedRoutes array, as you can see in the code.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Title: Routes evaluation is wrong if more than one route is given » Routes evaluation is wrong if more than one route is given with the same name

tour.tour.tour-test.yml defines multiple routes, but they are all distinct:

routes:
  -
    route_name: tour_test.1
  -
    route_name: tour_test.1_no_action
  -
    route_name: tour_test.3
    route_params:
      locale: foo
  -
    route_name: node.add

Which seems impossible in my example. So maybe the case given in the issue summary has never been thought of and tested?

anybody opened merge request !125

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Test-only branch fails as expected and exactly shows the issue we have.

I'll further work on the fix.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Active » Needs review

Whao, I think I solved it... Please take a look. Also works for our real-world case now.

Better review carefully to ensure I didn't make a mistake like the one before me ;)

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

I'll add another case in tour_dashboard_test very similar to our case!

anybody changed the visibility of the branch 3553841-maybe-further-tests to hidden.

anybody opened merge request !126

grevil’s picture

grevil commented

Perfect, looks great! RTBC!

Added one comment, but unsure whether it is out of scope or not, but I don't like the new label. We should revert it and adjust it in a potential follow-up issue, OR fix it here and adjust the label as mentioned in the MR.

Edit: Nevermind, got confused.

grevil’s picture

grevil commented
Status: Needs review » Reviewed & tested by the community

Setting to RTBC for now, but still needs feedback by @smustgrave concerning #14.

smustgrave’s picture

smustgrave commented
Status: Reviewed & tested by the community » Needs work

Seems to have test failures

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs work » Needs review

@smustgrave sorry I just played around a bit with additional tests in the additional MR and should have marked that as Draft! That was confusing.

MR!124 contains the needed tests and fixes. MR!125 contains the same tests only.

MR!126 was just to write done some ideas for further tests, but wasn't finished. We may decide to close it or proceed there once merged. I'm fine with both.

smustgrave closed merge request !125

smustgrave closed merge request !126

  • smustgrave committed 5016c88b on 2.0.x authored by anybody 
    Issue #3553841 : Routes evaluation is wrong if more than one route is...
smustgrave’s picture

smustgrave commented
Status: Needs review » Fixed

Test coverage seems fine.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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