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

✨ Support for using attribute as a target to parameter… #75

Conversation

andrew-demb
Copy link
Collaborator

@andrew-demb andrew-demb commented Nov 18, 2024

…Deprecate using as a target to the method. Add warning about next major release changes

These changes can be released with a minor tag release (introduced new feature).

Refs: thecodingmachine/graphqlite#708 (comment)

…ng as a target to the method. Add warning about next major release changes

Refs: thecodingmachine/graphqlite#708 (comment)
@andrew-demb andrew-demb changed the title ✨ Support for using attribute as a target to parameter. Depr… ✨ Support for using attribute as a target to parameter… Nov 18, 2024
@SCIF
Copy link

SCIF commented Nov 19, 2024

Hey @andrew-demb , thanks for all the work and investigiatoins you've done in past several days! That's very appreciated! I could be wrong as I've lost the track of all the reasoning of addressing each problem so my comment could be negligible. But doctrine/annotations support was removed in v7.
So I can see two options here:
1.to delete Note 2 and @Attributes, @Annotation, @Target on Assertion class
2. to rework Assertion in order to not implement Attribute::TARGET_PARAMETER strategy at all and just rework the test in symfony-bundle to use parameter attributes

@andrew-demb
Copy link
Collaborator Author

@SCIF I was thinking that dropping doctrine/annotations was not released yet.

I need to double-check current versions of graphqlite then and will update the PR.

Thanks

@andrew-demb
Copy link
Collaborator Author

@SCIF commit with dropping doctrine/annotations is not a part of any stable release yet.

PR about removing doctrine/annotations dated 2024-04-13.

Latest stable release dated 2024-03-20.

So, I tried to make my best to simplify the upgrade path for users of this library.

@SCIF
Copy link

SCIF commented Nov 20, 2024

Yep, it was not released but committed into the master already, so anyway cleaning of attributes worth to be done and note #2 doesn't make sense to leave

@andrew-demb
Copy link
Collaborator Author

@SCIF validator-bridge library can be used without graphqlite-bundle, and it works now with 7.0.0.

We don't know what version will be used to tag changes in dev-master - 7.1 or 8.0.

So preserving compatibility with doctrine/annotations here allows to use bridge with graphqlite 7.0.0

@andrew-demb
Copy link
Collaborator Author

The last, but not least, preserving compatibility with doctrine/annotations is pretty easy for the validator bridge.

@andrew-demb
Copy link
Collaborator Author

andrew-demb commented Nov 27, 2024

@nguyenk are you OK with PR?

Merging this PR will allow devs to start upgrading to the "non-deprecated" way to work with parameter annotations with graphiql.

@andrew-demb
Copy link
Collaborator Author

@mistraloz can you help release this PR, please (and this one #76)?

Or hint at whom we can ping to ask for help.

Both PR's are backward compatible.

@andrew-demb andrew-demb merged commit 36a8b0d into thecodingmachine:master Jan 3, 2025
@andrew-demb andrew-demb deleted the deprecate-target-method-annotation branch January 3, 2025 18:20
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.

2 participants