Change: Remove __constructor() from RuleSetDescriptionInterface

[niklam]

Constructor should be defined by the implementation, not by the interface.

With this change one could do, which is not possible at the moment due to the interface defining constructor with no parameters.

class MyDynamicRules implements RuleSetDescriptionInterface {
    protected bool $risky;

    public function __constructor(bool $risky = false) {
        $this->risky = $risky;
    }
    ...
    public function getRules(): array {
        // return rules based on $this->risky     
    }
}

This would allow doing something like

$myRules = new MyDynamicRules(true);
$config->setRules(array_merge(
    $myRules->getRules(),
    [ /* more rules, override previously set rules */ ]
);

[SpacePossum]

The issue would be that public function __constructor(bool $risky) { would now also be allowed, i.e. the interface no longer enforces that the constructor has no required parameters.

This might be an issue when relying on a construct like $set = new $class();, which is currently guaranteed to work, but after the interface change no longer. As such i think this is a BC break I think we not really need.

I know it is an example, but I think the fact if a set is risky or not might be better set in the set itself (either derived from the rules it is has configured or hardcoded), than passed along.

Thanks for sharing your thoughts! Would like to hear from others on this as well 👍

[julienfalque]

The interface is @internal so we can change it. Maybe we can find another way to build ruleset instances so that we don't need to enforce the signature of the constructor?

[niklam]

I have to admit I didn't consider the interface to be marked as @internal. For the default rulesets this issue could likely be solved by requiring them to extend AbstractRuleSetDescription (which I believe all of them do), and defining the empty constructor AbstractRuleSetDescription::__construct() as final, if that's seen as a requirement.

I would suggest making the interface public, so IDEs will not complain when using it outside the main project. This requirement is related to my other PR, #6083. Tho, as getSetDefinitions() is not checking any class compatibility, we could just as well define a new public interface.

[SpacePossum]

Making the interface public will indeed very likely happen if any of the two PR's go.
I'm not in favor of changing the type (hinting) from the interface to the abstract implementation or adding yet another interface.

[niklam]

One more option is to have one public interface, and have an internal interface extending the public one with constructor defined?

To me RuleSetInterface would be very nice and descriptive, but this is again matter of taste.

[SpacePossum]

Thank you @niklam.

[SpacePossum]

As it is internal we already cover the constructor behavior in the utest so don't need it on the interface 👍

[coveralls]

Coverage remained the same at 92.81% when pulling 1de299c on niklam:change/remove-constructor-from-interface into 9473b38 on FriendsOfPHP:master.

[niklam]

Thanks, @SpacePossum! What do you see as the best way to make this interface public? Do you think we could just make this one public, or should there be one as descendant/ancestor of this interface?

[SpacePossum]

I suspect it will be RuleSetDescriptionInterface itself that will become public, but lets discuss in the PR :)