Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Typehinted integer in PHP 7 is ommited from generated URL when it's zero #112

Closed
wants to merge 4 commits into from

Conversation

@ondrejmirtes
Copy link
Contributor

ondrejmirtes commented Jan 21, 2016

The attached commit shows the described situation.

Expected result of $this->link('edit', ['id' => 0]):

/index.php?id=0&action=edit&presenter=Test

Actual:

/index.php?action=edit&presenter=Test

vitkutny and others added 4 commits Jan 15, 2016
UIRuntime: unnecessary required parameters
@ondrejmirtes ondrejmirtes force-pushed the ondrejmirtes:zero-integer branch from ad16e59 to 1ba768a Jan 21, 2016
@ondrejmirtes

This comment has been minimized.

Copy link
Contributor Author

ondrejmirtes commented Jan 21, 2016

I would love to fix it but honestly don't know how :( PresenterComponentReflection::convertType() correctly processes the parameter as zero.

@dg

This comment has been minimized.

Copy link
Member

dg commented Jan 21, 2016

Actual result is IMHO fine.

@hrach

This comment has been minimized.

Copy link
Contributor

hrach commented Jan 21, 2016

IMO is't a bug.

@ondrejmirtes

This comment has been minimized.

Copy link
Contributor Author

ondrejmirtes commented Jan 21, 2016

I think it's a bug too. "int" represents all integers including negative ones and zero and I don't see a reason why a 0 integer should be treated differently.

@dg

This comment has been minimized.

Copy link
Member

dg commented Jan 21, 2016

This ?id=0 can be achieved in this way:

public function actionEdit(int $id = NULL)

@ondrejmirtes

This comment has been minimized.

Copy link
Contributor Author

ondrejmirtes commented Jan 21, 2016

actionEdit(int $id = NULL) means something different to me, it means that the parameter $id is optional and nullable. But actionEdit(int $id) means that $id is required and ranges from PHP_INT_MIN to PHP_INT_MAX and it cannot be NULL.

@Majkl578

This comment has been minimized.

Copy link
Contributor

Majkl578 commented Jan 21, 2016

I too think it's a bug unless the action is with default value (actionFoo(int $id = 0)). 0 is a valid value.

@dg

This comment has been minimized.

Copy link
Member

dg commented Jan 21, 2016

So what it should do when a=0 is not in URL?

@ondrejmirtes

This comment has been minimized.

Copy link
Contributor Author

ondrejmirtes commented Jan 21, 2016

So what it should do when a=0 is not in URL?

This question is about matching URLs and this PR is about constructing them.

But I'd say in this case the result of $this->getParameter('id') is null and null cannot be passed to int $id, so it's basically a type mismatch.

@hrach

This comment has been minimized.

Copy link
Contributor

hrach commented Jan 21, 2016

When a is not present, null should be passed to method call, an php exception will be triggered and handled by the app's error presenter.

@dg

This comment has been minimized.

Copy link
Member

dg commented Jan 21, 2016

i.e. it should throw 500 error? Ohh…

@ondrejmirtes

This comment has been minimized.

Copy link
Contributor Author

ondrejmirtes commented Jan 21, 2016

It means that the router is misconfigured/implemented poorly so I'm fine
with 500 too.

On Thursday, 21 January 2016, David Grudl notifications@github.com wrote:

i.e. it should throw 500 error? Ohh…


Reply to this email directly or view it on GitHub
#112 (comment).

Ondřej Mirtes

@Majkl578

This comment has been minimized.

Copy link
Contributor

Majkl578 commented Jan 21, 2016

It means that the router is misconfigured/implemented poorly so I'm fine with 500 too.

👍

@dg

This comment has been minimized.

Copy link
Member

dg commented Jan 21, 2016

It is not about router. We are talking about ?a=0 in query.

And 500 means server error. Server error due to the removal of a single parameter from URL? Boys! Do not be silly :-)

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Jan 21, 2016

As I proposed couple of years ago, missing required parameter (or wrong type, e.g. abc when int is required) should result in BadRequestException#404. Note that any such change would probably require new major version.

@ondrejmirtes

This comment has been minimized.

Copy link
Contributor Author

ondrejmirtes commented Jan 21, 2016

I want the HTTP request to give me an integer. If there's no integer in the request, I have to respond with an error. What kind of error is a subject to discuss.

I think this layer of request processing should be stopped at the router level. If an incorrect type is received by a presenter (involving some lossless type casting), something is very wrong and I would like this situation to result in a new error log line. So I'm voting for a 500 error.

But if Nette wants to be more loose in these situations, I can accept throwing 404 too.

@Majkl578

This comment has been minimized.

Copy link
Contributor

Majkl578 commented Jan 21, 2016

@dg: The issue was about generated URLs, not their handling.
If you require a parameter in the action and it happens to be missing in the request, it is obviously not OK. What @JanTvrdik proposes makes sense in such case.

Also I see a huge difference now that we have PHP 7 with scalar types. In the old times we often used default value (e.g. actionFoo($id = 0)) just to express the default type, but it wasn't valid signature as the parameter is required. With PHP 7 we no longer need to use this hack as we can use proper typehints (e.g. actionFoo(int $id)).

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Jan 21, 2016

Possibly related (5 years old) forum thread https://forum.nette.org/cs/6819-validace-parametru-v-presenterech but due to it's age lot's of information and ideas may be outdated

@hrach

This comment has been minimized.

Copy link
Contributor

hrach commented Jan 21, 2016

i.e. it should throw 500 error? Ohh…

It doesn't mean, that the resource does not exist. You didn't fullfill the contract, server failed to do his job. That's all.

I'm probably ok with 400. But 404 is non-sence.

@pilec

This comment has been minimized.

Copy link
Contributor

pilec commented Jan 21, 2016

@ondrejmirtes

This comment has been minimized.

Copy link
Contributor Author

ondrejmirtes commented Jan 21, 2016

Can we go back to the original issue of constructing URL with zero integer as a parameter and create a new thread for support for scalar typehints in presenters and how to solve receiving requests with them? Thank you.

@dg

This comment has been minimized.

Copy link
Member

dg commented Jan 21, 2016

@JanTvrdik As I proposed couple of years ago, missing required parameter should result in BadRequestException#404

This is obviously a good idea, but the giant BC break.
But it would be possible with scalar typehints because they are new (so no BC break), just inconsistency.

@ondrejmirtes I want the HTTP request to give me an integer.

It does :-) It gives you 0. Btw, whether it is, number of page or id, you have to validate it.

… I have to respond with an error. What kind of error is a subject to discuss.

Absolutely not. There's nothing to discuss, it just needs to be BadRequestException. (ie. 4xx, imho 404 is fine)

I think this layer of request processing should be stopped at the router level.

How can you define in router, that the query must contains a parameter a? And numeric in addition.

@Majkl578 The issue was about generated URLs, not their handling.

Generating and matching are very close. We cannot discuss about first one and aviod discussion about second one.

@ondrejmirtes

This comment has been minimized.

Copy link
Contributor Author

ondrejmirtes commented Jan 21, 2016

It does :-) It gives you 0. Whether it is, number of page or id, you have to validate it.

If there's a missing parameter in the request, I don't want a random integer. Zero has the same weight as any other integer.

How can you define in router, that the query must contains a parameter a? And numeric.

/admin/product/<presenter>/<action>[/<id [1-9][0-9]*>] - this is how we define positive integers in id parameter in masks in Nette\Application\Routers\Route. Any integer can be defined as [0-9]+.

@dg

This comment has been minimized.

Copy link
Member

dg commented Jan 21, 2016

@ondrejmirtes Can we go back to the original issue of constructing URL with zero integer as a parameter and create a new thread for support for scalar typehints in presenters and how to solve receiving requests with them? Thank you.

No, it is complex problem. You have to take into account the compatibility. What happens when someone will upgrade PHP 5 renderDetail($id = 0) to PHP 7 renderDetail(int $id). In old links there is no id=0. Etc.

(ad router: I mean query part of URL).

@ondrejmirtes

This comment has been minimized.

Copy link
Contributor Author

ondrejmirtes commented Jan 21, 2016

renderDetail($id = 0) and renderDetail(int $id) mean something different, the first one has default value and the second one does not. Correct refactoring to PHP 7 code is from renderDetail($id = 0) to renderDetail(int $id = 0).

@hrach

This comment has been minimized.

Copy link
Contributor

hrach commented Jan 21, 2016

Correct refactoring to PHP 7 code is from renderDetail($id = 0) to renderDetail(int $id = 0).

👍

@Majkl578

This comment has been minimized.

Copy link
Contributor

Majkl578 commented Jan 21, 2016

@pilec: 422 is not valid for this case, because request URI is not an entity (entity = http body).

@dg

This comment has been minimized.

Copy link
Member

dg commented Jan 21, 2016

  1. we can responds with 404 when parameter with scalar typehint and without default value is missing
  • no BC break (or tiny)
  • solves this issue
  • makes inconsistency with typehint array, with parameters without typehints and persistent parameters
  1. we can responds with 404 when any parameter without default value is missing
  • giant BC break
  • solves this issue
  • all parameters wil be handled the same way
  1. we can pass default values to parameters with scalar typehits and without default values (current behavior)
  • no BC break
  • solving this issue will create inconsistency how Nette generates URL
  • all parameters are handled the same way

I think that 2) is impossible and 1) vs 3) is about consistency (ie. a about different handling of array $arr vs. int $id etc).

IMHO 1) is best way.

@hrach

This comment has been minimized.

Copy link
Contributor

hrach commented Jan 21, 2016

IMHO 1) is best way.

👍 , still, I'm more for 400.

@dg

This comment has been minimized.

Copy link
Member

dg commented Jan 21, 2016

400 is perfectly ok, I'll try to find out how it affects Google. For example, 410 is big problem, because Google will ignore this URL, although it becomes exist in future. (And maybe that's not true anymore.)

@ondrejmirtes

This comment has been minimized.

Copy link
Contributor Author

ondrejmirtes commented Jan 21, 2016

400 is good from the point of view of the HTTP spec, but it will likely cause people having to write a new latte template or change their code in the ErrorPresenter.

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Jan 21, 2016

I think that the best choice for Nette 3 is (1) + change array typehint behavior to match other typehints (BC break) + possibly implement support for class typehints (e.g ˙show (Article $article)˙)

@dg

This comment has been minimized.

Copy link
Member

dg commented Jan 21, 2016

Class type hints are IMHO not needed, because object is created in router, which is tightly coupled with presenter.

Now I realized that they are already suppoted!

@dg

This comment has been minimized.

Copy link
Member

dg commented Jan 21, 2016

I forgot to mention persistent parameters - so 1) makes inconsistency between typehinted parameters and persistent parameters, because properties in PHP cannot have typehints. It is possible to use annotation @var, but it will be BC break.

@dg

This comment has been minimized.

Copy link
Member

dg commented Jan 21, 2016

@JanTvrdik change array typehint behavior to match other typehints (BC break)

It is tricky, because it is not possible to have empty array in URL. In fact, array is consistent with both solutions.

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Jan 21, 2016

Random thoughts:

  • We could make array $list mean "at least 1 item" and make array $list = [] mean "any number of items".
  • You cannot have empty array in URL but you can have empty array in application request.
@dg dg force-pushed the nette:master branch from aa721ef to 74f79a9 Jan 21, 2016
dg added a commit that referenced this pull request Jan 22, 2016
…type hint, no default value and argument is missing [Closes #112]
dg added a commit that referenced this pull request Jan 22, 2016
…type hint, no default value and argument is missing [Closes #112]
@dg dg force-pushed the nette:master branch from 74f79a9 to e624863 Jan 22, 2016
@dg dg closed this in 18ab357 Jan 22, 2016
dg added a commit that referenced this pull request Jan 22, 2016
…default value and argument is missing [Closes #112]
dg added a commit that referenced this pull request Jan 22, 2016
…default value and argument is missing [Closes #112]
dg added a commit that referenced this pull request Jan 22, 2016
…default value and argument is missing [Closes #112]
dg added a commit that referenced this pull request Jan 22, 2016
…default value and argument is missing [Closes #112]
dg added a commit that referenced this pull request Jan 22, 2016
…default value and argument is missing [Closes #112]
dg added a commit that referenced this pull request Jan 22, 2016
…default value and argument is missing [Closes #112]
dg added a commit that referenced this pull request Jan 22, 2016
…default value and argument is missing [Closes #112]
@ondrejmirtes

This comment has been minimized.

Copy link
Contributor Author

ondrejmirtes commented Jan 22, 2016

Cool, thanks! 👍

@ondrejmirtes ondrejmirtes deleted the ondrejmirtes:zero-integer branch Jan 22, 2016
@hrach

This comment has been minimized.

Copy link
Contributor

hrach commented Jan 22, 2016

Great! Thank you!

@fprochazka

This comment has been minimized.

Copy link
Contributor

fprochazka commented Jan 22, 2016

Looks great 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.