search

zendframework / zend-coding-standard

Zend Framework Coding Standard
BSD 3-Clause "New" or "Revised" License
35 stars 8 forks source link

[WIP] Extended Zend Framework Coding Standard #1

Closed michalbundyra closed 5 years ago

michalbundyra commented 7 years ago

Work in progress

Just want to know what you guys thinks about it.

Basically @weierophinney asked me to add just detection for unused imports. I went "a bit" more far with it and I've added much more rules. I found easy way how we can write custom rules and we don't need to copy them to Standards directory.

List what we have there now:

I would like to add also some more rules/sniffs:

Of course would be nice to have also tests for all of our custom sniffs...

So as you can see it's quite long list. Can you think about anything else what will be worth to add?

Let me know what do you think. Thanks!

/cc @weierophinney @Koopzington @xtreamwayz

geerteltink commented 7 years ago

I'm a bit worried about this. This adds a lot of restrictions. I do think that most of them are a good thing, since there is a lot of coding style inconsistency in the current code base. However it's going to be a lot of work to fix all the code in all packages. With the 75 PR's that koopz opened last night, only 19 passed the new tests. I do think that if you change the ruleset it should be done in one go.

One addition that might be useful: Add a sniff for the license header which could replace this PR and add it in one go for all packages.

Also I like to see tests so we know the sniffs are working as intended. And if some sniffs are changed in PhpCodeSniffer, we can adjust accordingly. Maybe a daily travis test can help out with this.

Koopzington commented 7 years ago

tbh i could've probably avoided most of the currently failing PRs (most of them are due to the newly introduced rule to have one space after the NOT operator) but i'm gonna work my way through them. Some repos will have to introduce BC breaking changes to incompatible method names in classes.

I like @xtreamwayz' idea for the license header sniff. It should help us getting those updates through way faster.

Most of the Sniffs seem to have the ability to automatically fix issues which would help a lot in case they're getting merged. I actually can't tell how many errors would show up if we test ZendCodingStandard.NamingConventions.ValidVariableName but due to the lack of a automatic fix for them this one could take a bit of time to get applied.

weierophinney commented 7 years ago

Some repos will have to introduce BC breaking changes to incompatible method names in classes.

Don't do this. Add exclusion rules for these files or add @codingStandardsIgnoreStart / @codingStandardsIgnoreEnd declarations around the method names.

We can also introduce new methods to which the originals proxy, but we do not want to break compatibility just to make the code pass the standards at this point.

geerteltink commented 7 years ago

Can't you mark some tests as warnings so they will be displayed, but won't fail the tests?

Koopzington commented 7 years ago

phpcs returns error code 1 once a warning shows up and causes the build to fail. You can either configure it to display warnings and return error code 1 or not display them and return error code 0.

Koopzington commented 7 years ago

I just realized something regarding that License Header Sniff: Any file that get's excluded from phpcs won't get updated.

Gamewalker commented 7 years ago

Hey, if you use this ruleset and you have annotations like:

    /**
     * @ORM\Id
     * @ORM\GeneratedValue(strategy="AUTO")
     * @ORM\Column(type="integer", nullable=false, options={"unsigned"=true})
     */
    protected $id;

phpcs tells you that 

use Doctrine\ORM\Mapping as ORM;

isn`t used. Seems like Annotations aren´t checked.

Also the following isnt working:

@ORM\Column(type="integer", tag is not allowed in member variable comment
michalbundyra commented 7 years ago

After quite long break I've back to work on this PR. 😄

Meanwhile PHP_CodeSniffer 3.0.0 has been released so I've updated it to work with latest version. Unfortunately in the latest version there are still some issues, I've created some PR to the PHP_CodeSniffer library some of them are already merged, and some are still awaiting. As far as I saw they are marked for 3.1.0 release.

I've prepared PR to 4 libraries with applied CS fixes from this branch:

In zend-code for example I've excluded couple files from running sniff: ZendCodingStandard.NamingConventions.ValidVariableName.NotCamelCaps and disabled sniff ZendCodingStandard.Commenting.FunctionComment.TypeHintMissing (because we can't add type hinting as long as we don't want to have BC Break. The same is also in zend-mvc and zend-expressive but in newest libraries we should use this sniff and probably in time we will be able to enable this sniff with next major versions of libraries.

I have couple more ideas what we can add to make our code more consistent:

What do you think? Do we need to add it? Any other ideas? I've seen great job in PR #2 and it would be nice to test it all together as it duplicate some part of this PR (like testing etc).

/cc @weierophinney @xtreamwayz

geerteltink commented 7 years ago

About the way of testing in #1 and #2. I couldn't get the official way working with PHPUnit 6 and also it doesn't give much feedback on the thrown violations. In #2 I know exactly which violation is thrown so it can't be mixed up with other unforeseen violations.

I still stand by what I said earlier that every single sniff in this file needs to carefully considered since it makes the coding style very strict.

michalbundyra commented 7 years ago

@xtreamwayz I've decided to implement testing here (almost) the same way as PHP_CodeSniffer has. I was looking at #2 and it seems that there always all sniffs run against every test assets. It's nice that we know there which violation is thrown, but as I can see in #2 we don't know in which lines and how many violations are thrown in a specific line.

Here we run ONLY one sniff against test assets and we know how many violations should be thrown in specific line, but we don't know which type. I agree, it is not the clearest way and we can have some unforeseen violations, but I think it shouldn't be a big issue, especially we test each sniff separately.

geerteltink commented 7 years ago

You have a point there.

On the other hand, if you run all sniffs on all test files you get less surprises when using real code. I mean if you run 1 sniff against specific code you won't catch any errors that might exist in other test cases for that specific sniff.

michalbundyra commented 7 years ago

Fair point. Maybe then we should have unit tests as we have here - separately for each sniff, and some integration tests which run all sniffs against some files and there we check which violations and where are thrown?

geerteltink commented 7 years ago

That's basically how I wrote the tests. I know exact which violations should be thrown. Any other violation should fail the test since it's either a bug in a sniff or the code in the test file is wrong. It would also catch any bugs or incompatible violations from phpcs changes in new versions.

... and some integration tests which run all sniffs against some files and there we check which violations and where are thrown?

Well good luck in doing that the phpcs way. You only know how many violations are thrown on each line. You don't know which violations are detected on which line. So you think the tests pass, but you actually might have bugs in your sniffs or even in your test code. And what if you add/remove sniffs? You need to figure out what violations where meant in that array so you can adjust it. That's not really an integration test. It's just writing some test code, detect the violations and adjust the numbers so the tests pass again.

michalbundyra commented 7 years ago

Fine, but again - your tests run all sniffs agains each test assets, so your tests assets have to be compatible with all other sniffs and violate only the one tested. So if we add new sniff and some assets will be not compatible with them we need change all of them. Not sure if it is clear enough. I would keep then the unit tests the way as it is in PHP_CodeSniffer. It works well there for a long while and maybe there is no point to reinvent the wheel. I think it's important to know which line and how many violations is thrown for specific sniff. And for me it wasn't just "playing with magic numbers" in tests, but I know exactly why we have these numbers there...

weierophinney commented 7 years ago

@webimpress —

return self instead of "classname" (in PHPDoc @return tag) (I saw in some libraries we have $this, ClassName, self, static (?), etc..., would be nice to have it consistent and checked by sniff)

There are some differences in semantics between them. First $this is invalid always; IIRC, neither phpdoc or PHPStorm detect it correctly, and PHP as a language certainly doesn't support it. self and static are valid (self means the same instance, static means the same instance based on late static binding); these are generally good for fluent interfaces in builder classes. ClassName should be used when we are returning a new instance (e.g., in Diactoros, where the interfaces mark instances as immutable, and with*() methods return a cloned instance with the changes requested).

As such, I'd only check to make sure that @return $this is not present, and, if so, marked as invalid. I'm not sure we can auto-fix those, though, as the developer will need to determine which of self, static, or ClassName is correct.

one space after tag (PHPDocs) or aligned values ?

One space after tags; if they wrap to a new line, indent by four spaces.

The rationale for this: when an argument type or its name changes, we have to re-align all the annotations. If we instead just do one space following, those changes only affect the one argument.

order of tags in PHPDocs (@params, @return, @throws ... ?)

Exactly that order.

That said: individual parameters and return values can be omitted if typehints are already present. (phpdoc has supported this since the most recent release, around 2.5 weeks ago.)

should we have phpcs.xml or phpcs.xml.dist in libraries?

phpcs.xml; I can't think of a valid reason for somebody to copy the dist file, make changes to the non-dist version; there's a possibility for both false-positives and false-negatives when running against CI if that happens.

froschdesign commented 7 years ago

@webimpress and @weierophinney

one space after tag (PHPDocs) or aligned values ?

Aligned values are proposed in "PSR-5: PHPDoc" (see example).

weierophinney commented 7 years ago

Aligned values are proposed in "PSR-5: PHPDoc" (see example).

No, they are not. The examples demonstrate them, but there is no text detailing alignment, other than alignment of the * character on each line of a docblock. As such, alignment of values is undefined in the spec, allowing us to choose how we may want to address them.

geerteltink commented 7 years ago

Can we add a coding-style.md file? Sort of like PSR-2 style with examples. Some of these descriptions doesn't say much and you need to dig into the code.

I wanted to say to add it to the maintainers repo, but it makes more sense to add the file here so it can be updated when a rule is changed.

weierophinney commented 7 years ago

I wanted to say to add it to the maintainers repo, but it makes more sense to add the file here so it can be updated when a rule is changed.

I agree; it also allows us to update the CONTRIBUTING.md file to point to either that or this repo for info on the CS.

smuggli commented 7 years ago

@webimpress Could you also add the native_function_invocation rule? It adds a \ in front of every PHP function (sadly there is no option for it should be done with use function). We already started using native invocation at ServiceManager. What are your thoughts on this?

michalbundyra commented 7 years ago

@smuggli It is already here: ZendCodingStandard.PHP.ImportInternalFunction, but disabled by default. We can discuss if we want enable it for all repositories or not. I've used this sniff to prepare PRs https://github.com/zendframework/zend-code/pull/121 and https://github.com/zendframework/zend-servicemanager/pull/198

smuggli commented 7 years ago

@webimpress Ohh, I missed that rule. Thanks for the quick response.

I would enable it but I am not in charge ;)

michalbundyra commented 6 years ago

Hello! Another big update ...

I have finally written sniffs to check functions comments. As we agreed on forum we require params and return tags only when type needs specification (array, iterable, |Traversable) or type hint/return type is not declared. There is also new sniff to check all throws (in tags and in method). All explicit exception throws should be declared in PHPDocs.

Types mixed/mixed[] are not allowed. These needs also better specification. The problem is if function returns array which contains integer and strings, PHP-5 draft suggest to use: (int|string)[]. Previously we did rather int[]|string[] and current sniffs accept only that form.

$this in return tag should be used as described in PSR-5 draft:

$this, the element to which this type applies is the same exact instance as the current class in the given context. As such this type is a stricter version of static as, in addition, the returned instance must not only be of the same class but also the same instance. This type is often used as return value for methods implementing the Fluent Interface design pattern.

It is not allowed to use explicit class name/parent class name - we should use self/static/$this/parent instead.

Added also some other minor sniffs like check order of tags and spaces between tags (lines and spaces between types/description).

What next? I need to create one more sniff to check class properties and require PHPDocs for them or type specification. Also PHP_CodeSniffer 3.1.0 is just behind the corner. It will resolve some small issue we currently have in one sniff, and maybe it will bring new sniff to discourage goto instruction (if not, imo we should add it to our library).

I'm going to prepare some PR with updated code sniffs:

There we will see how the library works, and what final result we get.

weierophinney commented 6 years ago

maybe it will bring new sniff to discourage goto instruction (if not, imo we should add it to our library).

As noted in a comment elsewhere, I do not want to enable such a rule by default. goto has quite a number of valid use cases (parsers/lexers in particular), and I'd prefer not to make folks go digging for CS workarounds to be able to use it when needed.

michalbundyra commented 6 years ago

@weierophinney regarding goto I've started discussion on forum Let's find out what other contributors think! 😄

froschdesign commented 6 years ago

@webimpress

I'm going to prepare some PR with updated code sniffs:

zend-code zend-expressive … There we will see how the library works, and what final result we get.

Do you have some results?

michalbundyra commented 6 years ago

@froschdesign

Do you have some results?

I wrote more about it on the forum, I need some responses there and then I can work on it again: https://discourse.zendframework.com/t/php-7-1-and-phpdocs/171/10

@weierophinney promised to have a look on it a while ago, but I know he is busy right now with ZendCon, so I hope after ZendCon I will get something on it.

@froschdesign Feel free to add your comment on the forum and vote in the poll there, it will help! Thanks!

oshdev commented 6 years ago

@webimpress in commit https://github.com/zendframework/zend-coding-standard/pull/1/commits/434c7d2cdb3ec2e60962d8d60d686ef1028b275f you changed return type declaration colon to be preceded, by a single space (...) : type). I'm wondering why as, at least from what I've seen, 0 spaces (...): type) has become somewhat a standard:

It makes me wonder, why would ZF coding standards be different to the general ones (I think coding standards for projects should be more strict if not identical to general ones, but never contrary conflicting with them).

weierophinney commented 6 years ago

@oshdev We chose the additional space before the colon before psr-12 was finalized; that particular rule is still being debated on the list, actually. If the standard finalizes without the space, we will update our ruleset.

The language allowed either syntax.

froschdesign commented 6 years ago

@weierophinney Can we bring this to a good end?

@webimpress Is something missing?

michalbundyra commented 6 years ago

@froschdesign yes, working on couple things:

froschdesign commented 6 years ago

@webimpress Okay, 3 steps and you are finished?

Do you see a chance to fix these problems:

There were 40 risky tests: 1) ZendCodingStandardTest\Sniffs\Arrays\FormatUnitTest::testSniff This test did not perform any assertions 2) …

https://travis-ci.org/zendframework/zend-coding-standard/jobs/332290017#L615

Otherwise, all tests are passed. LGTM! 👍

Xerkus commented 6 years ago

Will it be sensible to add rules to check for setUp() and tearDown() methods in tests to declare void return type? It is not declared by phpunit for BC reasons but i suppose it could change in the future, and if it will, no changes on our side will be needed

michalbundyra commented 6 years ago

@Xerkus

Will it be sensible to add rules to check for setUp() and tearDown() methods in tests to declare void return type?

I thought about it with couple other sniffs for tests only. I've checked it, and we can run some sniffs only in test/ directory.

Xerkus commented 6 years ago

Tried it on my branch of zend-router. There is a fix that I am concerned about:

-        return $pathLength === ($this->pathOffset + $this->matchedPathLength);
+        return $pathLength === $this->pathOffset + $this->matchedPathLength;

Those brackets are unnecessary, but they improve code readability.

What is really bad is that sniff will remove brackets and introduce bug by changing order of execution:

-        return $pathLength === ($this->pathOffset + $this->matchedPathLength) * 10;
+        return $pathLength === $this->pathOffset + $this->matchedPathLength * 10;
Xerkus commented 6 years ago

I am not sure how to feel about this fix. I kinda like class name more as a return type rather than self:

@@ -97,7 +99,7 @@ final class PartialRouteResult
         array $allowedMethods,
         int $pathOffset,
         int $matchedPathLength
-    ) : PartialRouteResult {
+    ) : self {
-    public function getRoute(string $name) : ?RouteInterface;
+    public function getRoute(string $name) : ?parent;
Xerkus commented 6 years ago

// edit Sorry, it was not sorting. I've read diff wrong.

Xerkus commented 6 years ago

This is definitely bad fix:

@@ -14,7 +14,7 @@ interface RouteStackInterface extends RouteInterface
     /**
      * Add a route to the stack.
      */
-    public function addRoute(string $name, RouteInterface $route, int $priority = null) : void;
+    public function addRoute(string $name, parent $route, int $priority = null) : void;
Xerkus commented 6 years ago

Bad fix for variable typehint:

         foreach ($this->routes as $name => $route) {
             /**
-             * @var $route RouteInterface
+             * @var RouteInterface
              */
             $result = $route->match($request, $pathOffset, $options);
         foreach ($this->routes as $name => $route) {
             /**
-             * @var RouteInterface $route
+             * @var RouteInterface
              */
             $result = $route->match($request, $pathOffset, $options);
weierophinney commented 6 years ago

Class name, when it's of the same class, as the return type makes extension next to impossible. While we want to likely encourage composition and remove fluent interfaces in as many locations as possible, self is sane for extension, supported by IDEs and phpdoc, and intuitive. We can't be missing different styles throughout our components, and this is the one that will result in the least need for exclusions.

On Mar 3, 2018 9:47 PM, "Aleksei Khudiakov" notifications@github.com wrote:

I am not sure how to feel about this fix. I kinda like class name more as a return type rather than self:

@@ -97,7 +99,7 @@ final class PartialRouteResult array $allowedMethods, int $pathOffset, int $matchedPathLength- ) : PartialRouteResult {+ ) : self {

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/zendframework/zend-coding-standard/pull/1#issuecomment-370199751, or mute the thread https://github.com/notifications/unsubscribe-auth/AABlV65mZekYJDLgXznrpzhB5g41U-DYks5ta2PogaJpZM4Kw2i6 .

weierophinney commented 6 years ago

No, it's not; the original was incorrect. phpdoc does not support names in annotations describing a property.

On Mar 3, 2018 10:20 PM, "Aleksei Khudiakov" notifications@github.com wrote:

Bad fix for variable typehint:

     foreach ($this->routes as $name => $route) {
         /**-             * @var $route RouteInterface+             * @var RouteInterface
          */
         $result = $route->match($request, $pathOffset, $options);

     foreach ($this->routes as $name => $route) {
         /**-             * @var RouteInterface $route+             * @var RouteInterface
          */
         $result = $route->match($request, $pathOffset, $options);

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/zendframework/zend-coding-standard/pull/1#issuecomment-370200939, or mute the thread https://github.com/notifications/unsubscribe-auth/AABlV2d9ftFq1io3TKxHXIj9i38Hbr0_ks5ta2uagaJpZM4Kw2i6 .

Xerkus commented 6 years ago

@weierophinney phpdoc does support names in annotations, and they are needed for annotation describing a variable(a variable, not property!) . IDEs are heavily relying on that, in fact (full blown like phpstorm and simpler like vim with php plugins). See https://docs.phpdoc.org/references/phpdoc/tags/var.html : @var [“Type”] [$element_name] [<description>]

I would argue that self is not good for extension, as it can't be self in extending/implementing classes. It means that on extending class or implementing interface it have to change to actual name. Visually signature is different and I really dislike that.

interface RouteInterface
{
   public static function factory(...) : self;
}

class Literal implements RouteInterface
{
    public static function factory(...) : RouteInterface
}

And in use cases that are not fluent interfaces self is not even intuitive as it does not return itself but rather a new instance/copy/decorated instance and so on: 

final class RouteResult
{
    /**
     * Produce a new route result with provided matched parameters. Can only be
     * used with successful result.
     */
    public function withMatchedParams(array $params) : RouteResult
    {
        // ...
        $result = clone $this;
        // ...
        return $result;
    }
}

I would rather forbid self in parameters and return types and work on removing fluid interfaces.

Xerkus commented 6 years ago

I would rather forbid self in parameters and return types and work on removing fluid interfaces.

I meant forbid self in parameters and return types only in interfaces.

Parameters, on the other hand, must not typehint on self/parent anywhere.

Xerkus commented 6 years ago

// snip edited out irrelevant comment. Should have checked code first

snapshotpl commented 6 years ago

Any plans to merge 1,5 year pull request? :) Need some help?

weierophinney commented 6 years ago

@snapshotpl —

Any plans to merge 1,5 year pull request? :) Need some help?

Note that @webimpress ' last commit was yesterday, and the last checkbox item was just finalized. ;-)

Also, there are a few things implemented that we have not yet completed polls for, and need to. (@webimpress can fill you in on those.)

mrVrAlex commented 6 years ago

I see what this version rules will not compatible with feature PSR-12. This is a conscious decision?

michalbundyra commented 6 years ago

@shandyDev

I see what this version rules will not compatible with feature PSR-12. This is a conscious decision?

What part is not consistent? Can you provide more details? Please bare in mind that PSR-12 is not yet approved...

mrVrAlex commented 6 years ago

Please bare in mind that PSR-12 is not yet approved...

I know, but currently it in Review status https://www.php-fig.org/psr/ where:

Unless a PSR is marked as “Accepted” it is subject to change. Draft can change drastically, but Review will only have minor changes.

What part is not consistent? Can you provide more details?

  1. Compound namespaces is allowed in PSR - https://github.com/php-fig/fig-standards/blob/master/proposed/extended-coding-style-guide.md#3-declare-statements-namespace-and-import-statements With Zend Coding Standart there will 2 errors:

    There must be one USE keyword per declaration Missing indent. Expected 4 spaces

  2. Not sure, but PSR12 have:

    When you have a return type declaration present there MUST be one space after the colon followed by the type declaration. The colon and declaration MUST be on the same line as the argument list closing parentheses with no spaces between the two characters.

weierophinney commented 6 years ago

@shandyDev

Compound namespaces is allowed in PSR

Allowed, but not required. As such, we'd still comply. (Remember, this coding standard is for the ZF repos, and for enforcing consistency within our projects; we're choosing to enforce one way to handle imports. That choice is compatible with PSR-12.)

When you have a return type declaration present there MUST be one space after the colon followed by the type declaration.

This one is currently being debated, with Larry Garfield and several others reviewing the spec suggesting that the format we use here makes more sense. Since this particular point is still being debated, we will leave it as-is, and revisit once it goes to a vote.