Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Framework coding standard compliant #7166

Closed
wants to merge 96 commits into from
Closed

Make Framework coding standard compliant #7166

wants to merge 96 commits into from

Conversation

dverkade
Copy link
Member

@dverkade dverkade commented Oct 24, 2016

Started work on the Magento Framework to become coding standard compliant.

  • Removed all instances of ignoring the coding standards in the docblock
  • Ran PHPCBF over it in order to fix issues
  • Manually fixed issues.

@dverkade
Copy link
Member Author

This is still a work in progress and I will fix all outstanding issues file by file.

- Code must not contain multiple empty lines in a row; found 2 empty lines
- Line exceeds maximum limit of 120 characters; contains 124 characters
- Missing function doc comment
- Code must not contain multiple empty lines in a row; found 2 empty lines
- Variable "value_hrs" is not in valid camel caps format
- Variable "value_min" is not in valid camel caps format
- Variable "value_sec" is not in valid camel caps format
- Code must not contain multiple empty lines in a row; found 2 empty lines
- Missing parameter name at position 1
- Variable "no_errors" is not in valid camel caps format
- Variable "dir_item" is not in valid camel caps format
- is_null must be avoided. Use strict comparison instead.
- Line exceeds maximum limit of 120 characters; contains 121 characters
- is_null must be avoided. Use strict comparison instead.
- Method name "Request::SslOffloadHeader" is not in camel caps format
…s being used by the builds"

This reverts commit 6cb43fc.
…mented out the complete sections with the @codingStandardsIgnoreStart and @codingStandardsIgnoreEnd annotations.
Travis should be changed so that static tests will be runned against PHP 7 as well. 

See for instance this build: https://travis-ci.org/magento/magento2/builds/178637640
Static tests pass against PHP 7 but fail against PHP 5.6. So there is difference between the two versions and we should build en pass static test for both PHP versions.
This is due to codesniffer bug where MySQL class name is processed as a function. So for now ignored coding standards for class definition. Could be changed later on when this bug is fixed in Codesniffer.
…ion of Magento generates "array()" so this should be the same.

- Removed extra empty line after interface generating, because and empty line is already present from the methods generating. So only one empty line is between the last method and class closure instead of two.
…:class does give you a fully qualified class name, but not with the \ at the beginning. This resultated in the class not starting from the global namespace and breaking the build.
@orlangur
Copy link
Contributor

This PR is already nearly impossible to process and it's just Framework part. Automated changes must not be mixed with some manual work.

I described viable strategy and why it won't work this way here.

@dverkade
Copy link
Member Author

Hi @orlangur what do you mean by "Automated changes must not be mixed with some manual work."

@orlangur
Copy link
Contributor

Hi @dverkade,

To minimize code review efforts changes like 24e6743 and 7d7c327 must be done automatically and in separate commits mentioning exact command performed. So that reviewer can just execute the same command and assure that output is exactly the same.

@okorshenko
Copy link
Contributor

@dverkade are you still working on this or we can close this PR?

@dverkade
Copy link
Member Author

Hi @okorshenko

Is see that many of these issues have been fixed by merging this pull request: 8685
I think it is better to close this one, since it's open to long and has to many conflicts. I will run the tests again on the develop branch and will submit a PR for every file that's need to change, so that the PR's are smaller and easier to accept.

Is that a solution that works best for you guys?

@dverkade dverkade closed this Mar 20, 2017
@orlangur
Copy link
Contributor

@dverkade seeing update in this PR I wanted to suggest you joining my efforts on making codebase standards compliant :)

Here is the detailed plan: #8823 (comment)

Let's do the 1. similar to approach I used in 8685:

  • create temporary branch
  • remove @codingStandardsIgnoreFile from *.php files with some automated tool (NOT touching *.phtml until we upgrade phpcs)
  • apply automatic fixes by phpcbf, php-cs-fixer
  • apply manual fixes all over the codebase until we have zero violations (arbitrary amount of commits here - just like you did in this PR)
  • after we have all builds green with temporary branch synced with mainline, we start to create clean branch for PR
  • first we do squashed manual commits, grouped as it seems logical to us (for example, I did c3e7df5 as separate commit to isolate such changes from other violations fixes)
  • then we add three automatic commits - annotation removal and fixes by two tools
  • and then we will probably need one more small manual commit to fix some new violations coming from develop
  • wait for PR to be processed and reapply automatic fixes as needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants