search

fuelphp-storage / foundation

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

Component path in route #15

Closed sagikazarmark closed 8 years ago

sagikazarmark commented 9 years ago

If a route is found the first path of the component is added to the route here. IMO it is bad, since it cannot be determined (if the namespace points to multiple directories) which is the current on.

The route should probably contain all the paths or none of them, since the path is available from the component which is available in the request. I feel a bit of redundacy here, but I can live with that.

sagikazarmark commented 9 years ago

I checked the Routing package, and the path in the route is clearly not part of it, so it is added in the foundation. I tend to remove that and get the path from the component instead. IMO route has nothing to do with file paths.

@WanWizard correct me If I am wrong, I still get lost in foundation sometimes.

WanWizard commented 9 years ago

The Routing class in Foundation is a wrapper for the routing package, and does Fuel specific things. One of those things is resolve the route to a component, and a controller in that component, which is what that bit of code does. This is not the task of the Routing package.

Having said that, this part needs to be revised anyway. It is currently used in https://github.com/fuelphp/foundation/blob/master/src/Request/Local.php#L139 (and it is removed again later), which breaks HMVC requests within the same component. This is because the Finder doesn't have a stack, so adding the same path twice will not work.

sagikazarmark commented 9 years ago

Ok, so we agree that the route has nothing to do with the path, thus it can be removed and component can be used instead for getting paths.

For the HMVC problem: how about cloning the finder, setting the paths there and putting back the original finder when the request is done?

WanWizard commented 9 years ago

Yes, correct.

The paths should be local to the request context, but the Finder currently isn't.

It's used everywhere, so we have to make sure it's implemented using a getFinderInstance() serviceprovider extension, that will get the instance from the last Request object on the stack (and if that does not exist, from Application).

This implies that Request needs to be injected with a clone of the Applications' Finder object, and it needs a getFinder() method to retrieve that. Everything within the Request then uses that clone, and the clone will be discarded when the Request finishes.

WanWizard commented 9 years ago

Thinking about it a bit more: paths are active on both the Application level (which contains the paths to the "core" default files) and on the Component level (specific paths for that component).

So probably it's better if Application, Component and Request all have their own instances of Finder, and Finder gets an inheritance system for paths, so it can use it's parents paths in a not-found situation. Similar to the way Input and Config now work. It allows you to change the paths on a per-request basis, without modifying the paths of the Component...

emlynwest commented 8 years ago

Closing because of foundation rewrite.