doctrine / coding-standard Goto Github PK

such as:

• install
• usage

Currently installing doctrine/coding-standard is not possible if we are using on root project squizlabs/php_codesniffer: 4.0.x-dev

This is a release blocker for 7.0: currently we require a dev-master commit of slevomat/coding-standard. This needs to be changed to a stable release before releasing 7.0.

I think these rules are the same.

• PSR12.Properties.ConstantVisibility (Included in PSR12)
• SlevomatCodingStandard.Classes.ClassConstantVisibility

If it is correct

<rule ref="PSR12">
    <!-- checked by SlevomatCodingStandard.Classes.ClassConstantVisibility -->
    <exclude name="PSR12.Properties.ConstantVisibility"/>
</rule>

I do not believe that the coding standard should enforce the usage of union types over the nullable ones. From the original discussion in doctrine/orm#9886 (comment):

Personally, I believe arbitrary union types (e.g. getValue(sting $key): int|string|false) often reflect poor API design while nullable types are fine (e.g. findUser(int $id): ?User).

I think that enforcing a feature introduced primarily to support the existing poor design of the PHP's own API and the type system as a one-size-fits-all solution is not the right move.

[...]

IMO, union types are often poor because they make the consumer deal with a set of types instead of one by adding conditionals. Without pattern matching, it's error-prone since there's no guarantee that every consumer will handle each type from the union.

Specifically, in the PHP's standard library, union types go hand in hand with the APIs that are known as extremely error-prone, e.g. if (strpos($str, substr)) ...) or while ($value = $stmt->fetch(PDO::FETCH_COLUMN)) .... Union types are also needed to deal with array keys because there's no built-in support for maps in PHP and numeric array keys are converted to strings.

All the int|string types used in the code that fetches data from the database are because there's no support for unsigned integers and decimals in PHP. Otherwise, it would be possible to implement an API that returns a specific type.

Looking at the DBAL code, the only place I see where a union type is used deliberately is the expression builder where the methods accept strings or expressions but even there they could be split into two: one for stings, the other for expressions.

See squizlabs/PHP_CodeSniffer#3932

Since the auto fixer for this rule sets the visibility to public, this may cause constants to get more visibility then really needed.

If they aren't auto fixed, the problem becomes more 'in your face', requiring you to consider what the exact visibility is that you need.

I know that mostly this is checked by phpstan/psalm, but I think it will be good to double check this on phpcs level

Why my phpcs print :

"phpcs: ERROR: Referenced sniff "SlevomatCodingStandard.Arrays.SingleLineArrayWhitespace" does not exist Run "phpcs --help" for usage information"

my errors when configured phpcs or error in the package?

I have the following method

public function foo(Collection $collection)
{
     $collection->add($this->bar);
     $collection->add($this->bar);

     if ($collection instanceof BOOM) {
          $collection->add($this->some)
     }
}

which reports an error because I didn't use an early exit.

But changing

     if ($collection instanceof BOOM) {
          $collection->add($this->some)
     }

to

     if (! $collection instanceof BOOM) {
          return;
     }

     $collection->add($this->some)

is taking more lines and not really reducing complexity.

Also, I may change later to

public function foo(Collection $collection)
{
     $collection->add($this->bar);
     $collection->add($this->bar);

     if ($collection instanceof Boom) {
          $collection->add($this->some)
     }

     if ($collection instanceof Please) {
          $collection->add($this->thanks)
     }
}

and so on.

The slevomat Early Exit standard has some configuration available
https://github.com/slevomat/coding-standard#slevomatcodingstandardcontrolstructuresearlyexit-

I would say it's better to use some.

Functionally this is not a problem, however, if the rule should ever be removed the other one may be left in the ruleset (and cause an headache when trying to figure out why nothing was affected).

First instance:

Second instance:

Is this correct?

https://github.com/doctrine/coding-standard/pull/205/checks?check_run_id=788441821

As you can see on this job there are differences but the job is success.

Is this addressed here? https://github.com/doctrine/coding-standard/issues

I would like to start discussion on adding these rules to doctrine CS:

    <rule ref="WordPress.Arrays.ArrayDeclarationSpacing">
        <exclude name="WordPress.Arrays.ArrayDeclarationSpacing.NoSpaceAfterArrayOpener"/>
        <exclude name="WordPress.Arrays.ArrayDeclarationSpacing.NoSpaceBeforeArrayCloser"/>
        <exclude-pattern>tests/*</exclude-pattern>
    </rule>

I got this idea when checking https://github.com/doctrine/orm/pull/10126/files#diff-71c09c517db3322ed613e4cdf6cd41ea2323bf44c1677d3c249dd83da4fbeb25R29-R30

The main argument for this change is that together with SlevomatCodingStandard.Arrays.TrailingArrayComma.MissingTrailingComma which is already applied this makes for much cleaner diffs when making changes.

I also prefer:

#[DiscriminatorMap([
    'cc' => Component\ConcreteComponent::class,
    'cd' => Decorator\ConcreteDecorator::class,
])]
abstract class Component

over

#[DiscriminatorMap(['cc' => Component\ConcreteComponent::class,
        'cd' => Decorator\ConcreteDecorator::class])]
abstract class Component

WDYT?

Consider the code example:

<?php

declare(strict_types=1);

namespace Test;

class Test
{
    /**#@+
     * Error codes.
     */
    public const ERR_1 = 1;
    public const ERR_2 = 2;
    /**#@-*/

    public function doStuff() : void
    {
    }
}

If a phpDoc template (example) is followed by a method without a docblock, then the "Squiz.Commenting.FunctionComment" sniff mistakenly attributes the closing template block to the method and marks it as invalid:

$ phpcs Test.php

------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------
 14 | ERROR | There must be no blank lines after the function comment
    |       | (Squiz.Commenting.FunctionComment.SpacingAfter)
------------------------------------------------------------------------------------------------

Although this specific rule is implemented using a component from the "Squiz" standard,

As per squizlabs/PHP_CodeSniffer#2838 (comment), the "Squiz" standard enforces docblocks and doesn't support docblock templates.

I'm filing the issue here to see if this specific rule can be reimplemented using some Slevomat building blocks which are better composable by design.

Since PSR-2 has been deprecated are there any plans change the ruleset base to PSR-12 or integrate any of the new/changed features from the ruleset?

Hi... I recently opened this #199

Instead of going wild and opening one by one a PR I thought about initiating a discussion here.

I believe there are more functions that can be forbidden:

'__autoload' => null,  // no reason to use this function since composer is out there

'define' => null,
'each' => null,  // @see http://php.net/manual/en/migration72.deprecated.php
'create_function' => null, // why using this?

// debug functions
'debug_backtrace' => null,
'debug_print_backtrace' => null,
'die' => null,
'exit' => null,
'phpinfo' => null,
'print_r' => null,
'var_dump' => null,

// Use a logger lib (preferably a PSR-3 one)
'error_log' => null,

// Framework debug functions
'dd' => null,
'dump' => null,

// use an HttpClient library instead.
'curl_exec' => null,
'curl_init' => null,
'curl_multi_exec' => null,

// Use a process library.. 
'exec' => null,

// The following functions should not be allowed inside a project and they should be set by your php.ini unless if you are building a very specific library.
'ini_alter' => null,
'ini_restore' => null,
'ini_set' => null,
'date_default_timezone_set' => null,

'apache_request_headers' => null, // Exists only on Apache webservers
'apache_response_headers' => null, // Exists only on Apache webservers
'getallheaders' => null, // Is an alias of apache_request_headers()

'putenv' => null, // use $_SERVER or $_ENV variable directly // https://github.com/laravel/framework/issues/7354
'getenv' => null,

ofc users can override this on their projects but it would be good to promote these as a good practice.

Let me know WDYT.. maybe discussing all this in one issue might be an overhead and I should open smaller ones?

👋🏾 there has been a lot of new sniffs added and options tweaked. Could we maybe tag a new version? Currently, I have nothing on my mind I'd like to contribute on top what's already here in the near future.

Or is there something I can do to speed it up?

While working on applying this standard on doctrine/orm, we found with @SenseException that the following will not raise any error:

// declarations
class Foo
{
    /** @var int */
    private $bar;
    /** @var int */
    private $baz;
}
class FooBar
{
}

We would expect errors about the lack of space between property declarations
and class declarations. Does anyone know of a sniff that we could integrate to
doctrine/coding-standard for this? @kukulich maybe?

Note that in PHP 7.4, I personally would be ok with a lack of space between property declarations when they fit on one line.

Version: 7.0.2

There is a problem using intersection types for type declaration in annotations.
These are valid since PHPStorm 2018.3.

https://blog.jetbrains.com/phpstorm/2018/09/phpstorm-2018-3-eap-183-2635-12/#intersection-types

Given the following code

/**
 * @param ContainerInterface&ServiceManager $container
 */
 public function __invoke(ContainerInterface $container) : Fubar {}

Expected:
no problem

Actual:

FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 XX | ERROR | [x] Expected 4 spaces after parameter type; 0 found

Using phpcbf to fix the issue resulted in this line:

* @param ContainerInterface    &ServiceManager $container

williamdes@williamdes:/mnt/Dev/@phpmyadmin/coding-standard$ composer2 update
The "dealerdirect/phpcodesniffer-composer-installer" plugin was skipped because it requires a Plugin API version ("^1.0") that does not match your Composer installation ("2.0.0"). You may need to run composer update with the "--no-plugins" option.
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - dealerdirect/phpcodesniffer-composer-installer v0.6.2 requires composer-plugin-api ^1.0 -> found composer-plugin-api[2.0.0] but it does not match the constraint.
    - doctrine/coding-standard 8.0.0 requires dealerdirect/phpcodesniffer-composer-installer ^0.6.2 -> satisfiable by dealerdirect/phpcodesniffer-composer-installer[v0.6.2].
    - Root composer.json requires doctrine/coding-standard ^8.0 -> satisfiable by doctrine/coding-standard[8.0.0].

You are using a snapshot build of Composer 2, which some of your plugins seem to be incompatible with. Make sure you update your plugins or report an issue to them to ask them to support Composer 2. To work around this you can run Composer with --ignore-platform-req=composer-plugin-api, but this may result in broken plugins and bigger problems down the line.
williamdes@williamdes:/mnt/Dev/@phpmyadmin/coding-standard$ composer2 --version
The "dealerdirect/phpcodesniffer-composer-installer" plugin was skipped because it requires a Plugin API version ("^1.0") that does not match your Composer installation ("2.0.0"). You may need to run composer update with the "--no-plugins" option.
Composer version 2.0.0-alpha2 2020-06-24 21:36:18
williamdes@williamdes:/mnt/Dev/@phpmyadmin/coding-standard$ 

Fix: allow 0.7.0 (https://github.com/Dealerdirect/phpcodesniffer-composer-installer/releases/tag/v0.7.0)