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:

About issue forks

Comments

berdir created an issue. See original summary.

berdir opened merge request !16776

berdir opened merge request !16784

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
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

nicxvan commented

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

berdir
Primary language German
Location Switzerland
commented

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.