hoaproject / Router

The Hoa\Router library.
https://hoa-project.net/
28 stars 11 forks source link

New feature : value control in unroute #12

Open guiled opened 10 years ago

guiled commented 10 years ago

Is it possible to have a value control in Router\Http in the unroute ? In this lines https://github.com/hoaproject/Router/blob/af9b54379fd67c00bb419018e7eeb1482dba008e/Http.php#L596-L610 I think it could be possible to extract the regexp defining the value format and make a simple test on the concerned value given to unroute (edit: I change the source link)

--- Want to back this issue? **[Post a bounty on it!](https://www.bountysource.com/issues/4942876-new-feature-value-control-in-unroute?utm_campaign=plugin&utm_content=tracker%2F459946&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F459946&utm_medium=issues&utm_source=github).
Hywan commented 9 years ago

I assign @osaris on this PR :-).

osaris commented 9 years ago

That make sense but my main concern is about performances ? Thoughts @hoaproject/hoackers ?

Hywan commented 9 years ago

It makes sense yes, but in which context? In a development environment maybe, in a production one it is better to let the error propagate and reach a 404 no (this is very stupid question)?

stephpy commented 9 years ago

If we allow to make “value validation“ into routes, it should be done in DEV and PROD environement, dev environment could be more verbose in exception message.

But i'm not sure it's to the router to do that. Your controller have already to check if the ID of the resource is exist, if the xxx value is valid, etc ...

However, a postValidation closure could be added to routes, not sure it's a good idea yet.

guiled commented 9 years ago

What I meant by "value control" is to check the value with the regexp extracted from the route. For example : (?\d+) We should check if the value match with /\d+/ Why put that control in unroute where it seems to be the wrong place to do that? Because it is the only place where we can do it with the route definition... For the question DEV or PROD, I think we should have a parameter somewhere in the unroute process to enable it (it should be disabled by default) Then we could have something like _unroute($id, $pattern, $variable, $allowEmpty, $dataControl = false)

osaris commented 9 years ago

I think @guiled is right when he says "it is the only place where can do it". You can't compare this check with the check of the existence of the ID as it's a lower level check here. We also need to keep in mind that this check is only for unroute so for data sent by the developer to the router (so data that must be clean), not for user input to the router. So it makes sense to only do this check in dev/test env and not production nope ?

Hywan commented 9 years ago

@osaris: :+1:. @guiled: Not sure about an argument to toggle (enable or disable) the validation. The user (of the library, so the developer) has written a regex. The URI must match the given the regex. So, from my point, we have to validate each time and not allow the user to disable this behavior, it has no meaning.

Thoughts?

Hywan commented 9 years ago

ping?

Hywan commented 9 years ago

ping :-)?

Hywan commented 9 years ago

Last ping?

thehawk970 commented 9 years ago

Validation on unroute can be usefull if a dev in a team "Damn this value xxx will match my regex of the death ?" and have a proper Exception "No dude, xxxx can not match /\d+/" (in dev mode ofc) But in prod mode this can be used for detect an hacking ? if we got "No dude, "; SELECT ..." can not match "/\d+/"

So i don't have any idea to code this :dancer:

Hywan commented 9 years ago

Ok so new status: This issue is ready with a casual difficulty. This is not that hard to implement. If some one wants help, ping @osaris, @Metalaka, @vonglasow, @K-Phoen or me on IRC :-].