On making the Validation package not-extendable

[bartgloudemans]

Hi @henriquemoody,

Via this discussion I'm trying to make you reconsider the design decisions you've laid out here: #1280 (comment)

First a bit of context:
We're using the Respect/Validation package in our code base and we're currently moving from ^1.1 to ^2.2. This means that we now have to deal with the fundamental changes in the package where rules have become final and some attributes have become private (like AbstractWrapper->validatable). So far I have been extending rules (to override messages, or adjusting to our specific business requirements) and accessing attributes that are now private to make it work in our code. It is a tough update.

To summarize your reasoning on why the package is now much more constrained as I understand it: people misused the package, did not fully understand the scope of their own responsibility, did not understand how to deal with and anticipate breaking changes and as a result started to complain about lacking support.

I can imagine it is immensely frustrating to deal with these demands as a creator of an open source package. But please do realize that they are a minority: there are much more users of your package that properly use your package. And most of all: they are wrong. Just close those frustrating and unjust complain-tickets, and let them suffer.

I think an important realization here is: every business (like ours) has its own requirements, quirks and legacy circumstances that cannot be foreseen by a vendor creator. So when you wonder why someone needs validation rules to be extendable, and you cannot think of a reason, it doesn't mean that there are multiple legitimate businesses out there that have a valid case for extending your classes/validation rules.

I think limiting the extensibility of software to enforce proper and secure usage is a mindset for an end-product. Respect/Validation is a vendor package, lives in the context of a bigger project and thus being extendable/hackable is a valid a requirement.

Anyway, I hope this perspective adds to the discussion. Thanks for your library, it has been incredibly valuable to us.

Kind regards,
Bart

[alganet]

Hello @bartgloudemans! Thanks for using Respect\Validation 🐼

I'm the current maintainer for the package, and also one of its original authors.

I agree with you about extensibility in many aspects. However, I can't deny the improve in code quality since @henriquemoody took those decisions back in the 1.0 to 2.0 transition and I stand behind them.

It is very likely that we'll continue to make rules final and encourage the use of enveloping and wrapping in the 2.x series. Maybe there's room to create more extensibility around things such as AbstractWrapper::$validatable or extracting core private stuff into reusable traits, I will look into that for 2.3.

I'm sorry that the upgrade from 1.x to 2.x is tough. I definitly believe I can help with that, feel free to share ideas and issues you had during these migrations. The upgrade to 3.x when that comes out should be much smoother, and I'll keep your considerations in mind.

[alganet]

@bartgloudemans I was looking at https://github.com/Respondens/Validation/pull/1/files, which I imagine has some of the changes you need.

Stuff like getTemplate and buildTemplate are welcome as PRs. You might try getParam('name') instead of getName() on the exception, I don't think we set that directly anymore (the AbstractRule has it though).

For the final stuff, may I suggest using an adapter trait?

use Respect\Validation\Validatable;
use Respect\Validation\Exceptions\ValidationException;

trait ExtensibleRuleAdapter
{
    protected Validatable $rule;

    public function __construct(...$args)
    {
        $this->rule = new (static::ADAPTS)(...$args);
    }

    public function assert($input): void
    {
        $this->rule->assert($input);
    }

    public function check($input): void
    {
        $this->rule->check($input);
    }

    public function getName(): ?string
    {
        return $this->rule->getName();
    }

    public function reportError($input, array $extraParameters = []): ValidationException
    {
        return $this->rule->reportError($input, $extraParameters);
    }

    public function setName(string $name): Validatable
    {
        return $this->rule->setName($name);
    }

    public function setTemplate(string $template): Validatable
    {
        return $this->rule->setTemplate($template);
    }

    public function validate($input): bool
    {
        return $this->rule->validate($input);
    }
}

Then, you set it up in some bootstrapping or configuration file. If you're extending rules already, you probably have this somewhere:

use Respect\Validation\Factory;

Factory::setDefaultInstance(
    (new Factory())
        ->withRuleNamespace('My\\Rules\\')
        ->withExceptionNamespace('My\\Exceptions\\')
);

That will enable you to adapt rules like this:

namespace My\Rules;

class MyInt implements Validatable
{
    use ExtensibleRuleAdapter;

    const ADAPTS = \Respect\Validation\Rules\IntVal::class;
}

And finally, just call it:

v::myInt(123)->check('foo');

That's it. You can then override the methods you want. These will throw the original rule exception. If you want custom exceptions, you can copy the reportError method from AbstractRule into the trait instead of calling it on the adapted instance, and it should do the trick.

Is this something that you can try and let me know if the approach works for your case?

[bartgloudemans]

Hi @alganet, thanks for your elaborate reply, it's much appreciated!
I'm responding a bit delayed, sorry: in the middle of other things.

I see how the adapter trait gets the job done. For example in our scenario, where we want all validation messages to be translation keys that follow the convention for translation keys on our platform. Which helps integrating the validation rules in our own environment.
Though, using the adapter trait, requires me to rename all validation rules and prefix them with my... as in myInt() where renaming is a meaningless act. It doesn't add meaning, it only distinguishes the new validation rule from the old validation rule. The old validation rule is now deprecated and should not be used anymore on our platform. It leaves us with a set of rule names that perfectly express what they do boolType() but will not be used as the myBoolType() is the preferred one. A waste of namespace :)

Additional context: the validation package is part of a utility class that we're providing to other team members to work with. I would prefer to offer them an intuitive interface, with explanatory names, without certain prefixes.

Anyway, I do understand that you made your decision consciously, and you won't change it back. However I still believe that a vendor package should provide an interface as flexible as possible in order to allow any integration desired by the platform using the package. And in my eyes inheritance is the right one here.

Again: thanks for the elaborate reply!

Kind regards,
Bart

[alganet]

@bartgloudemans you can do something like this (instead of the previous Factory setup):

$monkeyPatchedFactory = new Factory();
$monkeyPatchedFactoryReflection = new \ReflectionClass(Factory::class);
$rulesNs = $monkeyPatchedFactoryReflection->getProperty('rulesNamespaces');
$exceptionsNs = $monkeyPatchedFactoryReflection->getProperty('exceptionsNamespaces');
$rulesNs->setAccessible(true);
$exceptionsNs->setAccessible(true);

// Make your namespaces match before internal Respect ones
$rulesNs->setValue($monkeyPatchedFactory, [
    'My\\Rules',
    'Respect\\Validation\\Rules'
]);
$exceptionsNs->setValue($monkeyPatchedFactory, [
    'My\\Exceptions',
    'Respect\\Validation\\Exceptions'
]);

Factory::setDefaultInstance($monkeyPatchedFactory);

This would allow you to keep a rule named BoolVal (or any other default name) and have your rule be matched before the internal respect one, avoiding the namespacing waste.

Here in this example I'm using reflection to change the Factory, but if you find this approach useful, I can change the Factory to support this use case (without the hacky Reflections) in the next release (2.3).

My previous example was also missing calling the "parent", which you will probably need. You can do it with trait aliasing:

class BoolType implements Validatable
{
    use ExtensibleRuleAdapter {
        // Import the validate method renamed
        validate as adaptedValidate;
    }

    const ADAPTS = \Respect\Validation\Rules\BoolType::class;

    // Provide my own validate method calling the renamed one
    public function validate($input): bool
    {
        echo "Hello from custom BoolType";
        return $this->adaptedValidate($input);
    }
}

I understand that extending it is much easier, but removing the final can causes problems for some library users. People might be relying on instanceof BoolVal or similar to ensure a specific rule behavior. If we allow it to be extended, someone might make an object that reports as instanceof BoolVal but with a different meaning and break something somewhere. I want to provide that kind of API protection. If a person sees an instance of a Validation rule, that person should be sure of what it does and how it behaves.

Compositing instead of inheritance is a little bit more verbose, but it can do the same things with those guarantees we want to ensure in place.

My goal here is to support your use case using our extensibility through composition, and make it as easy as possible. If the trait works for you, we might even make it official, and I believe that would be a great addition to the package!

[henriquemoody]

Actually, if you only want to overwrite the exceptions, you can do what @alganet is telling you, but you don't even need to overwrite the rule itself. The Factory will use the list of exception namespaces to build a an exception with that specific rule name (see https://github.com/Respect/Validation/blob/master/library/Factory.php#L162).

Alternatively, you could even remove the Validation exceptions on composer install, add your namespace to the Factory and voilà.