phpat icon indicating copy to clipboard operation
phpat copied to clipboard

Plugins should not be able to referance other plugins

Open JasonEleventeen opened this issue 11 months ago • 2 comments

Question Plugins should not be able to referance other plugins

 - plugins
   - pluginA
   - pluginB
   - pluginC

How do we insure no plugin can reference any other plugin ?

JasonEleventeen avatar Mar 22 '24 02:03 JasonEleventeen

Hi. What is your definition of plugin in your question? Can you be more specific?

carlosas avatar Mar 23 '24 20:03 carlosas

Plugins in the system are modules that implement an interface but can be replaced with another implementation,

for example we have ExceptionLoggerInterface that can be implemented using Plugin\Console or Plugin\Sentry

But Plugin\Console and Plugin\Sentry should never reference each other as the namespace might not exist

here is our custom phpstan rule I'd like to replace

It blocks importing from 1 plugin into another as well as any other code referencing the plugin namespaces

<?php

namespace PHPStanCustom;

use PhpParser\Node;
use PhpParser\Node\Expr\ClassConstFetch;
use PhpParser\Node\Name\FullyQualified;
use PhpParser\Node\Stmt\Class_;
use PhpParser\Node\Stmt\UseUse;
use PHPStan\Analyser\Scope;
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use PHPStan\ShouldNotHappenException;

class PluginNameSpaceRule implements Rule
{
    private array $namespaces;

    public function __construct(array $namespaces)
    {
        $this->namespaces = $namespaces;
    }

    public function getNodeType(): string
    {
        // return all nodes
        return \PhpParser\Node::class;
    }

    public function processNode(Node $node, Scope $scope): array
    {
        $result = [];

        try {
            // NOTE: nodes are in a tree structure, we find the leaf and check the class name
            switch (get_class($node)) {
                case FullyQualified::class:
                    $className = $node->toString();
                    $result = $this->validateClassName($className, $scope, $result);
                    break;

                case Class_::class:
                    $className = $node->namespacedName->toString();
                    $result = $this->validateClassName($className, $scope, $result);
                    break;

                case ClassConstFetch::class:
                    $className = $node->class->toString();
                    $result = $this->validateClassName($className, $scope, $result);
                    break;

                case UseUse::class:
                    $className = $node->name->toString();
                    $result = $this->validateClassName($className, $scope, $result);
                    break;

                    // add --debug to phpstan to see this
                    # default:
                #
                #    print(get_class($node));
            }
        } catch (\Throwable $e) {
            #print_r($e);
        }

        return $result;
    }

    /**
     * @param string $className
     * @param Scope $scope
     * @param array $result
     * @return array
     * @throws ShouldNotHappenException
     */
    private function validateClassName(string $className, Scope $scope, array $result): array
    {
        $currentNamespace = $scope->getNamespace();

        foreach ($this->namespaces as $namespace) {
            // check current namespace and classname are within the same parent namespace
            if (str_starts_with($currentNamespace, $namespace) && str_starts_with($className, $namespace)) {
                # split the class names into parts
                $namespaceParts = explode('\\', $namespace);
                $classNameParts = array_slice(explode('\\', $className), 0, count($namespaceParts) + 1);
                $currentNamespaceParts = array_slice(explode('\\', $currentNamespace), 0, count($namespaceParts) + 1);

                // if this is a sibling namespace return an error
                if (implode('\\', $classNameParts) !== implode('\\', $currentNamespaceParts)) {
                    $result[] = RuleErrorBuilder::message(sprintf(
                        'Forbidden sibling import %s can not be used in namespace %s.',
                        $className,
                        $currentNamespace
                    ))->build();
                }

            // ban this import from being used in other namespaces
            } elseif ($currentNamespace && !str_starts_with($currentNamespace, $namespace) && str_starts_with($className, $namespace)) {
                $result[] = RuleErrorBuilder::message(sprintf(
                    'Forbidden import %s can not be used in namespace %s.',
                    $className,
                    $currentNamespace
                ))->build();
            }
        }

        return $result;
    }
}

JasonEleventeen avatar Mar 26 '24 00:03 JasonEleventeen

One of the limitations of PHPat is the inhability to add variable references between selectors in the rules.

It's not possible to create a rule like

  • Classes in the namespace App/{module} can only depend on classes in App/{module}

expecting to be extrapolated automatically with any {module} found and transformed in

  • Classes in the namespace App/ModuleOne can only depend on classes in App/ModuleOne
  • Classes in the namespace App/ModuleTwo can only depend on classes in App/ModuleTwo
  • ...

In PHPat you can only (for now) address this creating specific tests for each 'plugin'. You can also optimize it a bit using abstraction or traits.

For instance, you can parametrize the rule in an abstract test class, then have a child rule for each module:

abstract class AbstractPluginDependencyTest
{
    abstract public function getPluginName(): string;

    public function test_plugin_does_not_depend_on_others(): Rule
    {
        return PHPat::rule()
            ->classes(Selector::inNamespace('App\Plugins\'.$this->getPluginName()))
            ->shouldNotDependOn()
            ->classes(Selector::inNamespace('App\Plugins'))
            ->excluding(Selector::inNamespace('App\Plugins\'.$this->getPluginName()));
    }
}

final class PluginADependencyTest extends AbstractPluginDependencyTest
{
    public function getPluginName(): string
    {
        return 'PluginA';
    }
}

final class PluginBDependencyTest extends AbstractPluginDependencyTest
{
    public function getPluginName(): string
    {
        return 'PluginB';
    }
}

carlosas avatar Apr 15 '24 14:04 carlosas

Thanks! codes a bit cleaner/better, but we do have 80 plugins ;)

JasonEleventeen avatar Apr 16 '24 01:04 JasonEleventeen

I'll try to figure out if there is something that can be done to improve this without breaking compatibility, but for now this is the best I can offer :sweat_smile: closing the question

carlosas avatar Apr 27 '24 11:04 carlosas