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
code cleanup #5813
code cleanup #5813
Conversation
I'll rebase the branch in case you want to merge it. 😏 |
@@ -134,7 +134,7 @@ protected function applyGenericFilters($datatable) | |||
} | |||
|
|||
try { | |||
$value = Common::getRequestVar($name, $defaultValue, $type, $this->request); | |||
$value = Common::getRequestVar($varName, $defaultValue, $type, $this->request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you could put this in a different pull request since it looks like it could fix a bug and is not just code cleanup?
Sounds good, we'll definitely merge it in! |
Outsourced the bugfix into #5835. Remainder rebased. The changes only apply to the |
sometimes we put the variable even though it's the default value, because it may help readability. So I wouldn't change that.
+1, |
Thanks for pull request! |
I don't agree with that. Those variables clutter the code, making method calls much longer than needed and thus harder to read. A proper IDE will show the method signature quickly when needed, but there are numerous warnings about unused variables because of this, which makes it hard to spot those which are in fact typos (or bugs). |
The 1st commit can be considered to be a bugfix because the wrong variable was used there, at least with my understanding of the code.The
2nd1st commit removes unused variables while trying not to break stuff, as the code seems to be pretty fragile. There are still plenty of unused variables when calling methods in the manner of naming arguments like forself::json_decode($value, $assoc = true)
where$assoc
is in fact useless. I could remove them all as well.The
3rd2nd commit fixes the order of keywords in method signatures:static (public|protected|private) function
→(public|protected|private) static function
.Let me know what you think.