bitExpert / pathfinder

[DEPRECATED] A PSR-7 based router
Apache License 2.0
5 stars 4 forks source link

Made Route constructor protected to enforce the creational static method calls. #23

Closed shochdoerfer closed 8 years ago

shochdoerfer commented 8 years ago

Also corrected docblock comments as well as the method parameter type hints.

Reason for this change is to make it clearer how to construct a route object. Especially the $matchers parameter of the constructor is not really good because one needs to know how the array needs to be constructed.

To discuss: I would love to get rid of the $matchers parameter for the creational methods (e.g. get(), post()...) for the very same reason. Since the array is used internally as it was given one does need to know the exact structure of the array upfront. We do already have the ifMatches() method so be able to set a matcher individually. I think this is enough we need to offer.

dropdevcoding commented 8 years ago

I think if you set the accessibilty of the constructor to protected in order to enforce the "declarative style" we could even go one step further:

Imho the declaration of a route like:

Route::get('/user')->to('userAction');

is somewhat intuitive, so the static methods possibly should only have a single param: The path definition.

This also would reduce complexity inside the Route and its tests.

Packages using the Route (like adrenaline) will not be heavily affected, since the route may still be built with by using the according declarative functions (to, ifMatches).

So if you agree you also may change the signatures of that static functions in your PR to:

public static function get($path)
public static function post($path)
// ...
shochdoerfer commented 8 years ago

Makes sense but should a route not at least contain the target. A route with just an url is useless, or am I wrong?

shochdoerfer commented 8 years ago

Cleaned up the static methods. I would love to get rid of the optional $path and $target parameters. What is your opinion on that matter?

dropdevcoding commented 8 years ago

Well, I think in favour of intuitive usage, we should remove the optional target and make path become mandatory.

Of course you are right that a route without a target doesn't make sense but the router validates the route before being added because without that validation the declarative style wouldn't be possible since during the declaration the route is in invalid state.

So there are two possibilities: Make path and target mandatory and remove the route validation inside the router (which will make the route definition less intuitive but the route "cleaner") or make path mandatory, remove target which will make the definition become more intuitive and readable, but accepts a dirty state of the route.

If we removed the validation in router, we still would need to ensure, that the route has a name if its target is not a string, since this is needed for generateUri but we possibly could move that check to the generateUri itself.

shochdoerfer commented 8 years ago

As discussed with @dropdevcoding we will move the creational logic into a RouteBuilder which will construct a valid route object. To be able to do that the current changed should be merged to make sure we got the documentation fixes in place.