Use CallableResolver for form callbacks

Yep a lot of container not initialized yet failures.

Added #3537261: Move file form hooks to FileFormHooks class for the hooks cleanup mentioned above.

Fixed the failing test by handling the exception. Not sure if this is the right place to handle it.

#3537261: Move file form hooks to FileFormHooks class landed.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased on 11.x

This needs a fleshed out issue summary and CR. I'll work on the CR.

I tested this in a few other places and it works so I think the code is good to go.

Edit: CR needs work and some more examples I think.

Spent some time thinking about it and pulled an example from this MR and clarified a bit.

It's possible that it needs some additional examples, but I'm not sure that will make it clearer.

I think this would be good to get in sooner so we can show examples.

With OOP hooks I'm seeing more and more people including myself inadvertently add [$this, 'callback']

I spent a couple of hours looking at how we could use CallableResolver and avoid calling it in FormState.

I deprecated \Drupal\Core\Form\FormStateInterface::prepareCallback() and looked at where it was being called from.

Obvously a lot of test fails 😬 but could be a good first step.

I think a lot of the errors are because we no longer have the context of the FormState instance.

OK. I think this is ready for reviews again.

I went down the path of trying to add CallableResolver to entity form constructor and every sub-class constructor. I think that would be very disruptive. All the unit tests would need to be updated etc. I ended up with checking if the container was available.

Not ideal that Entity form calls a service as it should be a value object like other forms.

Squashed commits and rebased on 11.x

I think this is ready now, the feed back looks like it's all been addressed.

Took another pass through and didn't see anything else that stood out.

> Using $this as a pointer for form callback [$this, 'someMethod'] is an antipattern and should be avoided.

Instead of just saying it's bad, we just not allow it and throw an exception for it?

What's the reason it's bad though?

We could just disallow an object in that notation that isn't a Form, but I see plugins do it:

      $form['expose_button']['button'] = [
        '#limit_validation_errors' => [],
        '#type' => 'submit',
        '#value' => $this->t('Expose sort'),
        '#submit' => [[$this, 'displayExposedForm']],
      ];

We missed the 11.3.0 window so I updated the deprecation messages to 11.4.0

Rebased and bumped back down to an 11.3 deprecation since @longwave indicated we might be able to get this in in the next 12 hours.

Keeping an eye on tests.

Yeah I think this is a nice feature to have for 11.3.x.

Committed and pushed d3a9ff853ca to 11.x and 1f5b3ee85cc to 11.3.x. Thanks!

Woo! 🎉

Updated the CR to add more detailed explanation.

   * Checks if the service container is available and uses the callable
   * resolver service to get the callable from the definition. Otherwise, it
   * returns the definition as is.

The CR should say that there's this special case too.

Updated CR

Fyi I opened #3574721: FormBuilder service order causes failure, when the deprecation for this is removed then a non required service is followed by a required

This causes some confusing errors where forms might've been referencing non-public methods for a callback. With a protected/private callable now being passed outside and tested with an `is_callable()` by the `CallableResolver`:

- the `is_callable()` test fails: https://git.drupalcode.org/project/drupal/-/blob/11.3.x/core/lib/Drupal/... , as you are outside of a scope where the method is visible.
- building the exception message fails due to trying to implode the form instance and name of the method: https://git.drupalcode.org/project/drupal/-/blob/11.3.x/core/lib/Drupal/...

Uncaught PHP Exception Error: "Object of class {class name} could not be converted to string" at /var/www/html/web/core/lib/Drupal/Core/Utility/CallableResolver.php line 69

Might it make sense to short-circuit earlier, with an `is_callable()` check from inside the class, before calling out for external resolvers, in https://git.drupalcode.org/project/drupal/-/blob/11.3.x/core/lib/Drupal/... ?

@berdir

Yeah, it's breaking something that used to work.

> the methods were always invoked externally

Not quite true; though, might just be in `EntityForm::buildEntity()` that it is possible, and I just happened to run into it. Per the diff https://git.drupalcode.org/project/drupal/-/commit/1f5b3ee85cc272e9f2115... , the `call_user_func_array([...])` still has access to private/protected methods of the containing class (so, `EntityForm` or any descendants). The move to call with `$callable([...])` should still work, but because it now first tries to pass the callable out to the `CallableResolver` which then doesn't have permission to call the callable, we get that error (from #38) bubbling out of the `CallableResolver` when trying to throw the exception about an array not being a valid callable.

As for code reproducing, anything extending `EntityForm`, with a callback in `#entity_builders` that points at a protected/private method.

Yeah, just an early-returning is_callable() seems most simple. Threw together https://www.drupal.org/project/drupal/issues/3578661 with an accompanying MR making it so.

Thanks!