When trying to cache a page that has an Ubercart add to cart form on it I get an "Invalid argument supplied for foreach()" error. One of the form's child indexes is simply a NULL value. As a result the page won't cache due to the error.

I traced it back to authcache_form_value_suspect() which runs down element children recursively, when it's going through the element children it's assuming the element is always an array. In the case of this one NULL index it throws that error.

CommentFileSizeAuthor
#9 2885086.patch502 bytestr
#2 check_to_make_sure-2885086-2.patch538 bytesswickham

Comments

swickham created an issue. See original summary.

swickham’s picture

swickham commented
Status: Active » Needs review
StatusFileSize
new check_to_make_sure-2885086-2.patch538 bytes

The attached patch checks to make sure the element even has any values. If not, there's no need to check that it's suspicious.

znerol’s picture

znerol commented

Thanks for reporting the issue. Could you maybe post an example of the actual form structure? I'm not sure whether I follow the description. Do you see $key == NULL or $element == NULL? Also do you have a chance to figure out where that form comes from?

swickham’s picture

swickham commented

I mean in the recursive call $element[$key] == NULL.

This particular form is an Ubercart add to cart form. It has an index called "attributes" defined as NULL for whatever reason rather than it just not being defined at all.

Will get back to this shortly with the actual element values when I'm home and can debug it again to get the values.

swickham’s picture

swickham commented

Okay back home and caught it with XDebug again. It's basically like I said above. An index is defined as NULL for whatever reason and that's what it gets caught on. So the loop at the end of the function passed on that NULL value and the function assumes it's always being passed an array. So when the nested call tries to loop through this $element with a value of NULL it triggers the loop error which halts any attempt at caching.

Probably more info than necessary but here's the top level values of the Ubercart add to cart form which is sent as the initial element, the index causing the issues is the fifth one down:

Array
(
    [nid] => Array
    [qty] => Array
    [actions] => Array
    [node] => Array
    [attributes] => NULL
    [#form_id] => uc_product_add_to_cart_form_39
    [#type] => form
    [#build_id] => form-pQth454oiWpL24VTfahCGUpdCBx9hx4KQ-oHaXFZx1M
    [form_build_id] => Array
    [#token] => uc_product_add_to_cart_form_39
    [form_token] => Array
    [form_id] => Array
    [#id] => uc-product-add-to-cart-form-39
    [#method] => post
    [#action] => /page-path
    [#theme_wrappers] => Array
    [#tree] => NULL
    [#parents] => Array
    [#validate] => Array
    [#submit] => Array
    [#theme] => Array
    [#after_build] => Array
)
znerol’s picture

znerol commented

I think that the attributes key comes from _uc_attribute_alter_form() called from uc_attribute_uc_form_alter():

319  function uc_attribute_uc_form_alter(&$form, &$form_state, $form_id) {

[...]

332        foreach (element_children($element) as $key) {
333          $element[$key]['attributes'] = _uc_attribute_alter_form($id . '-' . $key, $node->products[$key], $use_ajax);

[...]

343        $form['attributes'] = _uc_attribute_alter_form($id, $node, $use_ajax);

1273 function _uc_attribute_alter_form($id, &$product, $use_ajax) {
1274   // If the product doesn't have attributes, return the form as it is.
1275   if (empty($product->attributes) || !is_array($product->attributes)) {
1276     return NULL;
1277   }

The problem is that the core element_children function expects that any sub-element is an array (i.e. all entries in the form-array whose key does not start with a #). As a result any code which implements the same pattern as authcache and loops through form elements using element_children is affected by the same problem. In my opinion this should be fixed in ubercart rather than in authcache. Adding code over here only hides that potential bug in the other module.

znerol’s picture

znerol commented
Status: Needs review » Postponed

Filed #2885388: The attributes form element must not be NULL over in ubercart. Postponing until the other one is resolved.

swickham’s picture

swickham commented

I absolutely agree it should be fixed in Ubercart, but I don't understand why there's more concern over not masking someone else's bugs than not allowing them to stop Authcache from working?

I just checked now and see other modules implementing a foreach loop of element_children run the same kind of check to make sure their own functionality isn't held back by the quality of other modules they have no control over.

tr’s picture

tr commented
Status: Postponed » Needs review
StatusFileSize
new 2885086.patch502 bytes

Why not just cast $element[$key] to be an array if you expect it / need it to be an array? No conditional necessary then ... In the weakly (poorly) typed language of PHP, NULL is a valid value for an array type variable. A cast would be you enforcing a condition that you know/expect to be true as opposed to 'protecting' yourself from other's code which might not have the same assumptions as yours. Patch attached.

The *requirement* (enforced by core and core tests) that a render array (not Form API array ...) child *must* itself be a renderable array is not imposed in D7, only in D8. D7 simply says the render array child *should* be an array, but D7 core deliberately handles it gracefully if it's not (and as I said, PHP will treat NULL as an array if it knows you need an array). And D6 said it could be anything you want. The requirement was specifically not backported to D7 due to concerns that it would break contrib - read webchick's comments in the issue where this was done: #1283892-26: Let Render API fail in a tale-telling way on invalid $element and #1283892-29: Let Render API fail in a tale-telling way on invalid $element.

And I have the same concern - Ubercart has used this structure for 10 years and this is the first report we've seen where it causes a problem. I will certainly fix that in D8 (although I haven't encountered a problem with this in D8 either ...) but Ubercart has literally thousands of contrib modules, and themeing of the product page (along with associated attributes) is something almost every site using Ubercat does - changing this in a D7 point release 6 years into the D7 cycle will possibly affect thousands of sites, and doesn't seem appropriate. Not changing this in D7 is also how core has handled similar problems, e.g. #1285098: Toolbar Drawer uses invalid Render API key.

Status: Needs review » Needs work

The last submitted patch, 9: 2885086.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

znerol’s picture

znerol commented
Status: Needs work » Postponed

Authcache refuses to store pages in the cache if the error-handler was triggered during a page build. This forces people to actually fix their code or find the cause in contrib modules. element_children() is heavily used in core and contrib for both, render arrays and form elements. I've only looked at some of those instances, but it is quite clear that not all of them have safeguards against invalid elements. I wouldn't be surprised if there are other sites suffering from the NULL form element introduced by ubercart, but since people tend to simply ignore/silence those things without suffering any consequences nobody cares to report.

Thanks for taking the time to reply here and for pointing out the issues, great research!

I'm tentatively set this to postponed again, I'm not planning to put any efforts into this issue.

znerol’s picture

znerol commented
Status: Postponed » Closed (duplicate)

Got bored in the train, posted a fix in #2856838: Add support for Ubercart.