Add initial PHPMD support and have the CI run it against src/

[JeroenDeDauw]

No description provided.

[thiemowmde]

A limit of "under 10" methods per class? I find that extreme and not helpful. Can we please raise this instead of adding such weird suppress tags?

[JeroenDeDauw]

I was under the impression this was really about public methods and the docs just forget to mention that. I tested now and this is wrong. It's about all methods. In which case I agree with you that this is not a good rule with value 10. It discourages splitting up methods.

[thiemowmde]

-1 because of the suppress tags, they are not increasing code quality, the contrary.

When looking at the "error" messages this gives (when removing the suppress tags) I doubt this is helpful in the long run.

I clearly prefer the Scruntinizer way of showing a metric like "this patch will, if merged, reduce the overall code quality from 713 points to 709 points". It's the job of the reviewer to decide if code is readable and maintainable and mergeable. I clearly don't want patches to be blocked and effectively unmergeable because of arbitrary details like a 2-character variable name that may just make sense in certain situations.

[JeroenDeDauw]

Inline comments have been addressed.

I definitely agree we should not put tons of suppression tags in our code. I don't see how that would happen though. First, someone would need to submit the tags, which I'm certainly not going to do. At which point we can simply refuse the addition in CR.

As to reasons why we would add many such tags: I also don't see how this is a problem in practice. If a rule does not suit us, or if the code is not read for it to be enabled, we simply don't enable it. Also note how we do not need to have the same rules in all our repos. In fact, I think that'd make very little sense. The rules that are suitable in this repo will simply not work for Wikibase.git, since there we violate certain things frequently in old bad code. We'd first need to clean up that code before we could enable the rules.

[adrianheine]

There is a fix for this issue in phpmd/phpmd#198 which will be in the next PMD release 2.2.0. I'm ok with merging this. I'd like to have a comment refering to the fix and explaining why it's currently present.

[JeroenDeDauw]

Oh neat! I did not realize this was already fixed and was thinking about submitting a patch myself. When running this rule against some small chunk of fundraising code (~5kloc), we ran into this dozens of times. Which means that for most component the whole rule can currently not be enabled, which is too bad.

[thiemowmde]

Still -1, sorry.

Thank you very much for the helpful explanations (enabling rules step by step, having different rules in different repos) and thanks for dropping two of the suppress tags, but my problem with the remaining tags is still the same. They are not increasing code quality, the opposite.

Also my general problem is unanswered: Are these rules going to be errors and block patches? I clearly don't want that, in no repo. We already have and will find exceptions for almost every rule in our code base. There will always be a case where it just makes sense to stretch a rule in favor of other arguments. What's going to happen to such patches? Why should the author of code that "just makes sense"(tm) be forced to refactor it (and possibly make it less readable) just because of an arbitrary rule that was valid for the existing code but is not for the new patch? Such exceptions do not mean a rule should be disabled or metrics should be raised for a whole repo just because of a single exception.

When reviewing a patch it's very, very helpful to see something like "this patch increases code quality by x, but decreases other code's quality by y", just like Scruntinizer does it. It's my job as a coder and/or reviewer to decide if a patch is acceptable, even if it decreases code quality.

Code quality metrics should be an indicator, no dictator.

(And all this should probably an email thread, not hidden in this pull request. I will do this if it helps.)

[adrianheine]

@thiemowmde I understand your points. I think there is a base line for metrics and coding style rules which can and should be enforced. I think we should find this base line and make it voting. It would be cool to have a non-voting code quality measurement as an addition to that.

[JanZerebecki]

If one can come up with a rule that only has few exceptions over the whole code base, I think it makes sense to make it voting. Which means if that changes for some reason you need to get your change to the CI config merged first. Which is a good thing, as breaking rules that work most of the time should be a consciously decided thing, with review. Explicitly marking up a few exceptions is a better idea than disabling a useful rule I think.

We already have and will find exceptions for almost every rule in our code base.

We should discuss which enforced rules we think don't make sense instead of generally not making it voting.

I think the two exceptions in this commit show rules that should not be voting as is. My proposal what to do with them:

@SuppressWarnings(PHPMD.ExcessiveClassComplexity)

Increase the limit so this passes then possibly refactor and decrease the limit.

@SuppressWarnings(PHPMD.UnusedLocalVariable)

Is there a nice way to avoid this (in the current phpmd version)? If no, disable the rule, until we can upgrade, because it will have too many false positives.

@thiemowmde Which other rules do you think are problematic?

[thiemowmde]

breaking rules that work most of the time should be a consciously decided thing, with review.

Yes, this is exactly how it should be, as I explained above. Give me a tool that tells me how good or bad a patch is, if a patch increases or decreases the overall code quality, how much and why. (And make sure I still have access to this information after a patch was merged.) But don't install a 1-bit "this code is bad" dictatorship based solely on arbitrary numbers that may or may not be relevant for readability and maintainability.

If I'm allowed to change these numbers the moment I run into a rule that hinders me, fine. But I'm afraid we will waste much more time discussing such situations than all this will save us in the long run.

Explicitly marking up a few exceptions is a better idea than disabling a useful rule

Again, these suppress tags are 1-bit. How bad is such tagged code? What does it mean for the overall code quality? Nobody will ever know as long as that piece of code is excluded. You can either do whatever you want in excluded code or nothing. There is no "slightly stretching a rule", but that's what I want and what I find acceptable.

Increase the limit so this passes then possibly refactor and decrease the limit.

I agree, if that helps moving forward, but this doesn't solve the overall problem I described above.

disable the rule, until we can upgrade

I agree. As far as I understand this is a bug in the tool. This does not justify changing our code in an odd way. (There are cases when this is acceptable, but not if there is such an easy workaround as disabling that rule, as it is here.)

Which other rules do you think are problematic?

Where is the list of rules?

[JanZerebecki]

The config is in this patch, for the documentation see: http://phpmd.org/rules/index.html

[thiemowmde]

@JanZerebecki asked me to write down what we just discussed: If the rules are modified and the remaining suppress tags removed, I'm fine with merging this, but only here in the Diff component, as a test to see how it turns out in the longer run.

In general I don't find PHPMD useful at all and will most probably object to enabling it on larger components. When looking at the individual rules in the basic rule sets, nothing makes sense to me. All the "count stuff and block at an arbitrary limit" rules are pointless. A single place in a code base where it just makes sense to stretch one of these rules will force us to raise that limit or disable that rule for the whole code base[1], which makes the rule completely pointless for the rest of the code and especially for new code. A lot of other rules are so obvious in code reviews that it's pointless to install a tool that enforces them. And again, if there is no way of stretching a rule like the superglobals one in certain situations without disabling or changing the rule for all other cases[1], what's the point in the first place? Code quality is not a decision based on numbers. There is no break-even-point where everything below is bad and everything above is good. It's a long-tail curve.

[1]One could use suppress tags, but they are making things worse. What do they mean? That we know certain code breaks rules but we don't care? Like a TODO or FIXME? That's not what people should think. If code is ok it should not be marked. Code should be marked with tags if it's not ok, not the other way around.

[JeroenDeDauw]

If the rules are modified and the remaining suppress tags removed, I'm fine with merging this

This has been done