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

[SCIF]

Thanks to @andrew-demb for the work done! 👍 @oojacoboo , @oprypkhantc I assume nobody here wants to maintain useless code. The reason why @andrew-demb has raised this issue is that he was working on adding support for Symfomy 7 to the graphqlite adapter. I believe the intention of everybody is for good but what I can see here is just some miscommunication and misunderstanding. I don't think @andrew-demb likes to pitch committing deprecated code to repo. So it looks like the whole discussion would be much shorter or even would not exist if there was a clear BC breaking/versioning strategy explained on README/contribution guide. Based on the review of code @andrew-demb, he assumes the server strategy applied, but most likely the adapter versioning is tried to be aligned with symfony versions or so. Let's discuss and develop versioning strategy and it can heavily influence the whole purpose of thecodingmachine/graphqlite-symfony-validator-bridge#75

[enricobono]

I guess using semver would make lots of developer's lives much easier. So I'd go for for the solution proposed by @andrew-demb in thecodingmachine/graphqlite-symfony-validator-bridge#75

[oprypkhantc]

@oojacoboo What do you think of setting up an automatic release action using https://github.com/semantic-release/semantic-release?

It automatically releases new versions, taking into account types of commits. Fixes and performance commits are patch versions, features are minor versions and anything with a BREAKING CHANGE message in the commit description are major versions. It also keeps the CHANGELOG file up-to-date, and creates a GitHub release. So aside from merging PRs, you wouldn't have to do anything and versions would me released immediately after merging a PR.

[oojacoboo]

@oprypkhantc I think this is a good idea. Releases for GraphQLite are a bit tedious with the website versioning and the release changelog. At the moment, that's all required to be done manually. If you (or someone else) want to set that up, that'd be awesome. We could give it a shot on this release. It sounds like everyone wants to go with 8.0 on this release due to the removal of Doctrine annotations. We'd need to update the README with PR/commit message requirements.

[andrew-demb]

@oojacoboo what target branch should be used to prepare the 7.1.0 (added deprecations and upgrade path)?

Is this branch (aka 7.x) should be created from v7.0.0 (19458fa)?

Or should we push all to the master, and it will be tagged as 8.0.0 without adding any new 7x.x versions?

[oojacoboo]

@andrew-demb we're pushing to master and targeting 8.0.0. There won't be a 7.1 release.

[Lappihuan]

@oojacoboo Is the website the only blocker for the release?
If so, I suggest prioritizing the release and addressing the website later, perhaps with an alternative solution.

@andrew-demb Thank you for all the effort in keeping the sublibs up to date!

[andrew-demb]

PR about reverting BC break for parameter annotations merged - this issue is resolved.