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

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

Open
vpassapera opened this issue Jun 17, 2021 · 1 comment

Comments

@vpassapera
Copy link

vpassapera commented Jun 17, 2021

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
Copy link
Contributor

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

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

No branches or pull requests

2 participants