Fatal error produced by default rule configuration upon D7 upgrade

[argiepiano]

How to reproduce:

• Create a simple site D7 with ubercart enabled
• Upgrade to Backdrop, using PHP 8.1
• The upgrade fails

I've traced the issue to uc_order_default_rules_configuration(). This function gets called as part of cache clearing of system_update_1079(). The function is there to create a default uc order rule "E-mail an order update notification".

This rule definition makes a call to filter_default_format(), which in turn calls filter_formats() with the account object for user 0 (anonymous). Line 842 of filter_formats() tries to get the array element $formats['user'][0]['enabled'], but for some reason this array element is not yet set, resulting on a fatal error.

I haven't had the time to investigate why that element is not there. I suspect this can't yet be set in the loop in lines 826 at this stage of the upgrade.

Supposedly, 7.4 should not fatal out, but I don't have the time to test this. Reporting this in case someone can check into it.

[argiepiano]

Confirming that PHP 7.4 does not produce a fatal error - rather a warning:

`Notice: Undefined index: enabled in filter_formats() (line 842 of /Users/XX/Sites/localhost/panettone/core/modules/filter/filter.module).

Warning: reset() expects parameter 1 to be array, null given in filter_default_format() (line 1003 of /Users/XX/Sites/localhost/panettone/core/modules/filter/filter.module).

Notice: Trying to get property 'format' of non-object in filter_default_format() (line 1004 of /Users/XX/Sites/localhost/panettone/core/modules/filter/filter.module).

Warning: Invalid argument supplied for foreach() in views_update_1005() (line 160 of /Users/XX/Sites/localhost/panettone/core/modules/views/views.install).`

And: Debug: 'Skipping broken view ' in views_get_applicable_views() (line 1278 of /Users/XX/Sites/localhost/panettone/core/modules/views/views.module).

[bugfolder]

Just took a look in the debugger with some breakpoints. During the upgrade, system_update_1079() gets called before filter_update_1000(), which is the call that converts db filter formats to config filter formats. So no config formats exist yet during system_update_1079(), which is why filter_formats() returns an empty list, not even containing the default filter format.

[bugfolder]

Adding a little data. I tried the obvious (though risky) thing of using hook_update_dependences() to force the relative order of those two update calls, but, perhaps not surprisingly, that led to a different crash during the update process related to the user module.

Perhaps instead, uc_order_default_rules_configuration() should check for existence of a default filter format, and if there is none, return an empty array; and then add another UC update hook to clear the rules cache at the end of the update process, to force a re-load of the default rule.

[bugfolder]

Yes, that works. I add the following to the beginning of uc_order_default_rules_configuration():

  // During D7->B upgrade, there may be no filter formats when this gets called.
  $config_names = config_get_names_with_prefix('filter.format.');
  if (empty($config_names)) {
    return array();
  }

The reason for using $config_names instead of filter_formats() is that the latter will issue some warnings if conversion hasn't happened; using $config_names gives a lower-level check of the existence of converted filter formats.

Incidentally, I didn't add a cache-clear explicitly, but in order to see if the default rule existed, I needed to enable the Rules UI module, which, of course, does a cache-clear as part of enablement, and since filter formats existed by then, the rule showed up.

So I think we have a solution.

[bugfolder]

It might be more performant to first check for the existence of cached filter formats to avoid the scandir() that's part of config_get_names_with_prefix().

[argiepiano]

Thanks for all this, @bugfolder. I'm on the road for a concert at the moment, but I'll try to check on this on Sunday.

[bugfolder]

I've filed a PR with the fix described above. With this in place, I am able to upgrade a D7 UC installation to B with no crash along the way. (I do end up with 4 broken UC views, but I don't think those are related.) And the offending default rule still ends up visible and enabled at the end of the upgrade.

It bugs me that in this implementation, we end up doing the filter formats check on every call to uc_order_default_rules_configuration () forever, even though it's only relevant during the initial upgrade, but at this point I don't see a good alternative.

[bugfolder]

I just remembered that as part of a D7->B upgrade, one needs to move file-based views into the db before running the upgrade. That explains the broken views. So that was unrelated, and so I'm pretty sure this PR fixes the issue. (Whether there's a more performant solution is perhaps still an open question.)

[bugfolder]

Just confirming: running drush ev 'foreach (views_get_all_views() as $view) {$view->save();}' before performing the upgrade eliminated all the broken views and the upgrade proceeded fully with this PR.

[argiepiano]

Hi @bugfolder. This is indeed a complex problem to solve. A few thoughts:

• We'll have the same issues with other default rules configurations that use filter_default_format(), for example, uc_roles_default_rules_configuration() and uc_file_default_rules_configuration(), if those submodules were enabled in the D7 site, so, any fix needs to also be implemented there.
• The problem goes beyond Ubercart. Any module that implements hook_default_ENTITY_TYPE (an undocumented hook, also defined in the key default hook in hook_entity_info()) has the potential to make a call at the "wrong" time during a D7 upgrade
• So, perhaps the fix (conditional check before running) could be done at the level of entity_plus_defaults_rebuild() (which eventually invokes those hooks) as opposed to on the implementations. Meaning that your PR could be applied at a higher level, to that entity_plus function, instead of the lower level in ubercart or even rules.

I agree that having that filter format check on every single cache clearing is not ideal. But it seems that this is probably a good solution at this point.

Other solutions we may consider:

• Simply hard-code plain_text instead of invoking filter_default_format() in the format for this rule configuration, and those other ubercart rules configurations I pasted above. It won't affect the format selected in the D7 rule, since the rule already existed in the database in the D7 site. It WILL affect new installations of ubercart, which will have plain_text as the email format upon installation (but this is mitigated by the fact that the default content for those emails IS plain text and has no markup).
• Try to find another, more performant check (rather than check for the existence of the filter). A few that I've considered:
  • During the D7 upgrade, the global $user is anonymous, meaning that we could check for this before running the rule default code. The only drawback is that, if a site allows anonymous to clear caches and possibly install new modules (extremely unlikely), then this rule configuration will never run and the rule will never be created
  • Another option is to do a backtrace_debug, and to make it as performant as possible by using the following:

$trace = debug_backtrace(!DEBUG_BACKTRACE_PROVIDE_OBJECT|DEBUG_BACKTRACE_IGNORE_ARGS,9);

This is a faster way, since it doesn't load objects or arguments, and only gets the last 9 functions in the stack. Then we could check for if($trace[8]['function'] == 'system_update_1079) {return;}'. This is the most fool-proof way to check that the call is originating from that system update hook

• Finally, a fast (probably faster than above) is to check for the constant MAINTENANCE_MODE which is set up to update only during update processes (unintuitively, this constant is not set up during maintenance mode!). The drawback of this is that the hook will never run during legitimate update.php requests - but this is mitigated by the fact that they WILL run upon installation of modules and other "regular" cache clearing.

Not sure which may work best. The backtrace may be faster than the check for filters, though.

[bugfolder]

Simply hard-code plain_text instead of invoking filter_default_format() in the format for this rule configuration, and those other ubercart rules configurations I pasted above.

I think that would be problematic if the D7 site doesn't have a plain_text format. For example, my current B site, formerly a D7 site, has numerical names for its older filter formats, including the plain text (whose machine name is 6).

So, perhaps the fix (conditional check before running) could be done at the level of entity_plus_defaults_rebuild()...

That seems reasonable; the lower-level and more broadly that one could apply the check, the better.

Another option is to do a backtrace_debug,...

Wow, that sounds a little bit hacky, but I can't think of any specific reason why it would be bad. Although perhaps we should check the entire stack, in case there is an future code update that inserts another call level in the call chain?

[bugfolder]

Based on the change in https://github.com/backdrop/backdrop-issues/issues/6633, putting this code at the beginning of uc_order_default_rules_configuration() fixes the problem:

   if (state_get('update_d7_upgrade')) {
     return array();
   }

When the core change gets accepted (or modified), I'll update the PR on this issue (and add a comment of explanation, as in the current PR).

[bugfolder]

New PR in place that makes use of state variable update_d7_upgrade.

https://github.com/backdrop-contrib/ubercart/pull/515

[argiepiano]

I've test and WFM (the broken views are still a problem, but unrelated).

[bugfolder]

the broken views are still a problem, but unrelated

That's a core problem, but it made me wonder if we should open a core issue to move any file-based views into the db during the basic core upgrade to fix this gotcha for everyone.