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

Allow registering already-instantiated custom GQL directives. #12469

Draft
wants to merge 1 commit into
base: 5.x
Choose a base branch
from

Conversation

michaelrog
Copy link

@michaelrog michaelrog commented Dec 16, 2022

Currently, it's only possible to define custom GQL directives statically, by providing a class name, on which Craft expects to invoke Directive::create().

But what if we want to generate custom directives dynamically?

  • We will need the ability to register already-instantiated Directive instances in addition to static class names.
  • And, we will need to move apply() into the instance context, to involve instance-specific behavior, rather than just static logic.

@michaelrog michaelrog requested a review from a team as a code owner December 16, 2022 22:44
@michaelrog michaelrog marked this pull request as draft December 18, 2022 21:12
@brandonkelly
Copy link
Member

@michaelrog Is there a reason this was submitted as a draft PR?

@michaelrog
Copy link
Author

michaelrog commented Dec 23, 2022

Is there a reason this was submitted as a draft PR?

After I submitted the original PR, I realized there was more to the problem, so I converted it back to a draft.

Initially, I thought I could just amend the registration logic to accept already-instantiated Directives. However, since apply() is static, allowing users to construct their own instances doesn't ultimately gain us anything, because the Directive can't use instance-specific behavior when applied.

My current plan:

  • add an applyFromInstance() method to the Directive class, which by default will just pass-through to the existing apply() (so this is non-breaking)
  • swap API usages of Directive::apply() for Directive::applyFromInstance()
  • deprecate Directive::apply() in favor of using instance methods going forward.

@brandonkelly Does this sound like an acceptable shift? Or, is there a reason to prefer Directive::apply() being static in the first place?

@michaelrog
Copy link
Author

My current plan:

  • add an applyFromInstance() method to the Directive class, which by default will just pass-through to the existing apply() (so this is non-breaking)
  • swap API usages of Directive::apply() for Directive::applyFromInstance()
  • deprecate Directive::apply() in favor of using instance methods going forward.

Maybe we can actually accomplish a static->instance transition without requiring a new method name, by using __call() and __callStatic() to route to the appropriate logic based on the context.

That way, we don't have to create, and subsequently deprecate and remove, a temporary instance method.

It'd be a bit hacky. Something like:

  • Rename the current (static) Directive::apply() to a private Directive::_applyStatic(), and add a branch in __callStatic() to route to that private method if apply() is called statically.
  • Add a branch in __call() that checks to see if the Directive has an instance apply() method. If so, run it. Otherwise, call apply() statically.
  • Swap internal API usages of static Directive::apply() to use instance Directive->apply()
  • Maybe file a deprecation warning if Directive::apply() is called statically, to encourage third-party updates.

This should still be backwards-compatible:

  • The base Directive and native directives will all use the instance ->apply() going forward.
  • Third-party directives can update to using an instance method at/before the next major break.
  • Third-party directives that haven't updated will try to call the old static ::apply(), which will get handled virtually via __callStatic().

I don't love it, but I think it'd work, and might be the lowest-friction "just works" way forward...

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.

None yet

2 participants