I'm getting PHP 5.3 Strict Warning in various places. It appears that array_shift() want to be passed a variable, not the result of a function.

Here are some locations where these are popping up.

Error at: admin/commerce/products
Fixed with: #976106: Views errors before content is added

Error at: admin/commerce/products/add/product

Strict warning: Only variables should be passed by reference in commerce_price_field_widget_form() (line 292 of d7\sites\all\modules\commerce\modules\price\commerce_price.module).

See patch below.

Please add more as you find them.

CommentFileSizeAuthor
#7 commerce-987624-4.patch14.55 KBbendiy
#5 commerce-987624-4.patch14.55 KBbendiy
#6 commerce-987624-4.patch14.55 KBbendiy
#4 commerce-987624-4.patch14.55 KBbendiy
#3 commerce-987624-3.patch7.53 KBbendiy
#2 commerce-987624-2.patch902 bytesbendiy
#1 commerce-987624-1.patch857 bytesbendiy

Comments

bendiy’s picture

bendiy commented
StatusFileSize
new commerce-987624-1.patch857 bytes

Here's the patch for one of these.

bendiy’s picture

bendiy commented
StatusFileSize
new commerce-987624-2.patch902 bytes

Here's another with a patch for it:

Strict warning: Only variables should be passed by reference in commerce_cart_add_to_cart_form() (line 506 of d7\sites\all\modules\commerce\modules\cart\commerce_cart.module).
bendiy’s picture

bendiy commented
Status: Needs work » Needs review
StatusFileSize
new commerce-987624-3.patch7.53 KB

Or I could just do a search and fix them all... ;)

The attached patch fixes the above errors #0, #1, #2 and a few other places with this issue.

bendiy’s picture

bendiy commented
StatusFileSize
new commerce-987624-4.patch14.55 KB

Here's a bunch more. I've combined #3 above with all the others I ran into when walking though "Add to Cart" > Checkout > Payment > Review.

bendiy’s picture

bendiy commented
StatusFileSize
new commerce-987624-4.patch14.55 KB

ignore

bendiy’s picture

bendiy commented
StatusFileSize
new commerce-987624-4.patch14.55 KB

ignore

bendiy’s picture

bendiy commented
StatusFileSize
new commerce-987624-4.patch14.55 KB

ignore
Sorry I think d.o froze.

rszrama’s picture

rszrama commented

Given how much I'm using the array_shift(array_keys($array)) bit of code, I wonder if it wouldn't be better to just define a helper function in commerce.module instead of splitting them all apart?

Something like:

/**
 * Returns the first key of the given array.
 */
function commerce_array_keys_shift($array) {
  $keys = array_keys($array);
  return array_shift($keys);
}
bendiy’s picture

bendiy commented

From a readability standpoint, that would imply that the Commerce module is doing something unique with a special array_shift function. It isn't. I understand the reason for "don't repeat yourself", and a custom function will satisfy that rule, but this problem is really just a coding standard paradigm shift in PHP 5.3. You will see more people coding like this in the future:

$options_keys = array_keys($options);
$options = $options[array_shift($options_keys)];

Yes it's one more line of code, but I think a custom functions will be more confusing:

$options = $options[commerce_array_keys_shift($options)];

When I see that, I think, what's going on in this commerce_array_keys_shift($array) function and why couldn't you just use the default on in PHP.

Here's an explanation from PHP's Rasmus himself:

The explanation really is as simple as the warning indicates. You can only pass variables by reference. The type of the variable is irrelevant to this. If you pass something by reference that isn't a variable, then the called function has no place to store its modifications. It worked in old versions of PHP without a warning, but it actually led to memory corruptions in some cases, so technically this has never worked.

http://bugs.php.net/bug.php?id=48937

rszrama’s picture

rszrama commented

I understand the way the references are working - it's an unfortunate change, and it affects any inline workaround we can come up with. I'm not just concerned about the number of lines but also readability, and the split approach will result in a lot of odd single use variables. I don't think it's unprecedented to write a helper function for common functions... I'd be happy to find a better name; that was just an example. : )

rszrama’s picture

rszrama commented

Chatted with Damien and he recommends the key() function anyways, which in most cases shouldn't require a reset() but in some cases will still require us creating an array based on the return value of a function beforehand.

bendiy’s picture

bendiy commented

You are right on about the "odd single use variables". I struggled to find a good name for some of them e.g. $commerce_checkout_pages_keys.

I think the helper function name is fine. It just seemed silly to me to have to do this for something so simple.

I'm sure other modules are running into this. What are they doing? A helper function this basic should be in core because it will be used by many other modules.

rszrama’s picture

rszrama commented
Component: Commerce » Developer experience
Status: Needs review » Fixed

Found an example of core using the reset() / key() approach:

  if (!isset($toolkits[$toolkit]) || !function_exists('image_' . $toolkit . '_load')) {
    // The selected toolkit isn't available so return the first one found. If
    // none are available this will return FALSE.
    reset($toolkits);
    $toolkit = key($toolkits);
  }

That's good enough for me, and it's really the right approach for us. Based on your patch, I've implemented this approach for the areas you found. I only used reset() where it seemed necessary.

https://github.com/rszrama/drupalcommerce/commit/24f0d4bfd60549ff910a06e...

Status: Fixed » Closed (fixed)

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