Skip to content

Conversation

@JanTvrdik
Copy link
Contributor

Http\Request::getFile() is inconsistent with getQuery and getPost

  • it uses Nette\Utils\Arrays::get; getQuery and getPost do not
  • it does not support $default but always returns NULL (that kind of make sense) for undefined keys.

@JanTvrdik
Copy link
Contributor Author

Solved by JanTvrdik@4c45beb

@JanTvrdik JanTvrdik closed this Nov 2, 2014
@dg
Copy link
Member

dg commented Nov 4, 2014

The reason is that getFile returns (as leafs) objects. For query and post data you can use:

$post['x'] = 123;
isset( $post['x']['y']['z'] ); // return FALSE

but not for files:

$files['x'] = new stdClass;
isset( $files['x']['y']['z'] ); // FATAL ERROR

@dg dg mentioned this pull request Nov 8, 2014
@JanTvrdik JanTvrdik reopened this Nov 9, 2014
@JanTvrdik
Copy link
Contributor Author

One note: Nette itself never uses methods getQuery(), getPost() or getFile() to get a specific key. Method getCookie with specific key is used in sessions.

What I love about it is consistency.
What I hate about it is performance. Getting a specific has now much bigger overhead. However it would have very little impact on the framework itself.

@JanTvrdik JanTvrdik force-pushed the multiple_keys branch 2 times, most recently from 2b40288 to 6bbdaef Compare November 9, 2014 23:25
@dg
Copy link
Member

dg commented Nov 10, 2014

I agree with need for consistency, but I think that better way is to deprecate current (unused) getFile behavior than increase complexity, that nobody needs.

Deprecate getFile is BC break, this solution is BC break for maintainers of own IRequest implementations.

@JanTvrdik
Copy link
Contributor Author

ok, will do

@JanTvrdik
Copy link
Contributor Author

Done – is it OK now?

@dg
Copy link
Member

dg commented Dec 8, 2014

👍

dg added a commit that referenced this pull request Dec 8, 2014
Fix Http\Request::getFile() inconsistency
@dg dg merged commit 88784b1 into nette:master Dec 8, 2014
@JanTvrdik JanTvrdik deleted the multiple_keys branch December 8, 2014 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants