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

Wrong CodeIgniter::handleRequest method definition #1786

Closed
hwiesmann opened this issue Mar 2, 2019 · 7 comments
Closed

Wrong CodeIgniter::handleRequest method definition #1786

hwiesmann opened this issue Mar 2, 2019 · 7 comments

Comments

@hwiesmann
Copy link

Describe the bug
The method CodeIgniter::handleRequest is incorrectly defined because the second parameter does not have a default value, respectively the first one has a default value. This bug exists since commit 1f08145.

protected function handleRequest(RouteCollectionInterface $routes = null, $cacheConfig, bool $returnResponse = false)

CodeIgniter 4 version
CodeIgniter4 Beta 0.0.1

Affected module(s)
CodeIgniter

@atishhamte
Copy link
Contributor

The function arguments were not changed.

@hwiesmann
Copy link
Author

No, that is right. But I am talking about the default value and not about parameters themselves. And a default value has been added (see commit).

@nowackipawel
Copy link
Contributor

Unfortunately there is a lot of methods defined that way everywhere in Ci - helpers , system modules . Ofc you are right about order (args without and with defaults) it should not happend but... as for now imho we have live with this. Not sure about this case and looks like nothing but happen, but ... sometimes default args are not desired and it is better to get exception (sh* happens) instead of unpredictable results :/

It is good it is protected method (not many people are using it as for now) but anyway on this stage it could be dangerous to change order of args as well as add default value :/

@hwiesmann
Copy link
Author

But this commit occurred only 3 days ago in a beta version! This sh* should be taken out otherwise you have problems for years...

@nowackipawel
Copy link
Contributor

If it is as you sayin then it shouldn't happend :/.
I saw that commit .. there are around 20 occurrences of that issue , mostly in Validation module.

@lonnieezell
Copy link
Member

I agree that best practices say you should put nullable and there's good reasons. I might have overlooked the first two in the commit, but all of the ones in validation rules are fine. Best practices cannot always account for every situation. The way that validation rules work this works out better than following the best practice.

The two that times in that commit that are not validation rules are internal methods that other developers won't touch so, while still not great and things that I will revisit, they don't affect anyone using the framework in any way. Just the handful of us that are contributing.

I do have an issue with calling something that does not follow a common practice "incorrect", though. It's only incorrect if the wrong arguments are being put in, or are in the wrong order, etc.

@InsiteFX
Copy link
Contributor

InsiteFX commented Mar 4, 2019

Parameters that have values should be placed at the end of the parameter list then you will not get any editor errors

@jim-parry jim-parry added this to the 4.0.0-beta.2 milestone Mar 5, 2019
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

No branches or pull requests

6 participants