search

magento / magento2

Prior to making any Submission(s), you must sign an Adobe Contributor License Agreement, available here at: https://opensource.adobe.com/cla.html. All Submissions you make to Adobe Inc. and its affiliates, assigns and subsidiaries (collectively “Adobe”) are subject to the terms of the Adobe Contributor License Agreement.
http://www.magento.com
Open Software License 3.0
11.55k stars 9.32k forks source link

Magento Code not conforming to PSR-2 #1155

Closed RMcLeod79 closed 8 years ago

RMcLeod79 commented 9 years ago

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.

orlangur commented 9 years ago

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.

RMcLeod79 commented 9 years ago

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.

orlangur commented 9 years ago

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/_files/obsolete_methods.php https://github.com/magento/magento2/blob/develop/dev/tests/static/testsuite/Magento/Test/Legacy/_files/obsolete_properties.php

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?

RMcLeod79 commented 9 years ago

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.

choukalos commented 9 years ago

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

RMcLeod79 commented 9 years ago

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.

alankent commented 9 years ago

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.

piotrekkaminski commented 8 years ago

Thank you for your submission.

We recently made some changes to the way we process GitHub submissions to more quickly identify and respond to core code issues.

Feature Requests and Improvements should now be submitted to the new Magento 2 Feature Requests and Improvements forum (see details here).

We are closing this GitHub ticket and have moved your request to the new forum.