Provide a generic class resolver

I like it!
I rounded out the usage to show it off better.

Nice change!

No interdiff, but the change was swapping

if (in_array('Drupal\Core\DependencyInjection\ContainerInjectionInterface', class_implements($class))) {
for
if (is_subclass_of($class, 'Drupal\Core\DependencyInjection\ContainerInjectionInterface')) {

1. +++ b/core/lib/Drupal/Core/Controller/ControllerResolver.php
   @@ -150,6 +141,23 @@ protected function createController($controller) {
   +  public function getInstanceFromClassName($class) {
   

   Shouldn't this be a static method?

   It doesn't appear to use or touch the state of the instantiated class in any way, it's just an I/O utility operation?

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -129,8 +137,10 @@ class EntityManager extends PluginManagerBase implements EntityManagerInterface
   -  public function __construct(\Traversable $namespaces, ContainerInterface $container, ModuleHandlerInterface $module_handler, CacheBackendInterface $cache, LanguageManager $language_manager, TranslationInterface $translation_manager) {
   +  public function __construct(\Traversable $namespaces, ContainerInterface $container, ModuleHandlerInterface $module_handler, CacheBackendInterface $cache, LanguageManager $language_manager, TranslationInterface $translation_manager, ControllerResolverInterface $controller_resolver) {
   
   @@ -138,6 +148,7 @@ public function __construct(\Traversable $namespaces, ContainerInterface $contai
   +    $this->controllerResolver = $controller_resolver;
   

   Injecting an entire ControllerResolver instance just to be able to figure out how to instantiate certain classes based on a hard-coded pattern is a completely unnecessary dependency?

   Am I missing something big here?

A completely different approach, via @msonnabaum.

Honestly there aren't any. Objects aren't something you use only when you have state. They're something you use to provide a mostly-free layer of indirection, or to encapsulate functionality.

And this should still be done upstream...

Fixed the unit test.

Something still feels wrong with this approach. Perhaps my interpretation is wrong, but let me try to think aloud (sorry in advance if I'm not using correct/proper terminologies):

1. ControllerResolver is a facility to resolve controllers for handling a request.
2. It does not look correct to me that low-level services like EntityManager and FormBuilder suddenly need this dependency. Especially EntityManager should be decoupled from the http/request architecture.
3. In fact, EntityManager is a "controller resolver" service of its own. Thus, a controller resolver gets a controller resolver injected? In addition, the "controller" (handler) terminology of the Entity API is further distorted by mixing actual request controllers into it?
4. ControllerResolver gets the whole service container injected, which means that aforementioned classes (and possibly more when follow-up issues will apply this pattern elsewhere) indirectly depend on the service container now. I thought we already had decided long ago that D8 should not result in a system that gets the DI container injected into almost everything?
5. It looks weird that the $class argument of getInstanceFromClassName($class) can also be a service ID. That is a typical double-purpose case, which results in a unclean API. In addition, I also wonder about unforeseen name clashes and false-positive $class == $service_id cases.
6. Ultimately, I've the impression that this patch turns ControllerResolver into a Drupal-esque ContainerNG, with the primary purpose of allowing people to say:
   

   Injecting the ControllerResolver into this class here is OK, because as you can see, we're not injecting the service container.

All of this sounds very concerning to me. Please let me know where I'm wrong.

Responding to 19.4 for now, I'll be back for the rest

which means that aforementioned classes ... indirectly depend on the service container now.

Umm. HtmlFormController and EntityManager already had $this->container. The others called \Drupal::getContainer(). This is giving them *less* surface area, not more.

+++ b/core/lib/Drupal/Core/Controller/ControllerResolver.php
@@ -150,6 +141,28 @@ protected function createController($controller) {
+      // @todo Remove the second in_array() once that interface has been removed.
+      if (is_subclass_of($class, 'Drupal\Core\DependencyInjection\ContainerInjectionInterface')) {
+        $instance = $class::create($this->container);

This comment?

I agree that ControllerResolver is not an ideal place to stick this bit of functionality, however, anything creating ContainerInjectionInterface objects has an obvious dependency on the container.

is there a github mirror PR when we can see the diff more beautifully? would help a lot

Corrected mismatch in arguments between constructor definition and arguments supplied to controller_resolver service definition.

class_resolver :-

service definition argument... a service container ... fair enough.
class_resolver constructor missing ... opps container gets dropped on floor.

Fixed.

No Need to use ContinaerAwareTraits... as we are extending ContainerAware..
In this case less is more... So I have backed out the constructor and altered the service definition.

Facepalm Adding traits -- results in duplication of functionality errors .. I wish I had seen that before I did it

abstract class ContainerAware implements ContainerAwareInterface
{
    /**
     * @var ContainerInterface
     *
     * @api
     */
    protected $container;

    /**
     * Sets the Container associated with this Controller.
     *
     * @param ContainerInterface $container A ContainerInterface instance
     *
     * @api
     */
    public function setContainer(ContainerInterface $container = null)
    {
        $this->container = $container;
    }

This change addresses one flaw... its a simple change but requires lots of changes to variables up and down the lasagne.

This needs careful review as I wanted to find a better solution but could not

HtmlFormController.php requires both a

$classResolver ...see getFormObject()

AND

$controllerResolver... as it calls its parent::__contruct on FormController

IT was previous fed only a ControllerResolver

NB HtmlFormController:getFormObject(Request $request, $form_arg) --- $request is an unused variable.. something wrong there...

Three things

> -    arguments: ['@container.namespaces', '@service_container', '@module_handler', '@cache.default', '@language_manager', '@string_translation']
> +    arguments: ['@container.namespaces', '@service_container', '@module_handler', '@cache.default', '@language_manager', '@string_translation', '@class_resolver']

1) Single line reroll from core.services.yml due to class with cache.default being renamed. The interdiff.txt is from this point onwards

2) Arguement parameters no longer camel cased. ( Thank you for pointing that out )

-    $this->formBuilder = new TestFormBuilder($this->moduleHandler, $this->keyValueExpirableFactory, $this->eventDispatcher, $this->urlGenerator, $this->translationManager, $this->controllerResolver, $this->csrfToken, $this->httpKernel);
+    $this->formBuilder = new TestFormBuilder($this->moduleHandler, $this->keyValueExpirableFactory, $this->eventDispatcher, $this->urlGenerator, $this->translationManager, $this->classResolver, $this->csrfToken, $this->httpKernel);

3) Next issue on the hit list...from core/tests/Drupal/Tests/Core/Form/FormTestBase.php

Previously controllerResolver was supplied as parameter, when classResolver was needed.

These recent patches are slowly correcting issues... the tesbot error logs are getting shorter. BUT Further failures are expected.

> function template_preprocess_views_view(&$variables) {
>      }
>  
>      $container = \Drupal::getContainer();
> -    $form_object = new ViewsForm($container->get('controller_resolver'), $container->get('url_generator'), $container->get('request'), $view->storage->id(), $view->current_display);
> +    $form_object = new ViewsForm($container->get('class_resolver'), $container->get('url_generator'), $container->get('request'), $view->storage->id(), $view->current_display);
>      $form = \Drupal::formBuilder()->getForm($form_object, $view, $output);
>      // The form is requesting that all non-essential views elements be hidden,
>      // usually because the rendered step is not a view result.

This controller/class resolver bug is common to many tests.

So another DIY. If my doc changes are fine then this patch is RTBC.

1. +++ b/core/lib/Drupal/Core/Controller/HtmlFormController.php
   @@ -10,6 +10,8 @@
   +use \Drupal\Core\Controller\ControllerResolverInterface;
   

   \ Not needed

2. +++ b/core/lib/Drupal/Core/DependencyInjection/ClassResolverInterface.php
   @@ -0,0 +1,22 @@
   +  public function getInstanceFromClassName($class);
   

   @param @throw and @return missing.

3. +++ b/core/lib/Drupal/Core/DependencyInjection/ClassResolver.php
   @@ -0,0 +1,43 @@
   +      // @todo Remove the second in_array() once that interface has been removed.
   

   more then 80 chars.

1. +++ b/core/lib/Drupal/Core/Controller/HtmlFormController.php
   @@ -31,10 +33,18 @@ class HtmlFormController extends FormController {
   +   * @var  Drupal\Core\DependencyInjection\ClassResolverInterface;
   

   There are two spaces here.

2. +++ b/core/lib/Drupal/Core/DependencyInjection/ClassResolver.php
   @@ -0,0 +1,44 @@
   +use Symfony\Component\DependencyInjection\ContainerAware;
   +use Symfony\Component\DependencyInjection\ContainerAwareInterface;
   ...
   +class ClassResolver extends ContainerAware implements ClassResolverInterface, ContainerAwareInterface {
   

   Why both?

3. +++ b/core/lib/Drupal/Core/DependencyInjection/ClassResolver.php
   @@ -0,0 +1,44 @@
   +      // @todo Remove the second in_array() once that interface has been
   +      //   removed.
   

   We removed this a looooong time ago

4. +++ b/core/lib/Drupal/Core/DependencyInjection/ClassResolver.php
   @@ -0,0 +1,44 @@
   +    return $instance;
   +  }
   +}
   

   Missing blank line

5. +++ b/core/lib/Drupal/Core/DependencyInjection/ClassResolverInterface.php
   @@ -0,0 +1,30 @@
   +   * @throws \InvalidArgumentException
   

   This needs to explain when it is thrown

6. +++ b/core/tests/Drupal/Tests/Core/Form/FormTestBase.php
   @@ -64,6 +64,13 @@
   +   * @var \PHPUnit_Framework_MockObject_MockObject|\Drupal\Core\Controller\ControllerResolverInterface
   

   Please swap the order of these two

assigning to myself while I resolve #47

All issues fixed....

I hope you don't consider this feature creep but in fixing #47.6,

once I swapped the type hinting to reflect the coding standard I have to make the file look consistent and so I had to also swap 10 others

Thanks @martin107 for fixing the issues. I have only made some doc changes so I think I can RTBC this patch.

Patch looks great, just did a full review and test.

I'm not sure how much getClassResolverStub() will actually be used, but makes sense to keep it I think.

+1 - Just added one missing space as I had the patch applied!

It's all about the finer details :D

Reroll.

Trivial reroll.... so back to rtbc

Yep, looks good to me.

I will do the reroll

Lots of niggly changes any of which could blow up testing

so manual testing :-

1) It installs :-)
2) ControllerResolverTest passes
3) EntityManagerTest passes

Lets see what test bot says

Green.

62: class_resolver-2165475-62.patch queued for re-testing.

I think this is significant enough to maybe merit a change record for the addition?

Also, here's a reroll without the trailing whitespace. ;)

Updating rerolled patch.

Patch looks good so back to RTBC

1. +++ b/core/lib/Drupal/Core/Controller/ControllerResolver.php
   @@ -109,31 +121,17 @@ protected function createController($controller) {
   +    $controller = $this->classResolver->getInstanceFromClassName($class_or_service);
   

   Shouldn't the method be getInstanceFromClassOrServiceName()?

2. +++ b/core/lib/Drupal/Core/Controller/HtmlFormController.php
   @@ -53,11 +63,7 @@ public function __construct(ControllerResolverInterface $resolver, ContainerInte
        }
   

   getInstanceFromClassName() does class_exists() and also handles services, so can't this just use the method? If the logic is different it could use a comment as to what needs changing.

3. +++ b/core/lib/Drupal/Core/DependencyInjection/ClassResolverInterface.php
   @@ -0,0 +1,31 @@
   +   *   A class name.
   

   Or service name right? Docs could be much clearer about this accepting both.

Also:

+    if ($this->container->has($class)) {
+      $instance = $this->container->get($class);
+    }

This looks like it could be a performance regression. When you request a service that doesn't exist, Symfony runs a levenshtein comparison on the string against all services to create suggestions as part of the exception it throws for missing services.

I didn't check if that's the case here, but it might work out cheaper to do the class_exists() check first. At least, this needs checking.

This needs a reroll after #2209977: Move form validation logic out of FormBuilder into a new class, and will need another one after #2257835: Move form submission logic out of FormBuilder into a new class which is RTBC, so I'm just going to wait and do that

Okay, if someone else wants to reroll it before then, feel free. I just know that the constructor signature of FormBuilder *just* changed, and is about to change again.

Here is a reroll either way. Entity Manager needed it too.

Usually good to rebase that branch too.

FFS!!

Meh. SOrry for inappropriate noise.

Thanks whoever did mass cancelling!!

Looks like there's still some cruft that could be removed:

1. +++ b/core/lib/Drupal/Core/Controller/HtmlFormController.php
   @@ -52,16 +62,7 @@ public function __construct(ControllerResolverInterface $resolver, ContainerInte
        // If this is a class, instantiate it.
   

   Comment no longer applies.

2. +++ b/core/lib/Drupal/Core/DependencyInjection/ClassResolver.php
   @@ -0,0 +1,47 @@
   +    if ($this->container->has($definition)) {
   

   You're right that the has() call protects against needless container lookups, sorry for false alarm.

3. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -129,13 +140,9 @@ public function __construct(FormValidatorInterface $form_validator, ModuleHandle
        if (is_string($form_arg) && class_exists($form_arg)) {
   

   Do we really still need the class_exists() here? That should be covered.

4. +++ b/core/modules/views/lib/Drupal/views/Form/ViewsForm.php
   @@ -169,17 +170,12 @@ public function submitForm(array &$form, array &$form_state) {
        if (class_exists($form_step_class)) {
   

   Again is the class_exists() necessary?

5. +++ b/core/modules/views/lib/Drupal/views/Form/ViewsForm.php
   @@ -169,17 +170,12 @@ public function submitForm(array &$form, array &$form_state) {
   +    return \Drupal::getContainer()->get($form_step_class);
   

   Why is this needed if the class resolver can get a service anyway before it reaches here?

Ah, nice one.
Thanks @catch!

91: class_resolver-2165475-91.patch queued for re-testing.

91: class_resolver-2165475-91.patch queued for re-testing.

Do we really still need the class_exists() here? That should be covered.

That is what was keeping the logic to return the original string for the form ID if just a string is provided. Otherwise, we assume it's a class when it's just a string like 'foo' or something. See FormBuilderTest::testGetFormIdWithString(). So we can add this back like this patch does, or re-think how we deal with this using the ClassResolver.

OK so let's keep that as is, however it could use a comment containing the basics of #98.

?

Yeah that's great. Back to RTBC.

Super.

Rerolled. Also, I think we can just remove the container aware stuff for the controller resolver now as the class resolver handles that?

Silly.

Another reroll of that.

Now I forgot to pull. Here is a re-re-base.

Committed 15cf730 and pushed to 8.x. Thanks!