djoos / Symfony-coding-standard

Development repository for the Symfony coding standard
MIT License
402 stars 102 forks source link

Abstract method declarations cause Symfony.Functions.ScopeOrder.Invalid failures #191

Open vpassapera opened 3 years ago

vpassapera commented 3 years ago

Given a class like:

<?php
/*
 *  This file is part of TCGTop8.
 *
 *  (c) Victor Passapera <vpassapera at outlook.com>
 *
 *  For the full copyright and license information, please view the LICENSE
 *  file that was distributed with this source code.
 */

namespace App\Reader;

use App\Client\RssClientInterface;
use App\Exception\NotValidGameException;
use App\Message\FetchedFeedItemMessage;
use App\Model\MtgFormat;
use Generator;
use Laminas\Feed\Reader\Exception\RuntimeException;
use Laminas\Feed\Reader\Feed\FeedInterface;
use Psr\Log\LoggerInterface;
use Stringy\Stringy;

/**
 * Class AbstractRssReader.
 */
abstract class AbstractRssReader implements VendorReaderInterface
{
    /**
     * @var LoggerInterface
     */
    private LoggerInterface $logger;

    /**
     * @var RssClientInterface
     */
    private RssClientInterface $client;

    /**
     * @return string
     */
    abstract public static function getFeedAddress(): string;

    /**
     * @param FeedInterface $item
     *
     * @return FetchedFeedItemMessage
     */
    abstract protected function getNewFeedItemMessage(FeedInterface $item): FetchedFeedItemMessage;

    /**
     * @param RssClientInterface $client
     * @param LoggerInterface    $logger
     */
    public function __construct(RssClientInterface $client, LoggerInterface $logger)
    {
        $this->client = $client;
        $this->logger = $logger;
    }

    /**
     * @return Generator|FetchedFeedItemMessage[]
     */
    public function getFeedItems(): Generator | iterable
    {
        try {
            $articles = $this->client->import(uri: static::getFeedAddress());
            foreach ($articles as $feedItem) {
                try {
                    yield $this->getNewFeedItemMessage($feedItem);
                } catch (NotValidGameException $e) {
                    $this->logger->info(
                        message: "Skipped '{$feedItem->getTitle()}' as unrelated to {$e->getTargetGame()}"
                    );
                }
            }
        } catch (RuntimeException $e) {
            $this->logger->critical(
                message: sprintf('Feed %s could not be imported.', static::getFeedAddress()),
                context: [
                    'exception' => $e->getMessage(),
                ]
            );
        }
    }

    /**
     * @param string $title
     * @param string $content
     *
     * @return string[]
     */
    protected function getFormats(string $title, string $content): array
    {
        $title = Stringy::create($title);
        $content = Stringy::create($content);
        $formats = array_filter(
            array: MtgFormat::toArray(),
            callback: function (string $format) use ($title, $content) {
                return $title->contains(needle: $format, caseSensitive: false) ||
                    $content->contains(needle: $format, caseSensitive: false);
            }
        );

        foreach (static::getSpecialFormats() as $key => $value) {
            if ($title->contains(needle: $key, caseSensitive: false) ||
                $content->contains(needle: $key, caseSensitive: false)) {
                $formats = array_merge($formats, $value);
            }
        }

        // If no formats were found, we'll just assign all formats.
        if (empty($formats)) {
            $formats = MtgFormat::toArray();
        }

        return array_values(array_unique($formats));
    }

    /**
     * @return string[][]
     */
    private static function getSpecialFormats(): array
    {
        return [
            ...
        ];
    }
}

And running with a config such as (Some rules excluded due to incompatiblity with PHP 8):

<?xml version="1.0"?>
<ruleset name="TCGTop8 Coding Style">
    <description>Coding standard for TCGTop8</description>
    <file>src</file>
    <file>tests</file>
    <arg name="extensions" value="php" />
    <arg value="sp" />
    <rule ref="Symfony">
        <exclude name="Symfony.Functions.Arguments" />
        <exclude name="Symfony.Errors.ExceptionMessage.Invalid" />
    </rule>
    <rule ref="Symfony.Commenting.FunctionComment.Missing">
        <exclude-pattern>tests/*</exclude-pattern>
        <exclude-pattern>src/Controller/*</exclude-pattern>
    </rule>
    <rule ref="Generic.Files.LineLength">
        <properties>
            <property name="lineLimit" value="120"/>
            <property name="absoluteLineLimit" value="120"/>
        </properties>
        <exclude-pattern>src/DataFixtures/*</exclude-pattern>
    </rule>
</ruleset>

It does not make sense to trigger a failure for the method scopes, but it happens:

FILE: /shared/httpd/tcgtop8-api/src/Reader/AbstractRssReader.php
-------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------------------------
 63 | ERROR | Declare public methods first,then protected ones and finally private ones
    |       | (Symfony.Functions.ScopeOrder.Invalid)
-------------------------------------------------------------------------------------------------------------------------------

https://www.php-fig.org/psr/psr-2/

PSR 2 shows examples of this within the code blocks provided (not explicitly), where the abstract method declarations occur before any of the concrete method declarations.

PSR 12 does not state against this.

The scoping rules should still apply, but abstract methods are defined on top of concrete methods, following the scope rules, then concrete methods, following the scope rules. At least, that's the expectation.

wickedOne commented 3 years ago

Declare public methods first, then protected ones and finally private ones. The exceptions to this rule are the class constructor and the setUp() and tearDown() methods of PHPUnit tests, which must always be the first methods to increase readability; https://symfony.com/doc/current/contributing/code/standards.html#structure

although exceptions made, none of them for abstract functions.... so basicaly the result is as intended