Problem/Motivation

Currently, if the "bundle" parameter is used as a route parameter inside a tour, it can only be used on nodes.

See line 134-142 inside "TourHelper":

              case 'bundle':
              case 'node_type':
                if (($entity = $route_match->getParameter('node')) && ($entity instanceof EntityInterface)) {
                  if ($entity->bundle() === $tour_param) {
                    // Add the node type (bundle) as comparable parameter.
                    $params['bundle'] = $entity->bundle();
                  }
                }
                break;

Steps to reproduce

Use a non node type as the bundle key. E.g.:

    route_name: entity.taxonomy_term.edit_form
    route_params:
      bundle: tags

See it not working.

Proposed resolution

Find a way to get the entity from the "$route_match" or some other way, without passing an actual parameter, or use a dynamic parameter that fits the current entity type?

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork tour-3554749

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

grevil created an issue. See original summary.

grevil opened merge request !128

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Priority: Normal » Minor

This is indeed unexpected and confusing. I think you would either expect bundle: to work for any entity types, not only nodes (that would be best and should also be bw-compatible).

The harder part is this line:

if (($entity = $route_match->getParameter('node')) && ($entity instanceof EntityInterface)) {

with hard-coded "node"!

See this discussion and suggestions for this general problem:
https://www.drupal.org/forum/support/module-development-and-code-questio...

Maybe we could use one of the ways shown there or this might even need a Drupal Core API helper?

anybody’s picture

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

Let's see, if this works...

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Nice, it works and fixed the failing test! Still please review carefully if it all makes sense.

Was more kind of trying if this may work...

smustgrave made their first commit to this issue’s fork.

smustgrave’s picture

smustgrave commented

made a tweak where I just combined the case 'node_type' with the node and taxonomy. Locally the test still passes. Did this because not a fan of a switch case without a break :)

smustgrave’s picture

smustgrave commented

any objections to these changes? I can merge

anybody’s picture

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

@smustgrave I left a comment. I'd expect this to break things. To be totally sure, we'd need more test coverage for all the cases where these switch cases are used (if even possible and not unreachable code)

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review

So added a check if node_type then change key to node that work ?

grevil’s picture

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

Nice! Works as expected! RTBC!

smustgrave’s picture

smustgrave commented
Status: Reviewed & tested by the community » Fixed

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.