After all foreach loops that reference the value (e.g. => &$value) unset the value variable afterward

I'm failing to see the issue here, especially since you didn't really describe what went wrong when you called that function twice. : D

Calling it multiple times in the same page request doesn't mess up anything for me - the output looks the same each time. That => &$... is pretty typical PHP allowing my to manipulate the value array by reference, and the $payment_method variable is never accessed again in that scope after the loop completes.

Feel free to reopen if you can let me know what's going wrong and how I can reproduce it, but for not I just don't see anything here that isn't working as designed.

Ok, I see. I think this is pretty poor behavior on PHP's part, especially considering we're in a completely different scope. I don't understand how they could think that's not a bug - what other locally declared variable persists outside of its scope, or am I missing something in how that final $payment_method value ends up getting truncated?

In any event, if this is the screwy way PHP is working, we need a general task in Commerce to apply this same rule to every single foreach loop where we reference the value. Resolving that task will fix your issue as well, and in the meantime you can put commerce_payment_methods_reset() between your function calls if you don't want to maintain a patched version of that module.

I added the unset for anything using => &whatever, for everything except the .install files which didn't seem necessary, although if anyone feels it would be good practice to do so, I can update the patch.

I'm not positive, but I believe the issue is more subtle than the title describes. The problem is that commerce_payment_method_get_title() calls commerce_payment_methods() which returns a referenced variable, then commerce_payment_method_get_title() borks the $payment_methods array by modifying it directly.

Since the commerce_payment_method_get_title() function is actually attempting to return a completely new subset of data, I've attached a patch which avoids changing the values of the $payment_methods by creating a new array.

Any feedback is appreciated.

daggerhart -- I like your approach, your patch did include an extra whitespace on the empty line between array definition and foreach loop.

smccabe -- I think this is the approach requested.

I assume Matt or Ryan will need to review which direction they want to go after the tests come back with a "yay" or "nay"

After further looking around, I'm going to walk my previous comment back so far as to say it is just wrong (in a lot of ways).

The PHP issue linked to definitely indicates a discrepancy in a programmer's expectations vs the result when dealing with static variables with references to values of a loop. OP was completely correct in his assessment. Only the last item in the array remains a reference, and that is a weirdness I'd rather avoid. I believe the correct approach is that of @smccabe, or to avoid the reference all together.

I've attached a patch which avoids the use of the reference completely in case that is the desired approach.

Attached patch removes all reference values in loops involving static variables, as opposed to #7 which only corrects one instance of the issue.

I agree daggerhart, I think that these functions don't actually require references at all and removing them is more elegant than just mashing an unset() at the bottom.

Do you think there is any advantage to writing tests for these? we could call them twice and compare the return data. It does seem like quite the edge case though.

One small note about naming mentioned below.

+++ b/modules/price/commerce_price.module
@@ -885,10 +885,10 @@ function commerce_price_component_types() {
-    foreach ($component_types as $name => &$component_type) {
-      $component_type += array(
+    foreach ($component_types as $name => $type) {
+      $component_types[$name] = $type + array(
         'name' => $name,
-        'display_title' => $component_type['title'],
+        'display_title' => $type['title'],

Why did you change it from component_type to just type? It doesn't seem the need 2 variables like the two previous changed functions

@smccabe - Whoops. No reason to change the variable name on that last one, I was just in the groove of things. Happy to re-roll with the original variable name if you'd like.

@daggerhart

it is probably a little cleaner that way, but it's probably a maintainers call, not mine. Otherwise I think the patch is good though! Where you able to run a test calling those functions twice and confirm the output is the same?

@smccabe - Yes, I was running a test similar to OP's.

dsm(commerce_payment_method_get_title());
dsm(commerce_payment_method_get_title());

Without the changes, the final item in the array is incorrect. Once the reference is removed everything works as it should.

I vote this as good to go then, someone with more clout butt in if you think this needs more testing.

It looks like #8 is the winner. I'd like to commit this. However, it looks easily reproducible that we could get a test added. I would like to see a test written that helps ensure data integrity before/after with patch.

Talked with smccabe about this in IRC. There isn't a way to test this as there is only one line item type and example payment method provided. We'd have to introduce a test submodule or something. Marking for manual testing.