Is it intended BC break in unreleased version? Parameter annotations from method no more auto-bounded to parameters

[andrew-demb]

In the master branch there's an unreleased (yet) commit that dropped support for Doctrine annotations - 870b448.

This commit also drops [1] a feature about allowing to use "method"-bound annotations (attributes) with explicit "target" (example: [2]).

The question is - is this dropping a feature intended? Are you open to reintroducing it - to simplify an upgrade path?

[1] 870b448#diff-b4cb271cb57cae8e3c9c55d3590bab4ea0781066f699d94ac970f89f8b7e06b4L356-L364
[2] https://github.com/thecodingmachine/graphqlite-symfony-validator-bridge/blob/2f677f6dc5c81660505e6c73952661d4d98f1199/tests/Fixtures/Controllers/UserController.php#L37

---

If I add this snippet locally, the logic about auto-bound annotations (attributes) from method to parameters will work as before.

// \TheCodingMachine\GraphQLite\AnnotationReader::getParameterAnnotationsPerParameter
        $parameterAnnotationsPerParameter = [];

        $attributes = $method->getAttributes();
        foreach ($attributes as $attribute) {
            if (false === \is_a($attribute->getName(), ParameterAnnotationInterface::class, true)) {
                continue;
            }

            $parameterAnnotation = $attribute->newInstance();
            $parameterAnnotationsPerParameter[$parameterAnnotation->getTarget()][] = $parameterAnnotation;
        }

[andrew-demb]

I think this is an unnecessary BC break for v7.

Here's a PR that reintroduces the feature #709

[oojacoboo]

@andrew-demb dropping annotations wasn't a mistake and is the path forward. Is there any reason why someone cannot switch to attributes? There is even a rector that'll do it all for you...

https://getrector.com/blog/how-to-upgrade-annotations-to-attributes

Aside from that I'm not entirely sure what the issue is here. Isn't this bundle just a wrapper around the Symfony Validator lib? Maybe you can clarify the issue here. I'm not a fan of this parameter targeting when the proper way to "annotate" is to put the attribute with the param, directly. I'm also not a fan of GraphQLite jumping through hoops to accommodate bundles, especially when encouraging incorrect design practices.

[andrew-demb]

@oojacoboo this library provides a binding for symfony/validator .

I made a PR just because a stable release of thecodingmachine/graphqlite (7.0.0) already supported doctrine/annotations and "target method" parameter annotations. And there's a stable release of thecodingmachine/graphqlite-symfony-validator-bridge (7.0.0) with test, that works fine with 7.0.0 [1].
But this test fails with the current dev-master of thecodingmachine/graphqlite (because of dropped support for "target method" parameter annotations).

So I try simplifying the upgrade path and minimizing the number of BC breaks for developers that already use thecodingmachine/graphqlite-symfony-validator-bridge 7.0.0.

[1] https://github.com/thecodingmachine/graphqlite-symfony-validator-bridge/blob/v7.0.0/tests/Fixtures/Controllers/UserController.php#L37

[andrew-demb]

To be clear , my question was about "target method" parameter annotation, not "doctrine/annotation".

Thanks in advance.

[oojacoboo]

@andrew-demb I'm really not a huge fan of this. But I'll allow it this time. The bundle needs to make clear that this is deprecated and to be removed in 8.0. Can you please add phpdoc deprecation tags to your PR here as well, so it's searchable for deprecation removal? Thanks.

[oprypkhantc]

Agreed; imho target was only ever intended for annotations and not attributes, so I wouldn't consider it as a different breaking change from annotations.

[andrew-demb]

@andrew-demb I'm really not a huge fan of this. But I'll allow it this time. The bundle needs to make clear that this is deprecated and to be removed in 8.0. Can you please add phpdoc deprecation tags to your PR here as well, so it's searchable for deprecation removal? Thanks.

Here's a PR for thecodingmachine/graphqlite-symfony-validator-bridge for providing a way to use the attribute as a target to parameter and a soft deprecation of using it as a target to method.

thecodingmachine/graphqlite-symfony-validator-bridge#75