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

Remove need to duplicate class name in service definition #28

Merged
merged 1 commit into from
Aug 10, 2016

Conversation

enumag
Copy link
Member

@enumag enumag commented Apr 26, 2016

This is the magic needed to simplify validator service definition.

# before
phoneValidator:
        class: Model\Validation\Validator\PhoneValidator
        tags:
            kdyby.validator.constraintValidator: Model\Validation\Validator\PhoneValidator

#after
phoneValidator:
        class: Model\Validation\Validator\PhoneValidator
        tags:
            - kdyby.validator.constraintValidator

I don't really care personally though.

Thoughts? cc @janedbal, @Majkl578

(WIP because we can't merge it without a test obviously.)

@janedbal
Copy link

Much better. This was one of the first tries when I was searching for solution.

Anyway, I would prefer solution without tag at all as mentioned here. That would lead to much better errors when you forget to register validator and the configuration would be simpliest possible:

- Model\Validation\Validator\PhoneValidator

@enumag
Copy link
Member Author

enumag commented Apr 27, 2016

@janedbal I thought about that as well but while it is possible to search by interface instead of a tag, I really prefer tagging for now. I'll add an example to documentation once this is merged or rejected though.

As I've said you can easily write your own extension to add the tag automatically:

ValidatorTagsExtension extends CompilerExtension
{
    public function beforeCompile()
    {
        $builder = $this->getContainerBuilder();
        foreach ($builder->findByType(\Symfony\Component\Validator\ConstraintValidatorInterface::class) as $service) {
            $builder->getDefinition($service)->addTag(ValidatorExtension::TAG_CONSTRAINT_VALIDATOR);
        }
    }
}

I didn't test it so it might require a few tweaks but you get the idea. ;-)

@enumag
Copy link
Member Author

enumag commented Apr 27, 2016

You're probably wondering why I like tagging better than searching by interface - because it seems obvious that interfaces are better in this case right? So I'll try to clarify that.

The problem is not just about validators but about all cases when you have more than one service of the same type in the DIC. In some cases you don't want to treat all of them equally which is what the interface approach would do. Perfect example of this is LoaderInterface in this package. This extension defines two services of such type but only one of them is tagged because all the tagged once are injected into the non-tagged LoaderChain.

In case of constraint validators there is the problem with the ExpressionValidator which requires a little fix - there is no way to do that with interfaces.

So for consistency I like to stick to tags in these cases as I don't want to combine both solutions.

@fprochazka
Copy link
Member

fprochazka commented Apr 27, 2016

As I've said you can easily write your own extension to add the tag automatically:

You can do that using decorator section in Nette

decorator:
    Symfony\Component\Validator\ConstraintValidatorInterface:
        tags: [kdyby.validator.constraintValidator]

Why are tags better: you can control what get's registered... sometimes you wanna have some service in the container, but do not register it like this

@@ -133,6 +130,7 @@ public function beforeCompile()
foreach ((array) $attributes as $name) {
$validators[$name] = (string) $service;
}
$validators[$builder->getDefinition($service)->getClass()] = (string) $service;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would pass it through ReflectionClass to make sure the name has correct casing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fprochazka Good idea, done.

@enumag enumag changed the title WIP: Remove need to duplicate class name in service definition Remove need to duplicate class name in service definition May 2, 2016
@enumag
Copy link
Member Author

enumag commented Aug 10, 2016

@fprochazka So... can we merge this?

@fprochazka fprochazka merged commit b4cddf5 into master Aug 10, 2016
@fprochazka fprochazka deleted the di-tag branch August 10, 2016 17:11
@fprochazka
Copy link
Member

@enumag sure, I wasn't aware it's ready :)

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

Successfully merging this pull request may close these issues.

4 participants