[Regression] TypeError: filefield_paths_filefield_paths_process_file(): Argument #4 ($instance) must be of type array, null given

[bugfolder]

Updating filefield_paths from version 1.0.2 to version 1.0.3 crashed our Ubercart site at customer checkout. After adding a product to the cart and clicking "Checkout", the following crash ensued:

TypeError: filefield_paths_filefield_paths_process_file(): Argument #4 ($instance) must be of type array, null given, called in /mysite/modules/contrib/filefield_paths/filefield_paths.module on line 383 in filefield_paths_filefield_paths_process_file() (line 43 of /mysite/modules/contrib/filefield_paths/includes/filefield_paths.inc).

The change happened because of the change in function declaration from filefield_paths 1.0.2 to 1.0.3. In 1.0.2, it was:

function filefield_paths_filefield_paths_process_file($type, $entity, $field, $instance, $langcode, &$items)

But in 1.0.3, argument types are declared:

function filefield_paths_filefield_paths_process_file($type, $entity, array $field, array $instance, $langcode, array &$items)

The problem is that $instance can be NULL, and this is what's happening in this case.

That function is called from filefield_paths_field_storage_pre_update(), with the call happening from this loop:

    foreach (field_info_fields() as $field) {
      if (in_array($field['type'], array_keys($field_types))) {
        $files    = array();
        $instance = field_info_instance($entity_type, $field['field_name'], $bundle);
        $enabled  = (isset($instance['settings']['filefield_paths_enabled']) && $instance['settings']['filefield_paths_enabled']) || !isset($instance['settings']['filefield_paths_enabled']);
        if ($enabled && isset($entity->{$field['field_name']}) && is_array($entity->{$field['field_name']})) {
          foreach ($entity->{$field['field_name']} as $langcode => &$deltas) {
            foreach ($deltas as $delta => &$file) {
              // Prepare file.
              if (function_exists($function = "{$field['module']}_field_load")) {
                $items = array(array(&$file));
                $function($entity_type, array($entity), $field, array($instance), $langcode, $items, FIELD_LOAD_CURRENT);
              }
              $files[] = &$file;
            }
            // Invoke hook_filefield_paths_process_file().
            foreach (module_implements('filefield_paths_process_file') as $module) {
              if (function_exists($function = "{$module}_filefield_paths_process_file")) {
                $function($entity_type, $entity, $field, $instance, $langcode, $files);
              }
            }
          }
        }
      }
    }

$instance is set from this line near the top:

        $instance = field_info_instance($entity_type, $field['field_name'], $bundle);

In this case, $entity_type = 'uc_order_product', $field['field_name'] = 'field_image_cache', $bundle = 'uc_order_product'. There are no instances for this entity type, field name and bundle, so field_info_field() properly returns NULL (as it is documented to do).

So, I believe that filefield_paths_field_storage_pre_update() needs to handle the case where $instance = NULL and not try to call filefield_paths_filefield_paths_process_file() (or, really, any other processing inside that loop) when $instance is NULL, now that $instance is declared typed.

Pinging @argiepiano on this, since this is Ubercart- and entity-related.

[indigoxela]

@bugfolder many thanks for reporting.

Looking at function filefield_paths_filefield_paths_process_file()... in case instance is null, there must have been problems before. Just no fatal ones. This now just escalated with the type hint.

I can see the problem, function field_info_instance() can return NULL, if the instance doesn't exist.

... But how in the world do you end up with $enabled evaluating to TRUE? That's the part I simply don't get.

[argiepiano]

I'll try to take a look tomorrow - attending a music conference at the moment and really swamped!

[bugfolder]

... But how in the world do you end up with $enabled evaluating to TRUE? That's the part I simply don't get.

Look at how $enabled is set (scroll all the way right, the key is the last bit on that line):

        $instance = field_info_instance($entity_type, $field['field_name'], $bundle);
        $enabled  = (isset($instance['settings']['filefield_paths_enabled']) && $instance['settings']['filefield_paths_enabled']) || !isset($instance['settings']['filefield_paths_enabled']);

The last bit that gets ||'d in is !isset($instance['settings']['filefield_paths_enabled']), which will always return TRUE if $instance is NULL.

[indigoxela]

Look at how $enabled is set (scroll all the way right, the key is the last bit on that line)...

Wow, now I get it (was a long day yesterday... 😉). Weird condition... (also in the D7 module), I don't get that logic. 🤔

... && $instance['settings']['filefield_paths_enabled']) || !isset($instance['settings']['filefield_paths_enabled'])

Huh?

[indigoxela]

I still don't get the logic of that condition, and decided to jump out of the loop before it.

@bugfolder I never used Ubercart, so I'm relying on your testing here. If you have an idea for a reduced testing scenario in a functional test, I'd be happy to implement. 😉

[indigoxela]

TBH, this whole function filefield_paths_field_storage_pre_update() seems a bit odd. The entity is already known, so also the bundle via $entity->bundle(). That means the field instances are just a field_info_instances() away. But instead it runs a loop over all fields known to the system (field_info_fields), including totally different and unrelated entity types and pseudo fields... 🤷

However, I'll suppress the impulse to refactor this ancient code. I simply don't know the module good enough (in depth).

At least, this PR should make it slightly more robust.

[indigoxela]

@bugfolder the inefficiency of that loop just bugged me too much, so I'd prefer a different approach.

It should still prevent the error you saw, as the loop now only runs over actual field instances on this particular entity - can you confirm the fix? Other ideas?

[bugfolder]

the inefficiency of that loop just bugged me too much, so I'd prefer a different approach.

That seemed weird to me, too. The new PR fixes the problem in Ubercart, thanks!

[bugfolder]

Oh, but I ran into something related. When running unrelated db updates after making this change, I got a lot of warnings of the form

Warning: Trying to access array offset on value of type null in filefield_paths_field_storage_pre_update() (line 367 of /mysite/modules/contrib/filefield_paths/filefield_paths.module).

Running out for a few hours, but I'll try to look into it further later today.

[indigoxela]

... but I'll try to look into it further later today.

Yes, please report any findings.

Function field_info_field() can return null, but I wonder how that can happen, when we're looping over field instances... 🤔 Sounds weird.

[indigoxela]

I wonder how that can happen, when we're looping over field instances.

Maybe I have an idea... There's this core bug re Comment, where field instance files don't get deleted on Comment module uninstall... As this problem's not limited to that core module, it can happen, that some stale instance field config never got wiped out. At least, in theory something like that might be the case for your Backdrop install.
We shouldn't blindly rely on instances still having a proper field definition in place, I guess. 🤔

@bugfolder do you also see nagging, when you go to /admin/config/development/configuration/single/export? That might verify my theory.

Anyway, I updated my PR to also consider (leftover) instance config without field config.

[bugfolder]

Using the previous PR (not your latest update), I narrowed down the warning to it happening during a "cache clear all" (which happens during an update, which is why I saw it there). So manually doing a "cache clear all" spews a whole slew of the warnings.

Instrumenting the code to detect when $field is empty, the warnings are all coming from $entity_type = rules_config (and $bundle = NULL). The entity is a RulesReactionRule. The $entity_info has $fieldable = TRUE, but $instance is an empty array.

I mention all this in case Rules is doing something squirrely; but now off to try our your new PR.

[bugfolder]

With the new PR:

• No warnings resulting from cache-clear.
• No warnings resulting from the db update.
• No crash from Ubercart going from cart to checkout.

So I think we're good!

[indigoxela]

Wow, many thanks for your digging. I wasn't aware that class RulesConfigEntity extends Entity.

Great, that the PR fixes the problem. I merged it. 👍

[argiepiano]

Hello @bugfolder and @indigoxela. I finally have time to look into this. Thank you both for taken care of a fix... but...

There is a fundamental issue with the way Ubercart handles the entity uc_order_product, which is the result of how the D7 maintainers incorporated entities in a "half-baked" way when they were first introduced in D7. In short, uc_order_product has no fields by default. So, it's correct that field_info_instances('uc_order_product', 'uc_order_product') return null, because the entity should have no fields. You can verify this by going to the Manage field UI that the Field API provides, at admin/store/settings/orders/products/fields.

If I remember correctly (I have to check this), the uc_order_product entity is built during the checkout process by cloning the product's node entity, which contains all those fields. The cloning is done rather carelessly, and the node entity's field structure is also assigned to uc_order_product. This should not be done this way. uc_order_product should have no fields at all, which is why instance returns null.

So, @indigoxela was correct to suspect that there was something wrong to begin with. The fix is masking this problem, but this is likely to create issues with more contribs that act upon instances of fields for entities.

@bugfolder, I'll try to dig a bit more on this in Ubercart, and will create an issue there.

Additional clarification: uc_order_product IS fieldable, but has no fields by default. A quick workaround IF this had not been fixed in this issue for filefield_path, would be to go to the Manage fields UI for uc_order_product at admin/store/settings/orders/products/fields and manually add the same fields present in the Product node type.

[argiepiano]

To further clarify what I'm saying here: I think the fix for filefield_paths in this issue is correct and makes sense. My comments above referred mostly to the fact that the entity object passed to filefield_paths_field_storage_pre_update() contained those field array structures (incorrectly), and that was causing the issue with the "old" approach in filefield_paths_field_storage_pre_update().

What I'm trying to say is that the uc_order_product entity object should not have those field arrays in the first place. But that's an Ubercart problem, no a filefield_paths one.

[indigoxela]

Hey @argiepiano, yeah, many thanks for confirming my initial suspicion. 😉

The weird (possibly questionable) things that the Ubercart and Rules modules do, helped to clean up some undoubtedly inefficient code in filefield_paths. So this bug report provided benefit here.

I'd suggest, a more in-depth discussion re entity handling should continue in the other issue queues.

[bugfolder]

I'll try to dig a bit more on this in Ubercart, and will create an issue there.

I'll watch for it (and won't distract this issue further), but will mention that in my initial investigation, I found that UcOrderProduct clones its properties from a UcCartItem.