-
Notifications
You must be signed in to change notification settings - Fork 201
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
feat(SdlSchemaPluginBase): Allow altering of schemas by introducing alter events (#1301) #1302
feat(SdlSchemaPluginBase): Allow altering of schemas by introducing alter events (#1301) #1302
Conversation
I would like to receive a review about the approach and then continue with tests for it. |
Thanks, makes sense to me. We have 2 events with this approach, I think that is ok to separate schema and extensions and be able to alter them separately. I think you can go ahead now and provide some test coverage once this is sufficient for our use case. |
…dlSchemaPluginBase.php updates.
2b0e918
to
230e4b2
Compare
29baf9e
to
92674e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, makes sense! just small issues, can you clean them up?
tests/modules/graphql_alterable_schema_test/src/EventSubscriber/GraphQlSubscriber.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks! Just one more typo that we can fix after merge.
Will merge the 8.x-4.x branch for another test run with most recent changes.
Thank you! |
Thanks for sticking with us and incorporating all the feedback @akhomy ! |
My pleasure. And thank you for the excellent discussion about the right approach. |
Follow up from the issue - #1301