?
avatar rdeutz
rdeutz
8 Feb 2017

It still takes but it already took years to update our code styles to use PHPCS2.0 (we are still on 1.5.whatever). With PHP7 and other new PHP versions new concepts are added to PHP and we need to find out how this can be integrated in our code styles. This will also take time to discuss and implement.

The PHP - Framework Interop Group. (FIG) is working on a PHP Standards Recommendation for code styles (PSR-12) https://github.com/php-fig/fig-standards/blob/master/proposed/extended-coding-style-guide.md as replacement for the old PSR-2.

So there are people already discussing these topics and how to integrate new PHP language features.

I think we should not reinvent the wheel here and just drop our own code style and use PSR-12.

Pros:

  • We could concentrate on developing a CMS rather then discuss code style issues
  • We could use a set for PHPCS3.0 and use automatic fixing for code styles
  • Developers joining us can use the well know code style and don't have to learn a new one.

Cons:

  • We drop a lot of work in the trash can
  • It is not OUR code style
avatar rdeutz rdeutz - open - 8 Feb 2017
avatar joomla-cms-bot joomla-cms-bot - change - 8 Feb 2017
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - labeled - 8 Feb 2017
avatar 810
810 - comment - 8 Feb 2017

PSR-12 will not be on codesniffer 2.0 but on 3.0, So we should skip the 2.0 version

see: squizlabs/PHP_CodeSniffer#1309

avatar rdeutz rdeutz - edited - 8 Feb 2017
avatar rdeutz
rdeutz - comment - 8 Feb 2017

@810 ah ok, updated RFC

avatar zero-24
zero-24 - comment - 8 Feb 2017

Do we really want If structures like this:

        if ($a === $b) {
            bar();
        } elseif ($a > $b) {
            $foo->bar($arg1);
        } else {
            BazClass::bar($arg2, $arg3);
        }

This looks unreadable for me. I need to look twice or more times to understand the code as everything is in one line. (maybe i'm the only one)

IIRC there is no reason in case of speed or whatever to do this, maybe someone can correct me.

And please remember that we do not allways have php devs working on our code. Please also think about the designers who using our code in ther templates etc.

Blank lines MAY be added to improve readability and to indicate related blocks of code except where explictly forbidden.

Sounds not that good for me :( So we dont be alloed add clean lines in our code. Like we currently do a clean line befor the if etc.

avatar mbabker
mbabker - comment - 8 Feb 2017

PSR-2/12 does not explicitly forbid a blank line before control structures as we currently enforce in our code, they do allow you to add that requirement if you want.

avatar yvesh
yvesh - comment - 8 Feb 2017

There are a lot of advantages in using a common code style.. And we are in need of new contributors, so this is definitely something to consider.

Personally i have no issue with reading structures like Tobias posted (Except that i really don't like any Else), but it's still possible to have blank lines before control structures.

avatar dgt41
dgt41 - comment - 8 Feb 2017

This looks unreadable for me. I need to look twice or more times to understand the code as everything is in one line. (maybe i'm the only one)

Actually this is more like javascript and IMHO that's a good thing because at some point all the web will be js... ?

avatar Bakual
Bakual - comment - 8 Feb 2017

Imho, if the codestyle can be applied automatically, then go for it. We will get used to the new style and the automatic correction is the bigger advantage here.
If we have to change the code manually to apply to the new rules, then you can trash the proposal. I don't want to go through another round of codestyle PRs. We don't even have yet the current rules enforced everywhere.

avatar mbabker
mbabker - comment - 8 Feb 2017

@zero-24 FWIW this screenshot comes out of a Symfony build I'm working on right now. It is fully PSR-2 compliant and doesn't sacrifice readability.

screen shot 2017-02-08 at 10 12 23 am

It's also pretty consistent with the JavaScript code style we're using in this same project.

screen shot 2017-02-08 at 10 13 47 am

avatar rdeutz
rdeutz - comment - 8 Feb 2017

Readability has also to do with how you code, the style is one aspect but not the big one I think.

avatar dgt41
dgt41 - comment - 8 Feb 2017

@mbabker i see vanilla becomes your favourite ice cream taste ?

avatar mbabker
mbabker - comment - 8 Feb 2017

Not really. There are a lot of functions in that file I changed from jQuery to Vanilla as an exercise to see how painful it was ?

avatar frankmayer
frankmayer - comment - 11 Feb 2017

I too, am for this proposal. Automatic codestyle fixing should of course also be implemented.

avatar photodude
photodude - comment - 15 Feb 2017

We are nearing a public alpha release of our code style using PHPCS 2 that includes autofixers for our custom sniffs. We also eliminated many of the custom sniffs that did nothing but duplicated included sniffs in PHPCS (see the new ruleset https://github.com/joomla/coding-standards/blob/phpcs-2/Joomla/ruleset.xml ).

Check out the current branch for the PHPCS2 version of the standard https://github.com/joomla/coding-standards/tree/phpcs-2

and the current list of issues to address/review prior to the public release joomla/coding-standards#143

as for PSR-2 vs PSR-12 I think we should adopt what we can as PHPCS implements PSR-12 support

avatar rdeutz
rdeutz - comment - 16 Feb 2017

@photodude I know you have spent a lot of time on updating the code style to PHPCS 2, but even if we have them it will not solve the problem that this needs constant attention and working on it. Past experiences have shown that we don't have the resources for keeping it up to date.

Using something all are using will make our life easier

avatar photodude
photodude - comment - 16 Feb 2017

@rdeutz Our custom sniffs, tab indenting messages, and odd ordering/spacing in docblocks tags are the only reasons updating to PHPCS2 has taken so long. If all we had was a custom ruleset using the standards included in PHPCS we could have already been running on phpcs2.

The same goes for moving to the future PHPCS3 release. If we didn't have custom sniffs to update due to the BC breaks we could switch to phpcs 3 on the day of release. (note: we have completed the phpcs 3.x updates which just involved namespacing the code, phpcs3.x could be used for any project with a minimum php of 5.4)

If we get rid of all of our non-typical rules that resulted in custom sniffs we would make something far simpler and make all of our lives easier. If we find something that there is something we would really like to have as a code style rule, rather than making a custom sniff, just contribute that to PHPCS and hope for inclusion.

Additionally, what about code style rules that PSR-2 and/or PSR-12 do not cover, How are we going handle those? I would suggest that we would implement just a custom ruleset and we pick from the rules included in PHPCS to cover those edge cases and eliminate all of the custom sniffs.

In anycase, there really isn't much left to do to complete the phpcs 2 version of the code standards. I feel that project needs to be released while we work towards updating our code standards for transitioning to PSR-2/PSR-12 plus any additional rules that need to be included.

avatar joomla-cms-bot joomla-cms-bot - change - 16 Feb 2017
The description was changed
avatar joomla-cms-bot joomla-cms-bot - edited - 16 Feb 2017
avatar franz-wohlkoenig franz-wohlkoenig - change - 30 Mar 2017
Category Code style Feature Request
avatar joomla-cms-bot joomla-cms-bot - change - 30 Mar 2017
The description was changed
avatar joomla-cms-bot joomla-cms-bot - edited - 30 Mar 2017
avatar franz-wohlkoenig franz-wohlkoenig - change - 5 Apr 2017
Status New Needs Review
avatar brianteeman
brianteeman - comment - 22 Dec 2017

Does this need to remain open?

avatar rdeutz
rdeutz - comment - 22 Dec 2017

All arguments are shared. The fact that it is now 10 months later and we still running our code style checks on 1.5 can be some kind of sign that the basic idea was not so bad.

avatar photodude
photodude - comment - 22 Dec 2017

Just a note on 2.x ruleset progress.
The framework is getting close to having all repos running on the 2.x ruleset.
I have a custom CMS 2.x ruleset nearing completion.

As mentioned above I'm all for eliminating the custom sniffs; that is part of the heavy lifting is that has held back updating to PHPCS 2.x. The transition to PHPCS 3.x for J4 will be much faster as the namespacing has already been done.

avatar brianteeman
brianteeman - comment - 22 Dec 2017

so can i close this RFC

avatar rdeutz rdeutz - change - 22 Dec 2017
Status Needs Review Closed
Closed_Date 0000-00-00 00:00:00 2017-12-22 21:58:51
Closed_By rdeutz
avatar rdeutz rdeutz - close - 22 Dec 2017
avatar rdeutz
rdeutz - comment - 22 Dec 2017

I will add it to the next production team meeting agenda, for now we can close it

avatar brianteeman
brianteeman - comment - 22 Dec 2017

thanks

avatar photodude
photodude - comment - 23 Dec 2017

I think it's important to mention that PHPCS does not currently support PSR-12.
They do support most of PSR-2 (if not the whole standard).

Add a Comment

Login with GitHub to post a comment