Problem/Motivation

EntityResolverManager supports arbitrary procedural controllers, let's deprecate that.

Steps to reproduce

Proposed resolution

Deprecate the fallback.
Clean up providers in tests.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3539922

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

nicxvan created an issue. See original summary.

nicxvan’s picture

Status: Active » Needs work

Some relevant test failures.

mstrelan’s picture

I think the CR needs updating to tell us where we can't use these keys. Is this in route definitions (yaml / route provider)?

berdir’s picture

> I think the CR needs updating to tell us where we can't use these keys. Is this in route definitions (yaml / route provider)?

I honestly have no idea. This is about _controller and form definitions. forms are already limited to being a class. I've never used a function as a _controller and wasn't even aware that this is supported. Maybe it isn't.

The purpose of it is essentially to automatically set upcasting information for entity parameters on routes and forms, something that in my experience works pretty badly and typically needs to be done by hand.

So essentially the CR is that something no longer works on functions, maybe never did, and it rarely works anyway.

I'm not sure we even want to do a CR here. If we're confused about it, then people viewing it will be even more confused.

berdir’s picture

So one example that causes the deprecation is this route definition in system_test.routing.yml:

system_test.always_denied:
  path: '/system-test/always-denied'
  defaults:
    _title: 'Always denied'
    _controller: 'chop'
  requirements:
    _access: 'FALSE'

so it apparently is possible to set a controller to a function, except this will never be called anyway.

The problem with the deprecation is that this runs on every single defined route. If we want to do a deprecation, we should only do it on a route with a function where we actually would do something. So we need to trigger it in \Drupal\Core\Entity\EntityResolverManager::setParametersFromReflection, when expand parameters and $controller is a function.

nicxvan’s picture

I don't get why we'd need to change where this is, we only throw it if it is a function and the function exists where we have it. How are these being called on all routes?

berdir’s picture

Because this is called from a route event subscriber that calls it on every route. See \Drupal\Core\EventSubscriber\EntityRouteAlterSubscriber::onRoutingRouteAlterSetType().

Note that this is different from the other issues. This is a fallback and it is not exclusively supporting only functions, it just also supports functions. While weird and not used in core except for one test, it's not something we have to remove.

so we have 3 options:

1. Deprecate just functions, that's fairly complex although possible as explained above.
2. Deprecate the whole functionality, As mentioned, in my experience, it rarely works, is unpredictable and hard to debug, especially when combined with title and access callbacks. Deprecation in same place, when we actually add the options, but unconditionally. This might actually be beneficial for performance, because this logic requires that on a router rebuild, we need to load and inspect any controller class/callback. On plain core, blackfire reports that this event subscriber uses about 100ms and it's more the more routes you have.
3. Won't fix this. It's not blocking anything.

IMHO, I'd either do 2 or 3. We could see just how many routes are affected in core if we trigger a deprecation for that? Many entity routes are generic and if necessary, we could add the options explicitly if not done yet?

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

nicxvan’s picture

Let's try 2 I guess then and see, this is the last thing technically required to be in .module files.

berdir’s picture

No, this is not required and it shouldn't be part of your meta issue.

This *also* supports functions, it does not only support functions.

nicxvan’s picture

Status: Needs work » Closed (won't fix)

Ok let's close this as won't fix then.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.