When replacing the upcasted values in the request attributes array, retain the original raw value too

I did the same on my typed data resolver sandbox as part of the ParamConverter refactoring/cleanup (including some other things). So yeah, we _really_ don't want to override our request attributes like we do now and instead put the upcast values somewhere else so we can still retrieve the original values.

+1

We can either commit that here (if green and after a good review) or together with the param converter refactoring.

We discussed this during the WSCCI meeting yesterday and agreed that it is necessary to solve this somehow. I am still wondering if it's better to keep a copy of the raw values somewhere and continue to do the upcasting by overriding in the actual attributes array or do what this patch does now (adding another ParameterBag to the attributes where we put the converted values).

Adding a follow-up for discussing whether it might be better to instead store the raw values somewhere else and keep doing these overrides.

#2034857: What to do with raw request attributes vs. converted (upcast) attributes

Let's proceed with this issue though as it helps to unblock/cleanup other things that rely on UrlGenerator (local actions, tasks, etc).

Judging by the latest replies on the other issue I guess we now agreed on storing a copy of the raw values instead. I like that much better. I have got a patch for that on my office pc waiting to be uploaded. Will post it here in the morning.

Storing a backup of the raw values seems much simpler indeed. What about something like this? I believe the ParamConverter is the right place to store the backup as it also knows which things were converted.

I made sure this patch touches as few files as possible and thus also reverted the architectural changes made to the ParamConverterManager previously in this issue. We are doing those in the DX issue anyways.

Thanks for the review.

Should be green now.

+++ b/core/lib/Drupal/Core/Controller/ControllerResolver.php
@@ -98,4 +99,27 @@ protected function createController($controller) {
+        // Use the raw value if a parameter has no typehint.

Are you sure about that? Why wouldn't I want the object just because I have no type hint? It could be a highly variable object in some cases.

(You're free to push back and say that in that case you can bloody well upcast it yourself if you feel that's appropriate.)

Eh, never mind. :-) Crosspost.

Are you sure about that? Why wouldn't I want the object just because I have no type hint? It could be a highly variable object in some cases.

(You're free to push back and say that in that case you can bloody well upcast it yourself if you feel that's appropriate.)

Oh, we are still doing upcasting, even if there is no type hint. It's just that we do not pass the upcasted object in the argument if there is no typehint. You can still pull it from the request if you wanted to:

public function mySuperGenericController($foo) {
  $object = is_object($foo); // This is FALSE
  $object = is_object($request->attributes->get('foo')); // This MAY be TRUE
}

So yeah, I am pretty sure it's okay to do this. What are you going to do in your controller if you don't even know that it's a object of some sort? How are you going to interact with it? There is just a limited number of things you can do without knowing what object you are actually working with (if we are speaking about realistic, real-world use-cases).

The issue summary says...

@TODO Better summary.

Also...

+++ b/core/tests/Drupal/Tests/Core/Controller/ControllerResolverTest.phpundefined
@@ -0,0 +1,76 @@
+    $this->assertEquals($mock_entity, $arguments[0], 'Type hinted variables used no upcasted values.');
+    $this->assertEquals(1, $arguments[1], 'Not type hinted variables used upcasted values.');

The messages here are incorrect. They should be saying what we expect to happen because when it fails, the statement fails to be true.

Updated issue summary.

fixing title

blub

Is _raw really the right key to use here?

Also, would it make sense to add the UID from the user object as the _raw 'account' value (the new global user object)?

per later discussion - maybe not sensible to sick account in there globally - individual consumers can handle that.

Peter suggested we should go with a colon instead. I have no strong preference. I just feel that our request attributes are getting quite messy quickly and we seem to dump unrelated stuff in there all the time. We really need to find a solution for that.

Also @see #2043781: Drupal::request()->attributes->get('account') may conflict with an account object loaded from the path ... Or think about HtmlFormController (form_state / form). Ugh

Let's find a convention. Or even better, find some other place for this stuff.

Motivation for colon is that is something that should never appear in a variable taken from the URI http://tools.ietf.org/html/rfc6570

+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.php
@@ -68,17 +69,23 @@ public function enhance(array $defaults, Request $request) {
 
+    // Keep the copies the raw values of things that were converted.
+    $backup = array_intersect_key($backup, array_flip($converted));
+    $defaults[':raw'] = new ParameterBag($backup);

I don't think the logic is quite right here - we should keep the value in :raw even if it was not converted, otherwise we have to go hunting for it later.

per discussion with fubhy in IRC - this keeps a copy of all the raw values that are names of variables in the route path pattern.

This should make it much simpler to supply all the needed parameters for the route e.g. for the URL generator.

Can we use _raw instead of :raw?

@jibran - why? We should use something that can never be a path variable name.

#32: 2031487-32.patch queued for re-testing.

We should go back to _ actually now that I thought about it more. It gives us the possibility to get it passed as $_raw.

@dawhhner - the assertion is that some controller would want that instead of the whole request. I can see both sides of that argument - and I dont' really liek the symfony _ prefix habbit.

Nevertheless, _-prefixing for "Special" is the Symfony convention and the convention we're already using in a bunch of places. I am very -1 on introducing more special characters. We're not Perl. :-)

Agreed.

Let's get on with it then.

Patch no longer applies after the parameter router patch. Needs a quick re-roll.

Re-roll

I think "_raw" is too vague. This converts it to "_raw_variables" since these are the raw values corresponding to the Route variables (i.e. path slugs like {node})

elaborate use case

#48: 2031487-48.patch queued for re-testing.

#48: 2031487-48.patch queued for re-testing.

hmm this was a real fail:

( ! ) Fatal error: Call to a member function compile() on a non-object in /Users/Shared/www/drupal-8/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.php on line 157

Looks like fubhy's last re-roll removed the $route variable by mistake. This puts it back.

RTBC as per #49

Not 100% about raw variables, but seems better than _raw. Committed/pushed to 8.x

Will need a change notice.

Change notice is fine