@craue opened this Pull Request on July 10th 2014 Contributor

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 2nd 1st 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 for self::json_decode($value, $assoc = true) where $assoc is in fact useless. I could remove them all as well.

The 3rd 2nd 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.

@craue commented on July 14th 2014 Contributor

I'll rebase the branch in case you want to merge it. :smirk:

@mattab commented on July 15th 2014 Member

I'll rebase the branch in case you want to merge it. :smirk:

Sounds good, we'll definitely merge it in!

@craue commented on July 15th 2014 Contributor

Outsourced the bugfix into #5835. Remainder rebased.

The changes only apply to the core folder. Should other folders (plugins, tests, but probably not libs) also be considered?
How to deal with the other unused variables I mentioned initially?

@mattab commented on July 15th 2014 Member

There are still plenty of unused variables when calling methods in the manner of naming arguments like for self::json_decode($value, $assoc = true) where $assoc is in fact useless. I could remove them all as well.

sometimes we put the variable even though it's the default value, because it may help readability. So I wouldn't change that.

The changes only apply to the core folder. Should other folders (plugins, tests, but probably not libs) also be considered?

+1, plugins/ and tests/ are also part of the core platform.

@mattab commented on July 15th 2014 Member

Thanks for pull request!

@craue commented on July 15th 2014 Contributor

There are still plenty of unused variables when calling methods in the manner of naming arguments like for self::json_decode($value, $assoc = true) where $assoc is in fact useless. I could remove them all as well.

sometimes we put the variable even though it's the default value, because it may help readability. So I wouldn't change that.

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).

This Pull Request was closed on July 15th 2014
Powered by GitHub Issue Mirror