Add exception throwing, when controller action not found

[MalikinSergey]

If undefined action passed In UrlGenerator::action(), it's throw fatal exception

Call to a member function domain() on a non-object

with fully uninformative debug stack

because route, which was returned by $this->routes->getByAction($action) is not checked, before it passes to $this->toRoute(... and can be null instead of Route

So, it seems logical to add some check, as in UrlGenerator::route()

[GrahamCampbell]

Fix the cs.

[MalikinSergey]

Done, right?

[GrahamCampbell]

Reverse the logic here, and throw the exception inside the guard statement.

[GrahamCampbell]

Thanks. Please fix the remaining issue, add tests, and squash to one commit.

[GrahamCampbell]

Also, please send this to 5.1. It's a change in behaviour.

[MalikinSergey]

@GrahamCampbell
about test, it's need to add to RoutingUrlGeneratorTest::testBasicRouteGeneration() case like this?

$nonExistsAction = 'SomeControllerThatNotExists@action';
$this->setExpectedException('InvalidArgumentException','"Action {$nonExistsAction} not defined."');
$url->action($nonExistsAction);

https://github.com/laravel/framework/blob/5.0/tests/Routing/RoutingUrlGeneratorTest.php#L114

[franzliedke]

How's this a change in behaviour? There used to be a misleading error message, now we throw an exception? That makes sense, and it's a bug fix.

[GrahamCampbell]

Not really. The misleading error message is the current behaviour people expect from 5.0. Throwing an exception instead would be a bc break.

[franzliedke]

I'm fairly sure people do not build on this behaviour.

There used to be an exception, now there's another one. This is still totally valid.

[barryvdh]

I've also encountered the Call to a member function domain() on a non-object error, it's pretty confusing indeed. The old behaviour didn't work right? So not sure how this is breaking?

[MalikinSergey]

@franzliedke @barryvdh
I agree with @GrahamCampbell, it's a behaviour change; for example, somebody can write logic like

try{
  $action = action('non-exists-controller@bar');
}
catch(FatalException $fatal){
  //some logic for handle this
}

and if we will change exception type, this code will not work properly.

See http://laravel.com/docs/4.2/contributions#which-branch

Minor features that are fully backwards compatible with the current Laravel release may be sent to the latest stable branch.

So, I've sent pull #7453 to 5.1 master branch

[MalikinSergey]

But I recognize, that current behaviour with fatal exception is very complicated to debug

[franzliedke]

@MalikinSergey Of course this is technically breaking. But I don't see who would build something around it like this... especially given that you shouldn't and you're simply dealing with a non-existant controller action.

[JoostK]

Relying on such a bug to potentially detect if a route exists is very much a dirty hack, to be solved very differently, e.g. by Route::getRoutes()->getByAction($action);.

This is an implementation detail and should never have been relied upon. Using the arguments here we can label almost every bug-fix as backwards incompatible, as someone may always have written a dirty hack around it.

[mikerogne]

Phew, thanks :)