ParameterConverter mangles raw values [#2103145]

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.

Comment	File	Size	Author
#9	2103145-8.patch	3.49 KB	damiankloip
#9
#9	interdiff-2103145-8.txt	1.03 KB	damiankloip
#6	interdiff.txt	2.1 KB	tim.plunkett
#6	MANGLES-2103145-6.patch	3.41 KB	tim.plunkett
#6
	phpconverter_rawvalue_reference.patch	3.06 KB	neclimdul

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

neclimdul

He/Him

CreditAttribution: neclimdul commented 2 October 2013 at 16:45

Status:

Active

» Needs review

run tests.

Comment #2

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 2 October 2013 at 20:49

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.

Comment #3

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 2 October 2013 at 20:50

Status:

Needs review

» Reviewed & tested by the community

Er, meant to do this.

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

Comment #4

dawehner

German

CreditAttribution: dawehner commented 2 October 2013 at 22:47

Comment #5

chx CreditAttribution: chx commented 4 October 2013 at 05:27

+++ 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.

Comment #6

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 4 October 2013 at 05:32

File	Size
MANGLES-2103145-6.patch	3.41 KB

interdiff.txt	2.1 KB

Combining that and my docs additions as discussed in IRC.

Comment #7

catch

he/him

English

CreditAttribution: catch commented 4 October 2013 at 10:32

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.