fuelphp-storage / foundation

FuelPHP Framework - Framework foundation library
MIT License
19 stars 8 forks source link

routing issue when using closures #6

Closed Remo closed 11 years ago

Remo commented 11 years ago

I've installed fuelphp2 on a new ubuntu dev box and found an issue I'm not sure how to handle, my experience with fuelphp is too limited..

When I open /index.php/test/, I get this error:

strpos() expects parameter 1 to be string, object given

The problem happens here https://github.com/fuelphp/foundation/blob/master/src/Fuel/Foundation/Request/Local.php#L184 where I get a closure object since the test route is definied like this:

$router->all('test', function() { return 'welcome/index'; });

I'm not sure if this still supported, I could easily replace it with a string.

WanWizard commented 11 years ago

Both 1.x and 2.0 do not support a closure to be the route target.

When you define a closure in the route, it is processed as an inline controller action. This is broken in the current 2.0 code, fixed locally, but not pushed yet.

Remo commented 11 years ago

I see! Are there lots of changes that haven't been pushed yet?

WanWizard commented 11 years ago

Pushed my local changes. Not properly tested, so it might still have issues.

Remo commented 11 years ago

The closure doesn't seem to be properly executed, it runs into this exception: https://github.com/fuelphp/foundation/blob/master/src/Fuel/Foundation/Request/Local.php#L144

WanWizard commented 11 years ago

As the exception says, a controller method (or a closure in this case) MUST return a Response object. Returning a string is not supported, 2.0 doesn't do automatic encapsulation, that was already deprecated in v1.5.

This works fine:

$router->all('test', function() { return \Response::forge('html', 'welcome/index'); });

which also shows you one of the reasons it's not supported, the Reponse object type can not be "guessed"....

Remo commented 11 years ago

makes sense, but the demo app doesn't do it this way. https://github.com/fuelphp/demo-application/pull/1 ?

Remo commented 11 years ago

that code didn't quite work for me. I've created a second pull request which works for me: https://github.com/fuelphp/demo-application/pull/2

WanWizard commented 11 years ago

Demo code wasn't pushed either... Which I've done now.

Remo commented 11 years ago

thanks!

WanWizard commented 11 years ago

there was also a change in the fuelphp package (updated index.php), which I've pushed as well.