Not sure if this is php version specific but through some tricks of references raw_variables get the converted values changed. The attached patch explicitly copies the array into a new array. This breaks references, try running php -r '$a = 1; $b = array(&$a); foreach ($b as $v) $v++; var_dump($a);' and see that despite $b[0] is a reference to $a, when foreaching over it, the reference is not preserved. Tests to confirm bug and fix.

Thanks to @dawehner for the test and figuring out where the bug was and @chx and @eclipsegc for talking it through and helping us figure out what was causing it.

CommentFileSizeAuthor
#9 2103145-8.patch3.49 KBdamiankloip
#9 interdiff-2103145-8.txt1.03 KBdamiankloip
#6 interdiff.txt2.1 KBtim.plunkett
#6 MANGLES-2103145-6.patch3.41 KBtim.plunkett
phpconverter_rawvalue_reference.patch3.06 KBneclimdul

Comments

neclimdul’s picture

neclimdul
He/Him
commented
Status: Active » Needs review

run tests.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Priority: Normal » Major
Issue tags: +WSCCI

I hit this while working on #2091691: Convert test non-form page callbacks to routes and controllers. Very confusing, glad it wasn't me.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

Er, meant to do this.

Issue #2103145 by neclimdul, chx, dawehner: ParamaterConverter mangles raw values.

dawehner’s picture

dawehner
Primary language German
commented

+1

chx’s picture

chx commented 
+++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.phpundefined
@@ -156,7 +156,12 @@ public function enhance(array $defaults, Request $request) {
+    // Explicitly copy values to break references.

// foreach copies the values from the array it iterates even if they are references so use it to break references.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new MANGLES-2103145-6.patch3.41 KB
new interdiff.txt2.1 KB

Combining that and my docs additions as discussed in IRC.

catch’s picture

catch
he/him
Primary language English
commented
Title: ParamaterConverter mangles raw values » ParameterConverter mangles raw values
Status: Reviewed & tested by the community » Needs work

From the comment I don't understand what's going to happen if we don't break those references - it'd be good to expand that a bit with an extra sentence or two to explain that the raw values end up getting replaced with the converted values if we don't do this. Bug took a long time to track down so worth adding as verbose docs as possible.

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2103145-8.txt1.03 KB
new 2103145-8.patch3.49 KB

How about this?

neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs review » Reviewed & tested by the community

I was thinking more http://24.media.tumblr.com/b7cd62f4c9fd440bc0b105dc7ba2275f/tumblr_mpfqk... but that works.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

OK that's great. And the test means it shouldn't get broken again anyway.

Committed/pushed to 8.x, thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Updated issue summary.