Problem/Motivation

This is the problem code. There are two ways that the throw can never be reached.

    if ($name instanceof SymfonyRoute) {
       $route = $name;
     }
     elseif (NULL === $route = clone $this->provider->getRouteByName($name)) {
       throw new RouteNotFoundException(sprintf('Route "%s" does not exist.', $name));
     }

1) This interface only allows returning of a route object. If it can't it already throws a RouteNotFoundException.
2) If it _did_ return NULL, it just triggers a fatal error because you can't clone NULL. https://3v4l.org/0YOG1

Steps to reproduce

You can return null from a route provider, like using a empty mock to trigger the fatal but the 3v4l link demonstrates the problem.

Proposed resolution

Remove the elseif logic.

Remaining tasks

n/a

User interface changes

n/a

API changes

n/a

Data model changes

n/a

Release notes snippet

n/a

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neclimdul created an issue. See original summary.

neclimdul’s picture

Status: Active » Needs review
FileSize
648 bytes

This should be an easy novice review.

Note, there are no tests because the code is unreachable so there isn't anything to test. Any tests that could trigger a problem would have to break interfaces.

Status: Needs review » Needs work

The last submitted patch, 2: 3184619-1.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

neclimdul’s picture

Status: Needs work » Needs review

random failure

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

What a nice little find in my triaging journey.

Starting tests on 9.3.x

quietone’s picture

Status: Needs review » Reviewed & tested by the community

I read the IS and looked at https://3v4l.org/0YOG1 and read the code. It all looks good to me. And of course, no tests are failing.

So, RTBC from me.

quietone’s picture

And it is going to rested on 9.2.x instead of 9.3.x. :-(

neclimdul’s picture

How nice of you to say!

Here's a patch with the use fix that should make CI happy again. Just going to leave it RTBC to save some noise on such a trivial fix.

  • catch committed 3730c17 on 9.4.x
    Issue #3184619 by neclimdul, quietone: Fix unreachable logic in...

  • catch committed 59d8ab1 on 9.3.x
    Issue #3184619 by neclimdul, quietone: Fix unreachable logic in...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.4.x and cherry-picked to 9.3.x, thanks!

Status: Fixed » Closed (fixed)

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