Suitecrm: Standardise Format Changes (Noticing changes for no reason/benefit)

[eggsurplus]

Noticing a trend as I check changes from version to version. Code styles/preferences are overriding code that has been there for many years and is dependent on the whim of whomever is editing a specific file. Take for example /data/SugarBean.php with the recent changes in the 7.8.0 release:

https://www.dropbox.com/s/ywqm69t5u6om8xc/Screenshot%202017-02-15%2016.11.57.png?dl=0

Here the valid entry point check is moved to be low the header comment and the parenthesis are removed from the require_once.

Another case:

https://www.dropbox.com/s/moftokujzvyq2ur/Screenshot%202017-02-15%2016.17.35.png?dl=0

The == true is removed which makes it harder to read.

There are many more like these.

While these changes are not syntactically incorrect, they add no value and in many cases goes against best practices. While it is recommended to have a styling guide, it shouldn't be enforced randomly. I suggest erring to the side of history within the project and only changing toward one's personal preferences in new code moving forward, if at all. Adopting a chameleon method to coding to blend in with past practices may be best until all code is under a unified coding standard.

[eggsurplus]

The biggest issue is that there is no consistency to the changes. While it may change one way in a file, a completely differently file will make a change in the exact opposite direction. A styleguide for the SuiteCRM project could help unify the code base. There are many benefits to this.

[chris001]

I agree @eggsurplus , using php-cs-fixer mentioned in #942 automatically formats the inconsistently formatted code. Travis-CI should be setup to call it right here on github every time a PHP file has changes merged.

[pribeiro42]

@chris001 Please take notice that PSR-2 style conflicts with SuiteCRM Coding Standards on:

• space vs. tab question (PSR-2 mandates use of 4 spaces, SuiteCRM mandates one tab set at 4 spaces);
• opening braces for classes (PSR-2 mandates opening brace on new line, SuiteCRM mandates opening brace on same line);

It would be nice if either SuiteCRM Coding Standars aligned with PSR-2 or the php-cs-fixer could apply the SuiteCRM Coding Standards...

[samus-aran]

Hey guys.

Just would like to address your concerns with a little clarify from our side.

We, as a company, are always trying to improve our processes and more importantly the code of a legacy application – there is no denying that it is legacy. Yes the code has worked for many years, but you would also agree that even though it works it needs to be updated to meet more modern standards and technology, yeah? That is always our aim - moving forward with the times (as fast we possibly can).

One of our immediate goals is to make SuiteCRM PSR-2 complaint but as with any large project we are resource limited and so those changes are happening in increments (as you can see). We do have a strategy in place currently that will increase these file changes which you will see more in the up coming releases – but again, they will be in increments.

The reason for these increments are because we have already tested the route making it automatic – we've discovered potential issues with that and we are working our way through them. But for the time being, this will be a manual process in the next release.

In regards to the standards, @eggsurplus – the moving of the entrypoint to the bottom of the license we felt this was the best way forward – keeps the code consistently under the license (where all the code should be). May not be a standard but felt it should be in our code.

@pribeiro42

• PSR-2 mandates use of 4 spaces, SuiteCRM mandates one tab set at 4 spaces)

Yes, that is correct. Initially we felt tabs was more beneficial and could specify to use spacing when doing sub-alignment but the consensus is now to use spaces – so we'll update the coding standards to reflect this and going forward with our formatting changes.

• opening braces for classes (PSR-2 mandates opening brace on new line, SuiteCRM mandates opening brace on same line);

Agreed. We'll update the Coding Standards to remove that discrepancy.

So guys, hope we've provided you some insight – there are still many things to be done to pull SuiteCRM up to date but we do appreciate your patience and very much value your input.

[pribeiro42]

@samus-aran Thanks a lot for your comment. If I could make a suggestion: since you'll be updating the Coding Standards, please review the level of compliance to PSR-2 that you wish to implement, so that it is easy to define a ruleset for automated tools like PHP Code Sniffer and the like.

Thank you once again..

[samus-aran]

Thanks @pribeiro42 - good point. One consideration we had was supplying a coding styling file publicly - but not quite sure if that would be too far of a step.

[pribeiro42]

@samus-aran I guess that would be fantastic :)

[eggsurplus]

Anything that would help make the changes more consistent going forward is a good thing. The issue that I'm seeing lately is that there is little consistency to the type of changes being made. Following something like PSR-2 would be great.

[chris001]

@samus-aran

we have already tested the route making it automatic – we've discovered potential issues with that and we are working our way through them.
Which issues?

[samus-aran]

@chris001 - Some minor things really - not all the automatic formatting works and still requires a human eye to review it (one particular case was if statements weren't correctly formatting which was a biggy for us). Had a quick look at your PR chris and we can have a discussion on that PR :)

[samus-aran]

Standardise Format Changes (Noticing changes for no reason/benefit)

[samus-aran]

We have now set up a new home for suggestions at Trello. All github issues that were labeled 'suggestion' have been moved and will be closed. Certain ones will be progressed within the new Suggestion Box and may be re-opened.

Announcement of moving Suggestions:
https://suitecrm.com/forum/suggestion-box/13691-moving-suggestions-from-github-to

New SuiteCRM Suggestion Box
https://trello.com/b/Ht7LbMqw/suitecrm-suggestion-box