Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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 | 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 |
phpconverter_rawvalue_reference.patch | 3.06 KB | neclimdul | |
Comments
Comment #1
neclimdulrun tests.
Comment #2
tim.plunkettI 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.plunkettEr, meant to do this.
Issue #2103145 by neclimdul, chx, dawehner: ParamaterConverter mangles raw values.
Comment #4
dawehner+1
Comment #5
chx CreditAttribution: chx commented// foreach copies the values from the array it iterates even if they are references so use it to break references.
Comment #6
tim.plunkettCombining that and my docs additions as discussed in IRC.
Comment #7
catchFrom 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.
Comment #9
damiankloip CreditAttribution: damiankloip commentedHow about this?
Comment #10
neclimdulI was thinking more http://24.media.tumblr.com/b7cd62f4c9fd440bc0b105dc7ba2275f/tumblr_mpfqk... but that works.
Comment #11
catchOK that's great. And the test means it shouldn't get broken again anyway.
Committed/pushed to 8.x, thanks!
Comment #12.0
(not verified) CreditAttribution: commentedUpdated issue summary.