Skip to content
This repository was archived by the owner on Jan 21, 2020. It is now read-only.

Zend Coding Standard 2 #5

Closed
wants to merge 60 commits into from
Closed

Zend Coding Standard 2 #5

wants to merge 60 commits into from

Conversation

geerteltink
Copy link
Member

@geerteltink geerteltink commented Sep 5, 2018 •
edited
Loading

This PR adds a stricter ruleset. #1 is adding a lot of extra code and custom rules which are hard to maintain and it's so huge that it is hard to review. The huge amount of work done in #1 is not lost and is moved to webimpress/coding-standard.

The purpose of this PR is to start over and importing rules from other packages where possible.

Blocked by:

Testing

Make sure you remove the global installation after testing from your global composer.json file!!!

$ composer global config repositories.zend-coding-standard vcs git@github.com:xtreamwayz/zend-coding-standard.git
$ composer global require --dev zendframework/zend-coding-standard:dev-feature/new-rules

# For this to work, add this to your path: ~/.composer/vendor/bin
# Using `-s` prints the rules that triggered the errors so they can be reviewed easily
$ phpcs -sp --standard=ZendCodingStandard src

# Automatically fixing errors
$ phpcbf -sp --standard=ZendCodingStandard src

weierophinney
weierophinney reviewed Sep 5, 2018
View reviewed changes
Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely a step forward!

I've noted a few questions I have on some of the sniffs, as well as a few I disagree with.

src/ZendCodingStandard/ruleset.xml Outdated
is_double => is_float,
is_integer => is_int,
is_long => is_int,
is_null => null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when this fix is applied? Seems like it might break code, unless null here means null ===?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to look into this what it exactly does, but I think it is replaced with null ===.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not replace the functions. It only displays an error:

 19 | ERROR | The use of function chop() is forbidden; use rtrim()
    |       | instead
 20 | ERROR | The use of function sizeof() is forbidden; use count()
    |       | instead
 21 | ERROR | The use of function is_null() is forbidden
 23 | ERROR | The use of function settype() is forbidden
 30 | ERROR | The use of function extract() is forbidden
 31 | ERROR | The use of function compact() is forbidden

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null means there is no alternative. is_null should be replaced with null === but changing this to that value gives the following error:

21 | ERROR | The use of function is_null() is forbidden; use 'null
   |       | ==='() instead

That looks like a weird function or so.

src/ZendCodingStandard/ruleset.xml Outdated
<!-- Forbid backtick operator -->
<rule ref="Generic.PHP.BacktickOperator"/>
<!-- Forbid short open tag -->
<rule ref="Generic.PHP.DisallowShortOpenTag"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this disallow <?=? I ask, as this is valid always starting in 5.4 or 5.5, and we recommend it for templates. If it doesn't, we're good; if it does, is there a way to make an exception for <?=?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this can be used to validate templates. I've never tried it. But yes, if phpcs works with templates then it should be removed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question is whether or not it flags <?= as invalid. If it doesn't, we're fine. :)

src/ZendCodingStandard/ruleset.xml Outdated
@category,
@created,
@package,
@since,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this one considered forbidden? It's useful for noting API additions...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the reasoning behind it is that all info is in git log and the changelog. But I'll remove it since it's easy to look it up directly in the code where most developers look anyway.

src/ZendCodingStandard/ruleset.xml Outdated
<!-- Require comments with single line written as one-liners -->
<rule ref="SlevomatCodingStandard.Commenting.RequireOneLinePropertyDocComment"/>
<!-- Forbid fancy yoda conditions -->
<rule ref="SlevomatCodingStandard.ControlStructures.DisallowYodaComparison"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disagree with this one; we use (null|false|true) [!=]== <expression> all over the place as a defensive programming approach.

src/ZendCodingStandard/ruleset.xml
<!-- Require the usage of assignment operators, eg `+=`, `.=` when possible -->
<rule ref="SlevomatCodingStandard.Operators.RequireCombinedAssignmentOperator"/>
<!-- Forbid `list(...)` syntax, use [...] instead -->
<rule ref="SlevomatCodingStandard.PHP.ShortList"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one will only work for libraries on PHP 7+; for those on 5.5, we'd need to disable this rule.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to tag this release 2.0 and include it ^7.1 packages. I don't see why we need to fix code for old packages. That's a lot of extra work and commits.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough!

src/ZendCodingStandard/ruleset.xml Outdated
</properties>
</rule>
<!-- Forbid useless @var for constants -->
<rule ref="SlevomatCodingStandard.TypeHints.UselessConstantTypeHint"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with this one. The proposed phpdoc spec specifically states @var is the appropriate tag for use with constants.

src/ZendCodingStandard/ruleset.xml Outdated
<!-- Forbid `global` -->
<rule ref="Squiz.PHP.GlobalKeyword"/>
<!-- Forbid functions inside functions -->
<rule ref="Squiz.PHP.InnerFunctions"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? This is a valid way to make private functions...

froschdesign
froschdesign suggested changes Sep 5, 2018
View reviewed changes
mkdocs.yml Outdated
site_name: Zend Coding Standard
site_description: 'The coding standard ruleset for Zend Framework components.'
repo_url: 'https://github.com/zendframework/zend-coding-standard'
copyright: 'Copyright (c) 2016-2018 <a href="https://www.zend.com/">Zend Technologies USA Inc.</a>'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line can be removed, because this is not used in theme.

docs/book/index.html Outdated
@@ -0,0 +1,9 @@
<div class="container">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the entire file, the same content is generated by the theme.

froschdesign
froschdesign reviewed Sep 5, 2018
View reviewed changes
.travis.yml Outdated
apt:
packages:
- libxml2-utils
script: xmllint --noout --schema vendor/squizlabs/php_codesniffer/phpcs.xsd src/ZendCodingStandard/ruleset.xml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! 👍

weierophinney
weierophinney reviewed Sep 5, 2018
View reviewed changes
Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation proposal looks good! 👍

@geerteltink
Add references
a276b18
@geerteltink
Update changelog with latest changes
bf0d951
@geerteltink
Update intro and readme
3386b02
@geerteltink
Fix creation of double trailing commas on specific arrays
300e613
@geerteltink
PSR-12: Force Unix LF (linefeed) line ending
61f3bc5 
Rule: All PHP files MUST use the Unix LF (linefeed) line ending only.
@geerteltink
Allow global functions
ea3de20 
Caused warnings in zend-diactoros like:

`Consider putting global function "parseCookieHeader" in a static class.`
@geerteltink geerteltink changed the title Modernize zend coding standard Zend Coding Standard 2 Sep 14, 2018
@geerteltink
Align comments, statements and arrays
53a7dcd 
Doing open source while sitting on the backseat with 200 kmh on the
Autobahn :)
@geerteltink
Align array double arrows
850f481
@geerteltink
Validate fixes before running tests
5eaa798
@geerteltink
Fix single arrow indention test
41d35b9
@geerteltink
Reset array alignment and start from scratch
3974380 
Using Squiz, Slevomat, General rules or a combination of those doesn't
work and it becomes a mess really fast. The WebimpressCodingStandard
has better rules for this, however they need some improvements.
@geerteltink
Update slevomat/coding-standard requirement and add regression test for
137dc8a 
fixed issues
@geerteltink
Add test for constant aligning
61f6b4b
@geerteltink
Align double arrows in multiline arrays
5f2280d
@geerteltink
Update expected errors report
a3d45d7
@geerteltink
Require php_codesniffer 3.3.2
27b735f
@geerteltink
Update array indentation tests
95213b5
@geerteltink
Use new format to declare array properties
38a2dea
@geerteltink
Update phpcs report
7dc2f78
@tux-rampage tux-rampage mentioned this pull request Oct 16, 2018
Open
This was referenced Nov 12, 2018
Closed
Closed
geerteltink added a commit that referenced this pull request Nov 18, 2018
@geerteltink
Merge pull request #5 from xtreamwayz/feature/new-rules
d81e6c6 
Close #5
@geerteltink
Copy link
Member Author

Merged into develop

@geerteltink geerteltink deleted the feature/new-rules branch November 18, 2018 13:56
@tux-rampage tux-rampage mentioned this pull request Nov 28, 2018
Closed
34 tasks
@geerteltink geerteltink added this to the 2.0.0 milestone Jun 3, 2019
@weierophinney weierophinney mentioned this pull request Dec 31, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@froschdesign froschdesign froschdesign requested changes

@Ocramius Ocramius Ocramius left review comments

@weierophinney weierophinney weierophinney requested changes

Assignees
No one assigned
Labels
documentation needed enhancement WIP
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@geerteltink @weierophinney @froschdesign @Ocramius