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

Command icon 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:

Comments

berdir created an issue. See original summary.

berdir’s picture

Status: Active » Needs review

I 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.

nicxvan’s picture

Yeah 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.

berdir’s picture

The 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.

joachim’s picture

This 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!

In FieldConfigInterface::setDefaultValueCallback(), a value of 'ThisClass::thisMethod' simply is a PHP callable, representing a static call to thisMethod() on the class ThisClass.

However, in the routing system, putting 'ThisClass::thisMethod' as the controller for a route will cause ControllerResolver to use the DI ClassResolver to instantiate ThisClass, and call thisMethod() on the instance:

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.

joachim’s picture

At 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.

berdir changed the visibility of the branch 3618219-better-documentation-for to hidden.

berdir’s picture

Agreed 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.

joachim’s picture

That'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?

berdir’s picture

there 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.

joachim’s picture

Component: base system » documentation

Yes, there is a documentation component! Here it is :)

berdir’s picture

Right, I forgot about that components are split in multiple groups, I only looked at the d.. in the first section.

berdir’s picture

Title: Better documentation for CallableResolver/ClassResolver, maybe deprecate support for double-colon » Explain supported callable types and use single colon for non-php callbacks for CallableResolver/ClassResolver

And an attempt at a better issue title