Php-cs-fixer: phpdoc_annotation_without_dot fixer makes case of annotation first character to lower

[sharifzadesina]

This is not mentioned and this should not happen or at least there should be an option for controlling this.

The PHP version I am using: 7.1.4

PHP CS Fixer version I am using: 2.2.1

A minimal sample of valid PHP code that is not fixed correctly:

<?php

/**
  * Test.
 *
 * @param string $param Test param.
 */
function test($param) {}

The fixed version of that code after running the fixer:

<?php

/**
  * Test.
 *
 * @param string $param test param
 */
function test($param) {}

The expected version:

<?php

/**
  * Test.
 *
 * @param string $param Test param
 */
function test($param) {}

[keradus]

This is not mentioned

it is:

phpdoc_annotation_without_dot [@Symfony]
Phpdocs annotation descriptions should not be a sentence.

---

and this should not happen

strongly disagree

---

at least there should be an option for controlling this

option of what?
please take into consideration that logic is not "remove trailing dot, lowercase first char", but "if description is a single statement - then make changes"

[sharifzadesina]

@keradus

Phpdocs annotation descriptions should not be a sentence.

It just says annotation should not be a sentence. starting with uppercase character =/= sentence.

strongly disagree

Ok, but let's take attention to the fixer name again -> phpdoc_annotation_without_dot

option of what?

Controlling making first character case to lower.

please take into consideration that logic is not "remove trailing dot, lowercase first char", but "if description is a single statement - then make changes"

Ok, but according to the name of the fixer it should just remove trailing dot if description is a single statement.

[SpacePossum]

phpdoc_annotation_without_dot
I agree the name itself may be misleading because it does not mention the casing change.

Renaming the fixer would be a BC break so we can do this only when the next major release is near, I've put it on the list of things to discuss when that time comes.

I'm not in fav. adding configuration options to this fixer. It does has one function; i.e. changing a sentence to something that is not. It is far from perfect when doing this and both this fixer and PSR5 seem to assume all text in PHPDocs will be English. This leads to all kind of issues and I don't want to add complexity to support something that is weak spec'ed in the first place (by PSR).

Therefor I'm closing this issue, so not because I don't agree with you, but because I think doing this right will be out of scope or to troublesome (or both).

[Synchro]

I've run into the same thing, and it results in another inconsistency; if a description does not end with a dot, its first letter is not converted to lower case.

I'm absolutely with @sharifzadesina - either what this rule does or how it is named is wrong. If it can't be renamed for BC reasons, then what it does should be changed to match its name, since it does more than it says.

If you want to lower-case the first char of a description (which is a pretty silly thing to do anyway), it should have a dedicated rule with an appropriate name to enforce it. That does not imply a BC break, since there isn't apparently a rule to do this already, and this one is broken. You say that you don't want to do this because of non-English compatibility, yet by arbitrarily enforcing an undocumented change, you're making it worse!

Further note:

please take into consideration that logic is not "remove trailing dot, lowercase first char", but "if description is a single statement - then make changes"

This is trying to have it both ways: you're saying that this element should not be considered a sentence, yet if it contains a ., it should be considered as containing multiple statements, i.e. sentences. Why impose arbitrary linguistic rules on content when it should only be concerned with formatting?

[keradus]

it should only be concerned with formatting

why should it? this rule is not about formatting.

For all the rest:
Please, open a PR in this case :+1:

[Synchro]

Submitting a PR is pointless if you won't agree there's anything to fix.

Everything in php-cs-fixer is about formatting - it's its entire raison d'être. It's literally not about anything else at all, certainly not language.

[keradus]

Everything in php-cs-fixer is about formatting

no, it is not. take a look at rules, they go far beyond formatting. eg, changing user functions, methods, operators.... changing annotations, removing them, add code (eg declare_strict) and so

Submitting a PR is pointless if you won't agree there's anything to fix.

if you wish to provide PR that would add new-named-fixer behaving like current fixer and deprecating + shrinking down current fixer to match name, it's not "out of the scope", go ahead ;)

[Synchro]

Apologies, that was wrong - I was considering only what's done inside docblocks.

[Synchro]

I've been investigating this further. I've found that this lower-casing rule:

1. is not specified in Symfony coding standards, even the more detailed conventions section
2. is counter to the coding standards examples provided by Symfony (on the same page)
3. is not a convention used by Symfony - I have not found a single example where it does - here's the first place I looked, and scrolling through any source file shows quite the contrary.
4. is not apparently used in public projects by Sensiolabs, whom you would have thought would abide by this if it was a rule
5. does not appear to be mentioned anywhere other than php-cs-fixer

It's really unclear where this rule comes from at all. Was it just made up by php-cs-fixer? Regardless, it works from a functional point of view, but it's not a documented part of the Symfony spec, not in common usage, nor does it make much sense, so it should not be applied by default.

I did a quick scan of the Symfony code base (just looking at @param elements):

$ noglob egrep -R '^\s+\*\s+@param\s+[a-z]+\s+\$[a-z0-9A-Z_]+\s+[A-Z]' src/Symfony/|wc -l
2556
$ noglob egrep -R '^\s+\*\s+@param\s+[a-z]+\s+\$[a-z0-9A-Z_]+\s+[a-z]' src/Symfony/|wc -l
75

There are other reports of this same issue in #2906, #2523, so evidently the behaviour is unexpected. The fix for #2906 illustrates what a pointless rule this is. How are you going to handle sentences that start with a proper name/noun across unknown languages? Similarly, there are other circumstances in which it would be inappropriate to enforce upper case: "iPhone". It should just be left alone.

I noticed the test cases only test for complete matches, not partial matches, e.g. there are no test cases that do not start with a lower-case letter but do end with a ..

PR forthcoming.

[SpacePossum]

Thanks for all the details! Looking forward to the PR :)

TBH I always struggle with this fixer and disable it on non-English projects. However one note I would like to make is that your analyse relies on Symfony standards. This project did spawn from the SF/Sensio (and their standards) but became independent. This rule is not based on SF standards but on PSR5 concept (I think).

This doesn't make it flawless, so improvements are welcome. We do keep a BC promise. So changing the behavior or name would mean you have to wait for the next major release. I already stated I would welcome such change, but if you want it sooner other ways might help you and is very possible (so doesn't require next major).

You say that you don't want to do this because of non-English compatibility

I didn't want to leave that expression; PSR5 seem to assume all text in PHPDocs will be English. is my annoyance with the fixer, but even broader, with PSR5 all together.

How are you going to handle sentences that start with a proper name/noun across unknown languages?

That is my point about PSR5 and most of the related issues/PR's about this fixer IMHO. Than again; if we want to improve, how should we? I closed this issue as I do _not_ want to try to "prove" it by adding a lot of logic to the fixer based on language, as I would rather have it limited (as is) and phased out over time, than trying to make it work with every language; as we know we can't based on current resources.

If you state Everything in php-cs-fixer is about formatting than it would imply you don't agree with the fixer on concept and therefore a PR to change it makes no sense even when perfect for all languages; it would change non-format. You can still have the PHP CS Fixer be only about formatting by picking the rules doing only that, non of the rules are enforced at any time.

To sum up; I think we are all on the same page here. What needs to be done however is to find a way forward that suits everybody. If you feel the fixer _does_ make sense in concept if tweaked than I'm happy to discuss, review etc.

[keradus]

is not specified in Symfony coding standards, even the more detailed conventions section

quoting the very first paragraph of it:

here is the golden rule: Imitate the existing Symfony code.

---

is not a convention used by Symfony - I have not found a single example where it does

https://github.com/symfony/symfony/pull/19198

If some part of Symfony is not updated to follow it, simply open a PR ;)

---

It's really unclear where this rule comes from at all. Was it just made up by php-cs-fixer?

https://github.com/symfony/symfony/pull/19198
the rule was requested by Fabien himself and groomed by few Symfony core members.

[Synchro]

My fork is here. I didn't burrow too deeply into the internal workings of the fixer, so I'm not entirely sure I'm doing it right. I copied the existing PhpdocAnnotationWithoutDotFixer class to PhpdocAnnotationWithoutDotLowercaseFixer, and then amended the existing fixer, simply removing the bit that alters case (as suggested by @keradus). I updated tests and the readme to match, and the tests pass in Travis-CI. It could use a review.

My main concern is exactly to avoid language-dependent issues, which would mean (as far as I'm concerned) that anything that's language dependent should be treated as a black box, so long as we can figure out where it begins and ends, we should avoid caring what's in it. This casing issue shows what happens if you mess with language-dependent things. Even apparently simple dot removal is fraught with language dependencies already - it already takes into account Chinese/Japanese full stop 。 characters , but neglects । (Devanagari) and ෴ (Sinhala), and there's an assumption that an ellipsis may be formed from three of the Chinese chars too (I've no idea if that's correct!). The best way to avoid issues like this is not to touch:

/**
 * Summary.
 * @param string $a <🐲 Here be dragons 🐉>
 */

If this is indeed a PSR-5 thing (which I've not looked into) and not a Symfony thing, my next question would be why is it enabled by '@Symfony' => true in a .php_cs file?

Personally I find the Symfony standard to be too restrictive and annoyingly verbose in a way that's not actually helpful (I like plain PSR-2), but that's just my opinion, and I've no objection to helping make sure it's implemented effectively.

[Synchro]

@keradus Ticket #19198 does not mention lower casing at all, only dot removal (which I have no issue with), and 98% of Symfony code has capitalised first characters of @param descriptions, so, according to "the golden rule" you cite, this issue is absolutely correct, and the lower casing should be dropped forthwith. Glad you agree :)

[keradus]

that's why I said rule was groomed later.

even today lowarcasing of first words was merged:
https://github.com/symfony/symfony/pull/24149

Glad you agree :)

please don't put words in my mouth

[Synchro]

I'm sure it will be a great step forward in introducing grammatical errors and reducing the readability of 2500+ Symfony params. Perhaps enforced spelling corrections in the wrong language next? /s

First you say that Symfony source should be taken as the definitive guide, then you say it shouldn't. Which is it?

[Synchro]

Stats for @return description text in Symfony (after that merge): 198 lower case, 1038 capitalised. Capitalised wins again.

[keradus]

Symfony did a call and going to direction they want. you know what? you don't need to use every single rule we do have implemented.

[Synchro]

I understand that - the problem is that php-cs-fixer includes something that it says is a Symfony standard when it clearly is not. Why do you consider it unreasonable for me to assume that if I request '@Symfony' => true that I should get something that equates to Symfony's standard? If not, why call it Symfony at all? What's the point? If they're not using and not documenting it, it's not a standard.

[keradus]

If this project comes from Symfony, if one of it's author is Symfony creator, if Symfony asked for given rule, is Symfony has merged multiple PRs that are increasing usage of that rule, yes, I do dare to claim that rule is be part of @Symfony ruleset ;)

(btw, be aware that there are dozens of rules that Symfony want to follow, but is not applying them for whole codebase to decrease conflicts hell with all PRs.)

[Synchro]

Maybe they should document it then (1 document, no conflicts), and stop wasting everybody's time producing PRs that they're going to break with random rule changes. I give up.