Require type hints for automatic entity upcasting

I think this would also be really really helpful to scotch and contrib built around scotch. We are defining plugin contexts via typed_data so if the routes were defining their parameter upcasting via typed_data as well (and I would vote for a more explicit defining here) then we could seamlessly work together, and I think that was always the point.

Eclipse

+++ b/core/lib/Drupal/Core/Routing/TypedDataEnhancer.php
@@ -0,0 +1,80 @@
+    // This enhancer only works if there are typed data definitions.
+    $options = $route->getOptions();
+    if (empty($options['typeddata'])) {
+      return $defaults;

This would imply no auto-detection at all, requiring module developers to explicitly define all conversions. That's a no. We can't regress DX like that.

+++ b/core/modules/system/tests/modules/upcasting_test/upcasting_test.routing.yml
@@ -0,0 +1,23 @@
+  options:
+    typeddata:
+      entity:
+        type: 'entity'
+        constraints:
+          EntityType: 'node'
+          Bundle: 'page'

We can't force all module developers to do this on every route.

We put a lot of effort into not causing a DX regression in the original param converter issue. I cannot get behind regressing that. I've mentioned that to EclipseGc multiple times.

%node is documentation, {node} is not. one tells me how to load, the other is magic that guesses. (well ok %node can be magicky too when it doesn't work, but the fact remains) This is not a regression, and buys us MUCH. This approach is very similar to any situation in which you might have two of the same thing in the url you need upcast. magic vs non-magic in that instance seems like a real dx wtf to me. Explicit documentation of how to upcast parameters isn't really weird, aren't there other benefits to this as well?

If you aren't open to that idea, then I'm out of ideas on how to prevent a plugin type like page_manager's tasks from existing. We'll have to do a lot of really dirty guessing of parameters when we're not actually on the route and it will be messy. I'd encourage us to really consider this further before declaring it a "no go". As always I'm happy to chat about this and illustrate my reasoning further.

Eclipse

%node triggers automagic conversion of an into to a stdClass. Using {node} does the same. "One is documentation, the other tells me how to load" is completely false. The only difference is that the "magic guess" of {node} is, currently, more involved than "guess for a function with a magic name".

Admittedly, another potential benefit is being able to use {account} instead of {user}, because {user} means a variable named $user that is not the same as the global $user variable, which is a security hole BEGGING to happen, but the fix to that is to get rid of the crime against nature that is global $user.

If we were to require explicit declaration and require that ALL parameters were typed data only, then at that point we probably just rip out the ParamConverter system entirely and make it a single route enhancer (or bridged from a single route enhancer). Anyone trying to upcast something that's not a typed data object then is on their own. That would likely make Sam happy, because we could trivially do that upcasting earlier during a Route Filter. However, that then requires explicit declaration on the route. That's a regression from "%node is magic, it just works". There's no way to whitewash that.

Maybe that trade-off is worth it. But we can't pretend it's not a trade-off. And... I'm not going to make that call myself. :-)

FWIW, a LOT of stuff is typed data already. every entity can be upcast that way (config or content) strings, ints, dates, durations, fields and more. This is one of the big reasons I think page_manager has had such a long road to community acceptance, the "context plugins" which is essentially what typed data is doing in D8, are a pain to write, and no one understands them and the only system using them is page_manager, so when people run into a situation where they need one, rather than invest in it, they just move on. Typed Data already covers a lot more than page_manager's context system ever tried to cover, and should solve I think 98% of the parameter upcasting use case, and then if we are forcing this, it prevents any "writing around" it that's likely to happen and gives contrib a consistent tool for solving the problem. Most typed_data plugins are actually pretty simple, it's really just entity that is confusing and that's being worked on.

Indeed we should probably figure out who can make this call.

Eclipse

I can throw in an opinion, if not make a call... I find the DX impact of #4 absolutely repulsive. It seems like the argument for foisting this extra overhead onto 100% of module developers is to make it easier for core developers and ~5% of module developers who define their own unique type casting logic. I do not find that an acceptable trade-off, since it seems like defining a foo.routing.yml file will happen about 20 bazillion times more often than defining a new way to typecast slugs.

"So, the gain from this would be: Less repetition of loading logic plus encouraging people to implement their objects as typed data :P." ... "This is important because I don't want to have to do all those checks in my controllers in D8."

I don't see how this makes this major, seems like just a nice to have? Maybe the issue summary needs an update?

No, this is not at all a good summary of why we need this, or what it buys us.

When you upcast something currently, I have exactly 0 idea of what that thing you're upcasting is. Also whatever it is, it likely has no layer around it which I can use when the upcast item is NOT available.

Let's take a simple example like node/%node (going to stay as close to D7 stuff as possible to illustrate this as clearly as possible).

The menu system in D7 knows that %node means node_load(). The system in D8 is going to check if 'node' is an entity name and upcast it. if not, there are some fallback guesses it will also make. From a metadata perspective this is completely useless. In terms of generating a "display" for this, I am immediately sunk because I know nothing about this page beyond what I can code/guess. And even if I know it's a node, I need it as typed data anyway, because when working in the UI I won't have a legit node object, I'll have something representing a node that is not actually (which typed data can do).

To put this into D7 terms, if you've ever looked at page_manager's task plugins, this is EXACTLY why they exist. The system has no clue what is on a page, so it needs a layer to do that interpretation. If I don't have some sort of metadata layer telling me what all the parameters in a route are, we will be in exactly the same place for D8 and no page except pages with explicit layers built for them will ever be able to have a display take over their output.

Embedding this metadata layer directly into the routing system will bring global display coverage to not only core, but also all of contrib. That is a HUGE win, and the fact that we're asking people to explicitly define how their parameters from the URL are loaded does not seem unreasonable, nor is it really a DX loss since there won't be any mystery about how the system works. This has the beneficial byproduct of forcing all future modules to define typed data handlers for anything they ever want to pass across the URL (there are many of these already for simple strings, integers, etc already) which means contrib continues to stay compatible with the core approach here.

I hope this clarifies why I think this is such a huge issue. I though I had outlined this once already, but apparently not, so my apologies.

Eclipse

My POV on the DX pain issue is that something along the lines of #5 is not a DX regression, but improvement. It tells me that the system is going to interpret that part of the path using the specified typed data plugin. That saves one from looking up and understanding the guessing approach. In short: less magic -> DX++. Then, I don't care much about having to provide those 3 LoC to define how it will load the object.

I think #5 is fine when you have a slug named {entity} that you want to constrain to be a node of type 'page'. But I agree with webchick and disagree with fago that forcing extra lines on route declarations for slugs named {node} that you want to be any node makes for worse DX. I also don't see why it's necessary to do that to achieve the goals of this issue. I think it's possible to have TypedData support, to allow for introspection from SCOTCH, and also to allow for sane default metadata of slug names that correspond to entity types. Maybe we just need to move where the code that applies the defaults lives? #1943846: Improve ParamConverterManager and developer experience for ParamConverters contains one such move, though maybe we want to move it somewhere even different to be available to SCOTCH.

I don't seen how separating the explicit loading declarations from the route buys us anything but yet more files/hooks/something. I want this on the route if possible, as this will ensure that we have a workflow that is not broken just for the sake of expediency. Likewise, both route parameters and typed data can do more than just entity loading. String, date, etc handling is a hugely powerful thing.

As a side note, I've got a d8 contrib I'm working through right now, and the number of files I'm creating is a little staggering. I like a lot of my workflow stuff doing it thus far, but that aspect has been a little daunting, and I already know most of the files I need to create... so, I'm "--" to separating the loading declaration from the route declaration.

Eclipse

I'm certainly not suggesting that the loading declarations be specified manually somewhere other than in the routing.yml file. For ones that need loading declarations that can't be easily inferred from the slug name, I agree with putting them within the options portion of each route. All I'm saying is that there can be code somewhere along the lines of:

// This is pseudo code
foreach ($slugs as $slug) {
  if ($slug IS_AN_ENTITY_TYPE) {
    $route_options[$slug] = 'entity:' . $slug; // or whatever the exact syntax ends up being
  }
}

This allows for routes like /node/{node} to not require options be manually specified within routing.yml files.

Ideally, the code that provides this defaulting should be somewhere centralized, so that both routing code and SCOTCH can just call $route->getOptions() (or something like that, if not that exactly) and receive a $options array that includes a merge of manually specified and default options, and therefore, not need to care what was manually specified and what was defaulted.

route parameters and typed data can do more than just entity loading. String, date, etc handling is a hugely powerful thing.

Whether it makes sense to define defaults for slug names that aren't entity types remains to be seen. For example, {entity_type} and {langcode} seem like string-valued slug names that we should be able to define some default validation constraints around. But whether we do that or not doesn't change the desirability of doing it for {node}, {user}, and all other slug names that are entity types, since we know that those are very common slug names used in very many routes.

well, if we're going to suggest that (and based upon you're explanation I'm not entirely opposed) why not just expect slugs like:
{string}
{entity:node}
[language]
{entity:user}
{integer}

This would ensure that the slugs are in typed data order, and we'd just need a way to put constraints on it further in the options if necessary. So long as I can request from the route object a definition of all it's variables and get either definitions or full typed data objects, I'll be happy.

Eclipse

Or, what if you have a route in module "A" that defines a slug named {foo} which is supposed to be a simple string and then module "B" comes along and defines an entity type called "foo"? ===> BOOM! The whole thing blows up.

Ok, I think that's a pretty compelling argument actually. Can you open a new issue for removing the magic from the existing EntityConverter in HEAD? We need to get webchick, catch, and others to weigh in on that.

Removing the old stuff while adding the new one seems OK here - if it doesn't massively bloat the patch it'll make it easier to review the change.

I haven't reviewed the patch properly here so not commenting on the DX issue at all.

The DX regression on Views routes is really really unfortunate.

views_ui.breakLock:
  pattern: '/admin/structure/views/view/{view}/break-lock'
  defaults:
    _form: '\Drupal\views_ui\Form\BreakLockForm'
    display_id: NULL
  requirements:
    _permission: 'administer views'

views_ui.breakLock:
  pattern: '/admin/structure/views/view/{view}/break-lock'
  defaults:
    _form: '\Drupal\views_ui\Form\BreakLockForm'
    display_id: NULL
  requirements:
    _permission: 'administer views'
  options:
    typeddata:
      view:
        type: 'entity'
        constraints:
          EntityType: 'view'

Seriously?

Also, ConfigEntities aren't typed data and I don't think we want them to be...

It is still a DX regression, because if I want to load a View ConfigEntity, why do I have to know what typeddata is?
That's needlessly exposing internals, and frankly has nothing to do with Views, Routes, or ConfigEntities.

1) Removing the intermediary ParamConverter level is absolutely not OK. We MUST be able to do ParamConversion outside of the routing process to be able to generate access-restricted links. Removing that decoupling is a won't fix.

2) Hacking the placeholder (dude, really, slug? That's a stupid name; even Symfony doesn't use it generically like that) with an alternate syntax is also not cool. Right now, you can use existing Symfony documentation to document how our routing API works, about 80% of the way. The URI template syntax is an IETF standard:

http://tools.ietf.org/html/rfc6570

No, we are not breaking an IETF standard we've already adopted. That is not happening. The explicit options/typeddata construct is better than that in every possible way, even if I am still firmly against it on the grounds that "Typed Data ALL THE THINGS!" is not a concept that anyone has managed to sell me on yet in the first place.

3) The one and only compelling argument I've heard from anyone so far for why we can't just have, duh, multiple converters (one for entities, one for your own random crap, etc.) is avoiding Task plugins for SCOTCH (which is one of the most obtuse parts of Panels in D7, and that's some pretty steep competition). OK, so how do we address that problem in a way OTHER THAN "every object in all of Drupal must be part this big reimplementation of PHP syntax called Typed Data that all of 4 people understand"?

That's the question. And it's nominally separate from param conversion in the first place.

In #1943846: Improve ParamConverterManager and developer experience for ParamConverters, I've been suggesting a syntax like this:

views_ui.breakLock:
  pattern: '/admin/structure/views/view/{view}/break-lock'
  defaults:
    _form: '\Drupal\views_ui\Form\BreakLockForm'
    display_id: NULL
  requirements:
    _permission: 'administer views'
  options:
    parameters:
      view:
        type: 'entity:view'

That removes the words typeddata and converter from the routing file, both of which are internal concepts. It retains the idea that you need to explicitly provide information about your parameters, at a minimum, their "type", if you want to do anything with them other than pass through their string values.

why we can't just have, duh, multiple converters (one for entities, one for your own random crap, etc.)

Not sure how relevant that question is to what's being discussed here, but the compelling argument for needing to explicitly state the type even for entity types, and even if that was implemented without TypedData, is in #21.

Right, automatically recognizing entity type names is kind of handy but seems to bring more problems when modules get enabled and the magic semantics of a given placeholder suddenly changes.

Explicit argument declarations ++

Above, I said one of my biggest problems with this typeddata approach:

That's needlessly exposing internals

I've rerolled #1943846: Improve ParamConverterManager and developer experience for ParamConverters, I really like that approach over this.

So we'd end up with

views_ui.breakLock:
  pattern: '/admin/structure/views/view/{view}/break-lock'
  defaults:
    _form: '\Drupal\views_ui\Form\BreakLockForm'
    display_id: NULL
  requirements:
    _permission: 'administer views'
  options:
    parameters:
      view:
        entity_type: 'view'
        tempstore: 'views'

For the routes that need a view entity upcast to a ViewUI from tempstore.

Can we retitle this to remove the TypedData parts, or should we just do that in the other issue and close this one?

#28: typed-data-upcasting-1906810-28.patch queued for re-testing.

rerolled the patch in #28

I apparently had a problem with that patch... let's try that again

+++ b/core/modules/contact/contact.routing.ymlundefined
@@ -4,3 +4,9 @@ contact_category_delete:
\ No newline at end of file

+++ b/core/modules/filter/filter.routing.ymlundefined
@@ -4,3 +4,9 @@ filter_admin_disable:
\ No newline at end of file

+++ b/core/modules/forum/forum.routing.ymlundefined
@@ -10,3 +10,9 @@ forum_settings:
\ No newline at end of file

+++ b/core/modules/image/image.routing.ymlundefined
@@ -11,3 +17,9 @@ image_effect_delete:
\ No newline at end of file

+++ b/core/modules/menu/menu.routing.ymlundefined
@@ -25,3 +39,9 @@ menu_delete_menu:
\ No newline at end of file

+++ b/core/modules/system/tests/modules/upcasting_test/upcasting_test.info.ymlundefined
@@ -0,0 +1,6 @@
\ No newline at end of file

+++ b/core/modules/system/tests/modules/upcasting_test/upcasting_test.routing.ymlundefined
@@ -0,0 +1,23 @@
\ No newline at end of file

The reroll introduced some no new line EOF errors. Please reroll again without introducing those errors (or modify the patch, if that's easier for you).

Reroll. Should eliminate the whitespace errors.

+1 to everything in #52, including the priority change. I especially like the idea of applying reflection at route build time and writing out the definition into the route options, rather than attempting any sort of reflection at request resolution time.

I came back to the conclusion that we -really- need this. And before the end of next week too.

1. Is getting this done in a week feasible?
2. Is #1943846: Improve ParamConverterManager and developer experience for ParamConverters a useful interim step?
3. If the answer to #1 is no, and the answer to #2 is yes, then what is the urgency of getting this done before API freeze? In other words, what APIs would you foresee this breaking, once the other issue is done? Or is the urgency because of other core work blocked on this?

Now, due to the fact that FeedInterface extends EntityInterface we already know that what we have here is an entity. Now, since all entities should define a unique interface like FeedInterface or NodeInterface we can iterate over the $entityManager->getDefinitions() and check (with Reflection) which entity type uses an entity class that implements FeedInterface.

Ok, so pondering on my above questions, since the only upcasting we currently have in HEAD is entities, and since those already are reflectable and introspectable as you describe, are you saying we should skip the required explicit type declaration proposed in #1943846: Improve ParamConverterManager and developer experience for ParamConverters, and implement something directly here that maintains the ability of most/all current routes in HEAD to not require any changes to the routing.yml files?

and also seeing that @effulgentsia seems to be more and more aboard with TypedData

For the record, I have always been on board with TypedData. Sorry if I wasn't clear about that earlier in this issue. I was initially against required explicit type declaration if it were in any way possible to support some kind of smart automagic. I came around to the need for it in #21, but am thrilled that you came up with a reflection-based idea for how to make automagic work robustly. If explicit type declaration is required for every route parameter (that requires upcasting) of every route, then I was and still am against requiring the long syntax in #4, and we don't yet have the ability to shorten it to #5 (though we hopefully will soon). However, if we can reflect and autogenerate the definitions for 90+% of our route parameters, then I'm ok with requiring the longer declarations for the edge cases, especially since it's likely we'll be able to support the shorter syntax eventually.

Anyway, thanks for all the great thinking that led you to #52 and for writing it up so well.

Simply expose your types as typed data. I don't see an argument for not doing that (and hence requiring a custom parameter converter).

1. Will we make tempstore:COLLECTION a TypedData type?
2. Do we want to support contrib modules that integrate routes from non-Drupal projects, where they want to upcast to something that uses some other typing system entirely, instead of Drupal's TypedData?

Re 58.1: ok, makes sense.

Re 58.2: Unfortunately, I'm not really very up to speed on the entity/document architectures of other projects. But for example, http://symfony.com/doc/master/cmf/bundles/routing.html#using-the-phpcr-o... mentions using the Symfony CMF router (same as Drupal's router) for routes that work with PHPCR-ODM. While I'm not familiar with the details of that, from what I do understand, Doctrine ODM mapped objects have some conceptual similarities to, but very different implementations than, Drupal's entities and TypedData objects. It seems possible that someone might want to write a Drupal module that provides some integration with something based on PHPCR-ODM. I don't know enough about such a use case to speculate on whether requiring that integration to involve a TypedData bridge makes any sense or not.

OK, since much of typed data is still a mystery to me (it still feels like over-engineered over-extending), given that I the following code in my module that looks like the following:

class Thingie {
  public function __construct(array $data) {
    $this->data = $data;
  }
  // Other read methods here.
}

class ThingieRepository {
  public function load($id) {
    // Returns a Thingie object by loading it from a 3rd party web service using Guzzle.
  }
}

What is the *minimum* amount of work I have to do in order to make this controller Just Work(tm):

class ThingieController {
  public function view(Thingie $thingie) {
    // Return some formatted string or something.
  }
}

And if the answer includes "Thingie extends SomeDrupalClass", then that's a won't fix.

Well, given the intro paragraph of #60, the answer to #64 is you have a choice:
- Either expose your Thingie as a Drupal @DataType, ala #66.
- Or, implement your own RouteEnhancer.

And if I read #61 right, then maybe "implement your own ParamConverter" will remain as yet another option.

So, after a short skype with fubhy, I am VERY +1 to this. We need to be reflecting from _content/_form as well, but the basic approach is good as far as I'm concerned. This will give us access to the definitions of objects w/o having to have real objects, it will give us magic integration if people follow best practices, and it allows us to manually notate it in a worst case scenario in contrib or whatever and still get the support we want. Scotch REALLY needs this, and I can't express how much I appreciate fubhy applying mental effort to solving this for everyone (DX and Scotch).

TL:DR +10000000000

Eclipse

Rerolled.

To be honest, I don't fully understand how what fubhy is proposing solves EclipseGc's issue. But I guess I don't need to. I am not a huge fan of it, but I can live with what he has proposed. The caveat is that we do need to keep the ParamConverter layer below RouteEnhancer, because we need ParamConverter to be divorced from routing per se so that we can use it elsewhere.

Addendum: I should still be able to add my own ParamConverter that ignores TypedData and LoadableInterface and so on entirely; if that makes it incompatible with Scotch, that's my problem, but I should be able to roll my own directly if I want to.

adding my princess tag

I just realized one potential issue with reflection: That means loading all controllers into memory on dumping in order to reflect on them. That's... unpleasant for performance. That's why we're not using reflection for annotations now.

That means loading all controllers into memory on dumping in order to reflect on them. That's... unpleasant for performance.

Ugh. Yes, performance in terms of CPU time isn't as much a concern since it's only on router rebuild, but it could lead to out of memory errors and a resulting WSOD, which is a deal killer. I wonder if any of the Doctrine code we already have in core can help us reflect typehints from tokenized source, similar to how we currently handle the plugin annotations. If not, there's https://github.com/Andrewsville/PHP-Token-Reflection, and perhaps other libraries to look into.

Another option is instead of trying to do it all during router rebuild, we progressively warm up the router. After a new rebuild, there's no auto-discovered type info written out. Each time we route, we check if the type info is there; if not, we reflect just the controller we need to concern ourselves with, and then update the router with that info to speed up that route for future requests.

Well, this could be measured by loading all current controllers/forms from forminterface and then try a reflection on it.
There are currently ~100 of them, but this number will potentially be 1000 or more on a real site.

Is this issue still valid?

It is. fubhy's been swamped with work, though. If you want to pick it up, that would be great. :-)

@fubhy said in #1983100: Provide a LoadableInterface for Typed Data objects that that interface is no longer needed, so I'm not sure what this means for this issue?

@EclipseGc & @sdboyer: Would be great to get an update from you with regard to this issue; specifically whether the TypedData information (sans upcasting) is useful, or much rather, whether the lack of it presents a hard roadblock for your work.

Either way this doesn't look critical to me. Splitting the difference at major.

@sun, et. al,

It's imperative that we have some sort of meta data layer on the route definition to tell use what sort of data we'll be getting. I'll attempt to illustrate:

/node/{node}

This tells us nothing, and even if you have some upcasting methodology that says "Oh we have this 'node' thing, check to see if there's an entity of that type... oh there is, great load it." that's not actually useful because if we ever want to administratively override /node/{node} then we absolutely need a definition of what {node} is, instead of figuring that out at run time for the page itself. This is how page_manager in D7 works, but since D7's menu system uses magic loading by *_load() methods, and provides no metadata, merlin invented a custom plugin type to define that metadata and route the requests to that menu item through this plugin which asks page manager if there are overrides for the specific item in question or whether it should farm it out to Drupal's normal callback. This means even though page_manager is smart enough to deal with almost every entity type ever invented (except for rules and search api indexes and servers) it can't hijack the pages those entities define as their view route. Yes we could have written more code to do that, but it's a stop gap measure and having the routing system in D8 just support this sort of behavior via introspection of the menu route, or required manual definition by a developer is a FAR superior methodology as it opens the door to overriding ANY page created anywhere... ever.

In short, yes it's super important that the route, without being executed, be able to tell me that {node} is a node entity and that {string} is a string.

Eclipse

Also, I apologize for taking so long to respond here. Life, work, stuff... you know.

Eclipse

In short, yes it's super important that the route, without being executed, be able to tell me that {node} is a node entity and that {string} is a string.

Just to inform other people in the issue, this kind of metadata is already available:

 drush php-eval "drush_print_r(\Drupal::service('router.route_provider')->getRouteByName('node.view'))"
Symfony\Component\Routing\Route Object
(
    [path:Symfony\Component\Routing\Route:private] => /node/{node}
    [host:Symfony\Component\Routing\Route:private] => 
    [schemes:Symfony\Component\Routing\Route:private] => Array
        (
        )

    [methods:Symfony\Component\Routing\Route:private] => Array
        (
        )

    [defaults:Symfony\Component\Routing\Route:private] => Array
        (
            [_content] => \Drupal\node\Controller\NodeController::page
            [_title_callback] => \Drupal\node\Controller\NodeController::pageTitle
        )

    [requirements:Symfony\Component\Routing\Route:private] => Array
        (
            [_entity_access] => node.view
        )

    [options:Symfony\Component\Routing\Route:private] => Array
        (
            [compiler_class] => \Drupal\Core\Routing\RouteCompiler
            [parameters] => Array
                (
                    [node] => Array
                        (
                            [type] => entity:node
                            [converter] => paramconverter.entity
                        )

                )

            [_access_checks] => Array
                (
                    [0] => access_check.entity
                )

        )

    [compiled:Symfony\Component\Routing\Route:private] => 
)

so the issue is more about avoiding the unwanted upcasting problem.

I have looked into the sandbox project and followed this commit - http://drupalcode.org/sandbox/sumsi/2031313.git/commit/d67edc0. Its my initial approach to pull out the only part which takes care of type data upcasting.

I pulled code from wrong branch, spoke with dawehner and I will be working on correct branch to submit new patch.

working on a new patch and will submit tomorrow

followed fubhy's sandbox - https://drupal.org/sandbox/sumsi/2031313. This patch may not be complete since I nitpicked manually and there were some files which I couldn't find in core but its just a initial approach just to get me started.

Maybe someone else can take a look at fubhy's sandbox. I have manually checked all the commits and found it very confusing.

After looking at the git repo of fubhy this is basically the functionality we need without all the crazy abstractions.

.

tag typo fix

Worked a bit on unit, let's see whether I covered all usecases with the tests.

Given that we use that kind of reflection now we initialize all dependencies of all controllers, which requires basically a fullstrapped drupal instead of a DrupalUnitTestCase

This could fix most of the failures.

1. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
   @@ -0,0 +1,192 @@
   +   * Entity manager.
   

   THE entity manager.

2. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
   @@ -0,0 +1,192 @@
   +  protected function getController(array $defaults) {
   

   Needs @param

3. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
   @@ -0,0 +1,192 @@
   +        if (in_array('Drupal\Core\DependencyInjection\ContainerInjectionInterface', class_implements($form_arg))) {
   

   This could use is_subclass_of() now I think, it works with interfaces too. I don;t really like call_implements...

4. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityResolverManagerTest.php
   @@ -0,0 +1,386 @@
   +      'description' => '',
   

   Yeah, people don't really care, but you know we need one :)

5. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityResolverManagerTest.php
   @@ -0,0 +1,386 @@
   +  protected function setupControllerResolver($controller_definition) {
   ...
   +  protected function setupEntityTypes() {
   

   Docblocks please.

6. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityResolverManagerTest.php
   @@ -0,0 +1,386 @@
   +class BasicControllerClass {
   ...
   +class EntityTest extends Entity {
   

   Maybe some simple docblocks

Yeah, people don't really care, but you know we need one :)

I don't see why. It would not add a single value and just get it into core allows you to improve things in the longrun.

One thing you should learn from the fixed failures: Never execute code in your constructor. Just assign your dependencies and continue later.

Some less failures.

.

Per dawehner's request so it's on his radar for DevDays...

Let's see, this should fix quite a couple of the failures.

Made a bit of progress on the REST side of it.

.

Maybe this helps already.

This fixes at least a couple of the failures.

Let's see ...

There we go.

Rerolled.

updating the title.

+++ b/core/lib/Drupal/Core/Flood/DatabaseBackend.php
@@ -36,7 +36,11 @@ class DatabaseBackend implements FloodInterface {
+<<<<<<< ours
...
+=======
+   *   The request stack.
+>>>>>>> theirs

git merge cruft.

Good catch.

A couple of additional cleanups got added/unit test coverage got improved.

1. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
   @@ -0,0 +1,211 @@
   +    if (isset($defaults['_content'])) {
   +      $controller = $this->controllerResolver->getControllerFromDefinition($defaults['_content']);
   +    }
   +    if (isset($defaults['_controller'])) {
   +      $controller = $this->controllerResolver->getControllerFromDefinition($defaults['_controller']);
   ...
   +    if (isset($defaults['_form'])) {
   

   Is this all there is? Does this run after things like _entity_form/_entity_list/_entity_view?

2. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
   @@ -0,0 +1,211 @@
   +      //   so for example also upcast NodeInterface $cat automatically.
   

   What is $cat?

3. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
   @@ -0,0 +1,211 @@
   +      list($entity_type, ) = explode('.', $entity_view);
   ...
   +      list($entity_type, ) = explode('.', $entity_form);
   

   These commas are optional

4. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
   @@ -0,0 +1,211 @@
   +          list(, $parameter_entity_type) = explode(':', $info['type']);
   

   This should have , 2 in the explode to prevent future weirdness

5. +++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
   @@ -56,8 +56,8 @@ function __construct($cid, CacheBackendInterface $cache, LockBackendInterface $l
   +    $this->persistable = $modules_loaded;
   +    $this->persistable &= \Drupal::hasRequest() && \Drupal::request()->isMethod('GET');
   

   Eek, can we combine them on one line? Bitwise always looks like assign-by-reference to me, and this is short enough to stay on one line

6. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationListController.php
   @@ -49,35 +39,39 @@ public function __construct(ConfigMapperManagerInterface $mapper_manager, $confi
   -      $container->get('plugin.manager.config_translation.mapper'),
   -      $container->get('request')->attributes->get('_raw_variables')->get('config_translation_mapper')
   +      $container->get('plugin.manager.config_translation.mapper')
   

   Nice!

7. +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleContentTest.php
   @@ -150,12 +150,12 @@ function testContentTypeDirLang() {
   -    \Drupal::languageManager()->reset();
    
        // Install Spanish language.
        $edit = array();
        $edit['predefined_langcode'] = 'es';
        $this->drupalPostForm('admin/config/regional/language/add', $edit, t('Add language'));
   +    \Drupal::languageManager()->reset();
   

   Why this change?

8. +++ b/core/modules/system/lib/Drupal/system/SystemManager.php
   @@ -8,9 +8,9 @@
   -use Symfony\Cmf\Component\Routing\RouteObjectInterface;
    use Drupal\Core\Database\Connection;
    use Drupal\Core\Extension\ModuleHandlerInterface;
   +use Symfony\Cmf\Component\Routing\RouteObjectInterface;
    use Symfony\Component\HttpFoundation\RequestStack;
    
    /**
   @@ -77,7 +77,7 @@ class SystemManager {
   
   @@ -77,7 +77,7 @@ class SystemManager {
       *   The database connection.
       * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager
       *   The entity manager.
   -   * @param \Symfony\Component\HttpFoundation\RequestStack
   +   * @param \Symfony\Component\HttpFoundation\RequestStack $request_stack
       *   The request stack.
       */
      public function __construct(ModuleHandlerInterface $module_handler, Connection $database, EntityManagerInterface $entity_manager, RequestStack $request_stack) {
   @@ -171,8 +171,7 @@ public function getMaxSeverity(&$requirements) {
   
   @@ -171,8 +171,7 @@ public function getMaxSeverity(&$requirements) {
       *   A render array suitable for drupal_render.
       */
      public function getBlockContents() {
   -    $request = $this->requestStack->getCurrentRequest();
   -    $route_name = $request->attributes->get(RouteObjectInterface::ROUTE_NAME);
   +    $route_name = $this->requestStack->getCurrentRequest()->attributes->get(RouteObjectInterface::ROUTE_NAME);
   

   Unless I'm misreading this, there are no actual changes here.

9. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/RegistryTest.php
   @@ -35,6 +35,7 @@ public static function getInfo() {
   +    \Drupal::getContainer()->enterScope('request');
   

   Hm, a comment would help here

10. +++ b/core/modules/views_ui/views_ui.routing.yml
    @@ -148,6 +152,7 @@ views_ui.form_edit_details:
    +        type: entity:view
    

    Why are these needed here, but not for other entities like node?

11. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityResolverManagerTest.php
    index 339aa00..0000000
    --- a/modules/README.txt
    
    --- a/modules/README.txt
    +++ /dev/null
    

    Put this back please :)

Thank you for your review, actually someone who not just talks shit all day long.

Is this all there is? Does this run after things like _entity_form/_entity_list/_entity_view?

There is indeed, but there we don't need reflection but can leverage entity information, see setParametersFromEntityInformation()

What is $cat?

It is just yet another variable name, changed that.

These commas are optional

Good point.

Why this change?

Because it fixes the test

Unless I'm misreading this, there are no actual changes here.

This was probably triggered by some merge problem.

Why are these needed here, but not for other entities like node?

Views don't actually use the entity upcasted but converts to viewsUI objects, this conflicts with the reflection based approach.

meh

1. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
   @@ -0,0 +1,211 @@
   +      // If the parameter name matches with an entity type try to set the
   +      // upcasting information automatically. Therefore take into account that
   +      // the user has specified some interface, so the upasting is intended.
   +      // @todo It would be also possible to not rely on the variable name at all
   +      //   so for example also upcast NodeInterface $other_var automatically.
   

   Isn't the @todo the whole point? Rely on the type hint exclusively, not the variable name?

2. +++ b/core/lib/Drupal/Core/EventSubscriber/EntityRouteAlterSubscriber.php
   index dbd44e9..edc0089 100644
   --- a/core/lib/Drupal/Core/Utility/ThemeRegistry.php
   
   --- a/core/lib/Drupal/Core/Utility/ThemeRegistry.php
   +++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
   
   +++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
   +++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
   @@ -56,8 +56,7 @@ function __construct($cid, CacheBackendInterface $cache, LockBackendInterface $l
   
   @@ -56,8 +56,7 @@ function __construct($cid, CacheBackendInterface $cache, LockBackendInterface $l
        $this->cache = $cache;
        $this->lock = $lock;
        $this->tags = $tags;
   -    $request = \Drupal::request();
   -    $this->persistable = $modules_loaded && $request->isMethod('GET');
   +    $this->persistable = $modules_loaded && \Drupal::hasRequest() && \Drupal::request()->isMethod('GET');
   

   I am very confused as to why this is necessary.

3. +++ b/core/modules/rest/lib/Drupal/rest/Plugin/ResourceBase.php
   @@ -175,4 +164,32 @@ public function availableMethods() {
   +  protected function getBaseRoute($canonical_path, $method) {
   +    $lower_method = strtolower($method);
   +
   +    $route = new Route($canonical_path, array(
   +      '_controller' => 'Drupal\rest\RequestHandler::handle',
   +      // Pass the resource plugin ID along as default property.
   

   Splitting this out to a method makes total sense, but we should name it createBaseRoute() or similar instead. getBaseRoute() suggests that we'd get back the same route object on each call, which is not the case.

Isn't the @todo the whole point? Rely on the type hint exclusively, not the variable name?

Nope, the whole point is to no upcast unless you really meant it.

I am very confused as to why this is necessary.

The reason are tests. Previously route rebuilding might have been triggered but no theme registry needed to be injected, but now they have to.

Hm. Them I'm very confused, because the last time I looked we wanted to NOT be relying on the placeholder name to avoid name collisions with newly defined entities and use the method signature only, as that's safer. If this is just a step toward that, fine, but I'm pretty sure that was the intent.

Well my intension for the issue is to fix an actual problem (accident upcasting), which is fixed by this issue. We rely on the reflection information anyway.

I spent some quality time with this one now and I must say:
Awesome patch! I really like it. Especially the unit test is beautiful to read, nice work!

I have a bunch of remarks, most of them are very minor and you can also ignore them if you disagree.

I just understood the last few comments after reading the patch and per my comment below, I'm pretty sure it's currently not possible to resolve that @todo in any (sane) way.

1. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
   @@ -0,0 +1,211 @@
   + * Sets the entity parameter converter options automatically.
   + *
   + * If the user specified an interface for the variable the upcasting is done
   + * automatically.
   

   Super minor, but the one-line description is a little bit ambiguous. At least "parameter" -> "route parameter" would be helpful I think.

   The longer description could use some elaboration as well, in my opinion. Maybe something like "If controllers of routes with route parameters, type-hint the parameters with an interface, upcasting is done automatically." ?

2. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
   @@ -0,0 +1,211 @@
   +   * Creates an controller instance out of route defaults.
   

   Minor: "an" -> "a". Maybe also "out of" -> "using"

3. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
   @@ -0,0 +1,211 @@
   +   * By design we cannot support all possible routes, but just the ones which
   +   * use the patterns provided by core.
   

   With "pattern" you mean stuff like _content,_form, _entity_form? I think that could be clarified.

4. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
   @@ -0,0 +1,211 @@
   +   *   Returns the controller instance if it is possible to figure out,
   +   *   otherwise NULL.
   

   Perhaps "... if it was possible to instantiate it; NULL otherwise."

5. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
   @@ -0,0 +1,211 @@
   +          $controller = array(new $form_arg, 'buildForm');
   

   Missing () after $form_arg.

6. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
   @@ -0,0 +1,211 @@
   +   *   The route object.
   ...
   +   *   The route object.
   

   Perhaps "The route object to populate with upcasting information."

7. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
   @@ -0,0 +1,211 @@
   +   *   Returns TRUE if the upcasting parameters could be set, otherwise FALSE.
   

   "...; FALSE otherwise.

8. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
   @@ -0,0 +1,211 @@
   +    $reflection = new \ReflectionMethod($controller[0], $controller[1]);
   

   I know this is more a matter of personal preference, but would you object to list($instance, $method) = $controller; new \ReflectionMethod($instance, $method); ?

9. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
   @@ -0,0 +1,211 @@
   +    $parameters = $reflection->getParameters();
   ...
   +    $parameter_definitions = $route->getOption('parameters') ?: array();
   +    foreach ($parameters as $parameter) {
   

   I had to read this a couple times to realize that the foreach is over $parameters and not $parameter_definitions. I know this is super-nitpicky, but could we re-organize those variable initializations a bit?

10. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
    @@ -0,0 +1,211 @@
    +      // @todo It would be also possible to not rely on the variable name at all
    +      //   so for example also upcast NodeInterface $other_var automatically.
    

    Yes, but for that we really need Interface == $entity_type_id. Right now NodeInterface is type-hinted all over the place (which is good!) but the entity system itself, doesn't know it. It knows 'node' and it knows \Drupal\node\Entity\Node, which happens to implement NodeInterface. I can swap out the class and then make it implement a different interface. If that interface extends NodeInterface then all is good. But there is no programmatic way to figure out the interface for an entity type.

    tl:dr; ++, follow-up

11. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
    @@ -0,0 +1,211 @@
    +      if (isset($entity_types[$parameter_name])) {
    ...
    +        if (!($reflection_class = $parameter->getClass())) {
    +          continue;
    +        }
    

    Aloso again very minor and arguable, but I would find the code flow more legible if the ($reflection_class = $parameter->getClass()) would be moved into the if().

12. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
    @@ -0,0 +1,211 @@
    +          if (!isset($parameter_definitions[$parameter_name])) {
    +            $parameter_definitions[$parameter_name] = array();
    +          }
    ...
    +      if (!isset($parameter_definitions[$entity_type])) {
    +        $parameter_definitions[$entity_type] = array();
    +      }
    

    $parameter_definitions += array($parameter_name => array()); ?

13. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
    @@ -0,0 +1,211 @@
    +   * Sets the upcasting information using _entity_view, _entity_list etc.
    

    Suggestion:
    "Sets the upcasting information using the _entity_* route defaults.

    Supported are the '_entity_view', '_entity_list' and '_entity_form' route defaults."

14. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
    @@ -0,0 +1,211 @@
    +      list($entity_type) = explode('.', $entity_view, 2);
    ...
    +      list($entity_type) = explode('.', $entity_form, 2);
    

    Minor, but the 2 can be omitted (or kept :-), doesn't matter, just saying)

15. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
    @@ -0,0 +1,211 @@
    +          list(, $parameter_entity_type) = explode(':', $info['type'], 2);
    

    Perhaps a comment saying "The parameter types are of the form 'entity:$entity_type'." ?

16. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
    @@ -0,0 +1,211 @@
    +      if ($controller = $this->getController($route->getDefaults())) {
    +        // Try to use reflection.
    +        if ($this->setParametersFromReflection($controller, $route)) {
    +          return;
    +        }
    +      }
    +      // Try to use _entity_view, _entity_list information on the route.
    +      $this->setParametersFromEntityInformation($route);
    

    So if I understand the code correctly, the setParametersFromEntityInformation() is only needed for _entity_*-style controllers which do not utilize type-hinting. I.e. for those entity view/list/form builders that do utilize type-hinting the call is not needed, right?

    If that is true, would it make sense to simply remove support for that, and really force people to use type-hinting even for _entity_*-style controllers?

    I thought about this because if I'm not mistaken the current code unconditionally sets an upcasting parameter named after the entity type for _entity_*-style controllers, while e.g. for _entity_list controllers this will generally not be needed. Now that's certainly not a big problem, but it's also not really nice.

17. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
    @@ -0,0 +1,211 @@
    +    catch (PluginNotFoundException $e) {
    +      // Tests maybe trigger non-existent entity types.
    

    Yay, ++ for typed catching.

    I think the comment could be improved a bit, though. Maybe "Some tests define routes with parameters for non-existant entity types." (Also note that unless I'm mistaken existat is spelled with an a in English).

18. +++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
    @@ -0,0 +1,211 @@
    +  /**
    +   * Returns a list of all entity types.
    +   *
    +   * @return \Drupal\Core\Entity\EntityTypeInterface[]
    +   */
    +  protected function getEntityTypes() {
    +    if (!isset($this->entityTypes)) {
    +      $this->entityTypes = $this->entityManager->getDefinitions();
    +    }
    +    return $this->entityTypes;
    +  }
    
    +++ b/core/lib/Drupal/Core/EventSubscriber/EntityRouteAlterSubscriber.php
    @@ -49,22 +49,8 @@ public function __construct(EntityManagerInterface $entity_manager) {
         foreach ($event->getRouteCollection() as $route) {
    ...
    +      $this->resolverManager->setRouteOptions($route);
         }
    

    It seems this is only being called once in this class (and not within a loop), so I'm curious why you introduced a separate method. Is setRouteOptions() being called multiple times per request?

    Edit: Just saw the code below.

    Still wanted to note that we do still call EntityManager::getDefinition() in setParametersFromEntityInformation(), i.e. where we don't use the cached entity type list.

19. +++ b/core/lib/Drupal/Core/EventSubscriber/EntityRouteAlterSubscriber.php
    @@ -26,20 +26,20 @@
    -   * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager
    ...
    +   * @var \Drupal\Core\Entity\EntityResolverManager
    

    This one was mistaken, it should still be @param.

20. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigEntityMapper.php
    @@ -233,4 +234,57 @@ public function getContextualLinkGroup() {
    +    $parameters = $route->getOption('parameters') ?: array();
    +    $parameters[$this->entityType] = array(
    +      'type' => 'entity:' . $this->entityType,
    +    );
    

    In the EntityResolverManager, you used +=. Is there a specific reason you're using just = here?

21. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigEntityMapper.php
    @@ -233,4 +234,57 @@ public function getContextualLinkGroup() {
    +    return $this->addEntityParameterConversion(parent::getAddRoute());
    ...
    +    return $this->addEntityParameterConversion(parent::getBaseRoute());
    ...
    +    return $this->addEntityParameterConversion(parent::getEditRoute());
    ...
    +    return $this->addEntityParameterConversion(parent::getOverviewRoute());
    ...
    +    return $this->addEntityParameterConversion(parent::getDeleteRoute());
    

    Just a thought, feel free to object: Would it make sense to add a processRoute(Route $route) method to ConfigNamesMapper, that is called in each of the get*Route() methods, but is a no-op in ConfigNamesMapper. That way we would not have to override all those methods. The pattern is the same that is used for populateFromRequest() in that class, if I'm not mistaken.

22. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationListController.php
    @@ -49,35 +39,39 @@ public function __construct(ConfigMapperManagerInterface $mapper_manager, $confi
    +   * @param string $config_translation_mapper
    +   *   The name of the mapper.
    

    I know this is just copied around, but can we rename this to $mapper_id or something while we are at it. This way it is a little bit confusing.

23. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationListController.php
    @@ -49,35 +39,39 @@ public function __construct(ConfigMapperManagerInterface $mapper_manager, $confi
        * @throws \Symfony\Component\HttpKernel\Exception\NotFoundHttpException
    ...
    +   * @return array
    

    AFAIK @throws comes after @return, but not really important either way.

24. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationListController.php
    @@ -49,35 +39,39 @@ public function __construct(ConfigMapperManagerInterface $mapper_manager, $confi
    +  public function listing($config_translation_mapper) {
    

    I like this change, it makes a lot of sense. It turns the class into a true list builder which itself is stateless.

25. +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleContentTest.php
    @@ -150,12 +150,12 @@ function testContentTypeDirLang() {
    -    \Drupal::languageManager()->reset();
    ...
    +    \Drupal::languageManager()->reset();
    
    +++ b/core/modules/node/lib/Drupal/node/Access/NodeAddAccessCheck.php
    +++ b/core/modules/node/lib/Drupal/node/Access/NodeAddAccessCheck.php
    @@ -10,6 +10,7 @@
    

    Wow, that must have been fun to debug...

26. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityResolverManagerTest.php
    @@ -0,0 +1,489 @@
    +/**
    + * Provides a test for the entity resolver.
    + *
    + * @coversDefaultClass \Drupal\Core\Entity\EntityResolverManager
    + */
    

    Can I haz @group Drupal and @group Entity :-)?

27. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityResolverManagerTest.php
    @@ -0,0 +1,489 @@
    +   * The mocked dependency Injection container.
    

    "Injection" -> "injection" (lowercase i)

28. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityResolverManagerTest.php
    @@ -0,0 +1,489 @@
    +   * Tests the setRouteOptions method with no special route/controller.
    

    Here and below as well, I think instead of "the setRouteOptions method" you can just say "setRouteOptions()".

    Maybe instead instead of "special route/controller", "route parameters" is sufficient?

29. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityResolverManagerTest.php
    @@ -0,0 +1,489 @@
    +   * Tests the setRouteOptions method with a controller with a non entity arg.
    

    Doing "the setRouteOptions method" -> "setRouteOptions()" would allow to also do "arg" -> "argument" here.

    Also "non entity" -> "non-entity".

30. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityResolverManagerTest.php
    @@ -0,0 +1,489 @@
    +   * Tests the setRouteOptions method with a _content controller.
    

    Would "controller" -> "default" be more correct? I think controller could be confused with _controller here.

Yes, but for that we really need Interface == $entity_type_id. Right now NodeInterface is type-hinted all over the place (which is good!) but the entity system itself, doesn't know it. It knows 'node' and it knows \Drupal\node\Entity\Node, which happens to implement NodeInterface. I can swap out the class and then make it implement a different interface. If that interface extends NodeInterface then all is good. But there is no programmatic way to figure out the interface for an entity type.

#2266605: Add the entity specific interfaces to the entity type

Minor, but the 2 can be omitted (or kept :-), doesn't matter, just saying)

Tim will force you to specify all the times.

If that is true, would it make sense to simply remove support for that, and really force people to use type-hinting even for _entity_*-style controllers?

I thought about this because if I'm not mistaken the current code unconditionally sets an upcasting parameter named after the entity type for _entity_*-style controllers, while e.g. for _entity_list controllers this will generally not be needed. Now that's certainly not a big problem, but it's also not really nice.

I am pretty sure that there are places all over the place where it is kind of automatic assumed that the upcasting happends. Just have a look at the node_help() in the interdiff. Given that people
don't really like explicit code I think this would block the patch, if the support would be removed.

(Also note that unless I'm mistaken existat is spelled with an a in English).

Mh, both phpstorm and dict.cc disagree with you: http://www.dict.cc/?s=existant

Still wanted to note that we do still call EntityManager::getDefinition() in setParametersFromEntityInformation(), i.e. where we don't use the cached entity type list.

Good catch.

In the EntityResolverManager, you used +=. Is there a specific reason you're using just = here?

Not at all, you brain!

Just a thought, feel free to object: Would it make sense to add a processRoute(Route $route) method to ConfigNamesMapper, that is called in each of the get*Route() methods, but is a no-op in ConfigNamesMapper. That way we would not have to override all those methods. The pattern is the same that is used for populateFromRequest() in that class, if I'm not mistaken.

Yeah, this looks way simpler.

Wow, that must have been fun to debug...

Oh totally

Can I haz @group Drupal and @group Entity :-)?

Sure

Maybe instead instead of "special route/controller", "route parameters" is sufficient?

Sure

Meh

And back to green.

Rerolled.

Reroll

This needed a re-roll due to #2178795: Allow DiscoveryInterface::getDefinition() to throw an exception for an invalid plugin.

I can't really reproduce this fail. Both with native PHPUnit and with run-tests.sh EntityUnitTest passes for me.

Pulling this quote from the test results

Output: [PHP Fatal error: Cannot use Drupal\entity_test\Entity\EntityTest as EntityTest because the name is already in use in /var/lib/drupaltestbot/sites/default/files/checkout/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php on line 13

PS testbot reports fail with php version 5.4 ... tests all green locally for me 5.4.10

PPS

See below now that tstoeckler is done stabbing in the dark ... you can't see this, but this is me "Dancing in the dark" :-)

Stabbing in the dark...

Ok, now with a less silly name.

Btw, I have NFI why that caused the testbot to choke on this.

I think the testbot is just playing with me at this point.

Interdiff is still relative to #166. (Previous one was as well, even though it was incorrectly named.)

173: 1906810-173-upcast-type-hint.patch queued for re-testing.

"SystemListingCompatibleTest test executed." found Other InstallationProfileModuleTestsTest.php 63
fail was from #2239969-22: Session of (UI) test runner leaks into web tests which was reverted.
sending for a retest.

173: 1906810-173-upcast-type-hint.patch queued for re-testing.

Oh that seemed to be a "fun" one ... Rock on tstoeckler!

Yeah. RTBC++. Even though I wasted a lot of time on this :-) my actual code contributions to this issue are minimal (an almost trivial merge and the interdiff in #173) so if @dawehner agrees with the interdiff, this is ready to go!

I'm a bit confused why none of the core entity modules are touched by the patch i.e. NodeViewController still type hints with EntityInterface with the patch applied.

Needs an issue summary update and/or follow-ups being created?

@catch
This would be certainly doable properly once #2028097: Map data types to interfaces to make typed data discoverable is doable/done. Currently there is no simple/relyable way to get the interface from the entity type itself.

173: 1906810-173-upcast-type-hint.patch queued for re-testing.