Magento Code not conforming to PSR-2

[apiuser]

Feature request from RMcLeod79, posted on GitHub Mar 31, 2015

Whilst the Magento documentation for Coding Standards state that PSR-2 should be used, there are multiple instances of protected properties being prefixed with an underscore i.e protected $_someProperty This does not conform to the PSR-2 Standard.

As the docs state that PSR-2 is used instances of $_ should be removed.

[apiuser]

Comment from orlangur, posted on GitHub Mar 31, 2015

Note the difference between MUST and SHOULD according to RFC 2119 ;-)

Currently Magento 2 code is completely conforming PSR-2, but not all PSR-2 recommendations, like underscore prefix for class members/methods and 120 characters line length, are fulfilled yet.

PSR-2 conformance may be easily checked by running PHP CodeSniffer against the whole code base.

I do agree with you that strict conformance to PSR-2 (so that neither errors nor warnings are reported by tools) would be better. After it is achieved, to enforce strict conformance severity of all PSR-2 rules in PHPCS shall be changed to error level.

[apiuser]

Comment from RMcLeod79, posted on GitHub Mar 31, 2015

I know that it is a case of SHOULD rather than MUST, I mainly opened this ticket so I could reference it in a pull request.

Seeing as the team have more important things on their plate than refactoring code to conform to PSR-2 I thought I could do the refactoring.

[apiuser]

Comment from orlangur, posted on GitHub Mar 31, 2015

Ok, got it.

Note that when property or method is renamed it must be added to obsolete lists: https://github.com/magento/magento2/blob/develop/dev/tests/static/testsuite/Magento/Test/Legacy/_fil... https://github.com/magento/magento2/blob/develop/dev/tests/static/testsuite/Magento/Test/Legacy/_fil...

Also, I believe it will be nearly impossible to review changes if they will be performed manually. Do you plan to implement a tool to change all occurrences at once, with built-in code analysis to avoid collisions?

[apiuser]

Comment from RMcLeod79, posted on GitHub Mar 31, 2015

Thanks for the heads up on the obsolete files! Looking at those files I'm guessing it's just a case of listing them, no need to show new name.

Yep going to write a script to do it, obviously got to take into account collisions and PHP super globals and make sure __construct and magic methods aren't replaced.

[apiuser]

Comment from choukalos, posted on GitHub Mar 31, 2015

How disruptive will this be to those building extensions / customizations on Magento 2?

[apiuser]

Comment from RMcLeod79, posted on GitHub Mar 31, 2015

Potentially quite disruptive as it changes the name of all class properties and methods that have an underscore prefix.

I am writing a script that will go through code, change properties and methods and check that there are no clashes with properties. Which could be used by 3rd party extensions to check their code.

If it is felt that this is too big a risk, especially to 3rd party developers I'm happy to leave it and close this issue, as it is only a nice to have.

[apiuser]

Comment from alankent, posted on GitHub Mar 31, 2015

It is hard to judge the impact - like will we do it. I think it will be a matter of having a script then finding a time when the least work going on to reduce merge conflict pain. But, personally, it would be WONDERFUL to get this cleaned up before GA. If its not too much effort it would be great to have this script in the wings we so can run it at an appropriate time. It would also be good to make it runnable by extension developers at the same time (in case they access any similar names). Then when we did the big push we can tell extension developers to run the script on their code base as well. Is there passion to clean up the code? YES. But the pragmatic side does kick in as well and wanted to be open about the risk side as well. Also repeating, we want the script (and a demo showing a converted branch passes tests). We don't want a pull request with just the result of the script. Extension developers want the script too.