Problem/Motivation
What is supported for callbacks by CallableResolver is quite confusing, especially for forms, because the same syntax can mean multiple things.
From my comment in #3616277: Deprecate several locale functions
I'm not sure what is clear and what is not, sorry if I'm explaining something obvious, but trying to cover all the different things we and PHP supports:
The '::method' is preferred when you are actually the form object/class that defines the form, because the code you mentioned handles it, and it calls it on the existing object that we already have, it's fast and the expected result, $this is actually the object you expect, you could change a property for example and use that in our build method (for submit/ajax callbacks). We've had that since 8.0.
Using static:class . '::method for a non-static will fall back to \Drupal\Core\DependencyInjection\ClassResolver::getInstanceFromDefinition and create a new instance of your form class. It's slower, and only works since we added support for CallableResolver to form callbacks. That is the new bit and it's mostly meant for services.
CallableResolver treats ':' and '::' as interchangeable, I don't like that as I think it adds a lot of confusing, I would have preferred if would only support a single colon.
See https://3v4l.org/lP5bh#v8.5.9 for what PHP supports on its own. :: only, static methods only. So a static method with :: will hit the is_callable() and return early in CallableResolver. If it's not static, it will get an instance of that object, either as a service name, through ::create() or just new $class() and then call the non-static method.
I think we should consider deprecating treating :: the same as :, and only support:
Class::method or array, must be a static method, handled by PHP
object:method is called on an object, must be a service, class that implements ContainerInjectionInterface or not have any constructor arguments
::method is supported only on form objects.I think that would remove some of that confusing.
Steps to reproduce
Proposed resolution
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Issue fork drupal-3618219
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3618219-docs
changes, plain diff MR !16784
- 3618219-better-documentation-for
changes, plain diff MR !16776
Comments
Comment #4
berdirI played a bit with deprecating "::" for non-static methods, but there is a lot of them, including route/permission callbacks, the hook system uses that, controllers and more. We probably don't want to do that.
The main thing to review is the other MR, 3618219-docs. There is a list of what it supports already on the class, but there's little to no explanation and the one-line code tags are hard to read both in the code and on https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Utility%2..., I hope my approach is a bit better in that regard.
Comment #5
nicxvan commentedYeah this has certainly caught me a couple of times, might be good to clean up. Not sure if documentation or the deprecation is correct though.
Comment #6
berdirThe deprecation was more an experiment to see how widespread that use is, it's probably not viable and I instead documented that : is recommended but both are supported. We could extract some of those changes in a separate issue but I think it makes more sense to focus on MR !16784 here.
I think more important for now is feedback on whether the updated docs are easier to understand as opposed to correct or not.
Comment #7
joachim commentedThis was one of the questions that took AGES to resolve when I was involved in creating CallableResolver.
The key problem, explained in more detail on this issue (#2982949: Introduce CallableResolver to help standardise the DX and error handling for callbacks across various subsystems, is that different parts of Drupal use 'ClassName::method' to mean different things!
This meant the issue was stuck for AGES, and it only got unstuck by a change (IIRC) in PHP which meant that is_callable() would tell us if 'ClassName::method' was meant to be statically called or not.
So I'm not sure we can deprecate this as things currently are -- we'd need to clean up the systems that are currently incompatible.
Comment #8
joachim commentedAt the time, IIRC I suggested either two methods, one for :: being treated as a static and one with the class being instantiated. Or maybe it was a 2nd parameter to getCallableFromDefinition() to tell it how to handle them. It would then be up to the system that uses callablles to decide how it handles a :: -- routes would do one thing, forms another.
It occurs to me now that since we've invented ':' to mean the first piece is a service, we could invent triple-colon to mean the first piece needs to be instantiated.
Comment #10
berdirAgreed that deprecating :: for non-static callbacks is tricky and has a huge impact, I'm fine not doing that. Ignore that part. I've hidden that MR, it was just an experiment.
I don't think a ::: is necessary. CallallableResolver doesn't make a difference for that. That's the responsibility of \Drupal\Core\DependencyInjection\ClassResolverInterface and it's just one API. For any "foo:bar" (or "foo::bar" when it's not already a valid callback), CallableResolver just passes "foo" to \Drupal\Core\DependencyInjection\ClassResolverInterface::getInstanceFromDefinition and expects an object from it, it doesn't care how that happens.
My proposal and https://git.drupalcode.org/project/drupal/-/merge_requests/16784 is only about docs and by default, streamline our docs to use :: only when it is a static php callback, anything that's a service or class that an be instantiated should prefer using : but we continue to support both.
Comment #11
joachim commentedThat's true, what we could do is say that ClassName:method denotes either a service, OR a class to instantiate, and that ClassName::method always means static. We'd need to deprecate and convert route controller declarations.
In the meantime, if this is just a docs issue, should its component be changed?
Comment #12
berdirthere is no documentation component in core, it's changing docblocks on services that don't really have their own component, so I think this is the right component?
> We'd need to deprecate and convert route controller declarations.
deprecating something is IMHO an optional additional step after seeing just how big the implications of that would be. First step for me is having better docs, then we can talk about enforcing it.
Comment #13
joachim commentedYes, there is a documentation component! Here it is :)
Comment #14
berdirRight, I forgot about that components are split in multiple groups, I only looked at the d.. in the first section.
Comment #15
berdirAnd an attempt at a better issue title