Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The core filter module is running updates on config without ensuring config exists first and uses a constant that is not defined at update time. #6729

Open
jenlampton opened this issue Oct 13, 2024 · 7 comments · May be fixed by backdrop/backdrop#4885

Comments

@jenlampton
Copy link
Member

jenlampton commented Oct 13, 2024

Description of the bug

When upgrading from Drupal 7, I am seeing the following fatal error when running core/update.php:

An AJAX HTTP error occurred. HTTP Result Code: 500 Debugging information follows. Path: https://adhd-b.ddev.site/core/update.php?op=selection&token=i7Jv_S40J12XDDm-JGLLNpvcM9W0KxoLT8GKappCybU&id=68393&op=do_nojs&op=do StatusText: error ResponseText: TypeError: array_merge(): Argument #1 must be of type array, null given in array_merge() (line 2755 of /var/www/html/docroot/core/modules/user/user.module).

When proceeding to "the error page" (which is actually the same page), I see the following printed to the screen:

  • Warning: Attempt to read property "permissions" on bool in user_role_grant_permissions() (line 2755 of /var/www/html/docroot/core/modules/user/user.module).
  • Deprecated function: realpath(): Passing null to parameter #1 ($path) of type string is deprecated in BackdropLocalStreamWrapper->getLocalPath() (line 380 of /var/www/html/docroot/core/includes/stream_wrappers.inc).
  • The update process was aborted prematurely while running update #1001 in filter.module. All errors have been logged. You may need to check the watchdog database table manually.

When I return to core/update.php and attempt to update again, I can see that User module is about to run update 1008, and Filter module is about to run 1001, so the failure happened at 1007/1000.

The second time I run the update, everything completes with no issue.


Update 1001 in filter.module calls

user_role_grant_permissions(BACKDROP_AUTHENTICATED_ROLE, array('upload editor images'));
  1. Config is not moved into the database until user_update_1007().
  2. The constant BACKDROP_AUTHENTICATED_ROLE is hard-coded to authenticated, but the role name is not changed to authenticated until user_update_1007().

I think the simplest solution would be to add an implementation of hook_update_dependencies() to ensure that this filter update only runs after roles have been converted to configuration, and renamed.

/**
 * Implements hook_update_dependencies().
 */
function filter_update_dependencies() {
  // Ensure that roles have been converted to config before calling
  // user_role_grant_permissions().
  $dependencies['filter'][1001] = array(
    'user' => 1007,
  );
  return $dependencies;
}

Steps To Reproduce

Upgrade from Drupal 7

Actual behavior

Fatal errors

Expected behavior

No fatal errors

Additional information

Add any other information that could help, such as:

  • Backdrop CMS version: 1.29.1
  • Web server and its version:
  • PHP version:
  • Database sever (MySQL or MariaDB?) and its version:
  • Operating System and its version:
  • Browser(s) and their versions:
@jenlampton jenlampton changed the title The core filter module is running updates on config, without ensureing the config exists first. The core filter module is running updates on config, without ensuring the config exists first. Oct 13, 2024
@jenlampton
Copy link
Member Author

I also ran into an issue running filter_update_1004() because that function uses a constant that is not defined at update time: VIEWS_STORAGE_DEFAULT.

I'm amending this PR to include the update for the constant as well.

I ran into this again today doing a Drupal 7 core upgrade to Backdrop in the Backdrop summit at BADCamp. I think it's a more critical issue if it happens on every upgrade?

@jenlampton jenlampton changed the title The core filter module is running updates on config, without ensuring the config exists first. The core filter module is running updates on config without ensuring config exists first and uses a constant that is not defined at update time. Oct 25, 2024
@herbdool
Copy link

herbdool commented Nov 9, 2024

@jenlampton What other modules were installed on the Drupal 7 site? I cannot replicate the bug on a bare bones D7 site. I'd like to check that your PR fixes it. (I also tried it after installing Rules UI and Entity - for random choices - but still no error.)

@herbdool
Copy link

herbdool commented Nov 9, 2024

I thought the PR at least would do no harm, but when I run the same D7 db with the PR, I get these errors:

Notice: Trying to get property 'permissions' of non-object in user_role_grant_permissions() (line 2755 of /app/httpdocs/core/modules/user/user.module).
Warning: array_merge(): Expected parameter 1 to be an array, null given in user_role_grant_permissions() (line 2755 of /app/httpdocs/core/modules/user/user.module).
Warning: array_unique() expects parameter 1 to be array, null given in user_role_grant_permissions() (line 2755 of /app/httpdocs/core/modules/user/user.module).
Warning: array_values() expects parameter 1 to be array, null given in user_role_grant_permissions() (line 2755 of /app/httpdocs/core/modules/user/user.module).
Warning: Creating default object from empty value in user_role_grant_permissions() (line 2755 of /app/httpdocs/core/modules/user/user.module).
Notice: Undefined property: stdClass::$name in user_role_save() (line 2597 of /app/httpdocs/core/modules/user/user.module).
Notice: Undefined property: stdClass::$label in user_role_save() (line 2598 of /app/httpdocs/core/modules/user/user.module).
Warning: in_array() expects parameter 2 to be array, null given in user_update_1013() (line 1175 of /app/httpdocs/core/modules/user/user.install).

@herbdool
Copy link

herbdool commented Nov 9, 2024

I can avoid the previous error if instead of using user_role_grant_permissions(), we just do it directly with config:

  $config = config('user.role.authenticated');
  $permissions = $config->get('permissions');
  $permissions[] = 'upload editor images';
  $config->set('permissions', $permissions);
  $config->save();

I recommend adding this to your PR @jenlampton. I left a comment on the PR.

Though I'd still like to know how to reliably replicate your original error, so I can see if this really fixes it.

@quicksketch
Copy link
Member

Good suggestions @herbdool. Update hooks should only use database queries and config updates; avoiding any module-provided functions which might change in the future (or call hooks or whatnot).

@jenlampton
Copy link
Member Author

jenlampton commented Nov 14, 2024

I've merged both suggested changes. Thank you!

Update hooks should only use database queries and config updates; avoiding any module-provided functions which might change in the future (or call hooks or whatnot).

You'd think this would have occurred to me since it was the function call that was causing all the problems. 🤦

What other modules were installed on the Drupal 7 site?

I was upgrading a clean install of core / standard profile for the BadCamp presentation, but I ran into it the first time on a Drupal 7 site that was previously running the PHP filter module. Maybe try that?

@jenlampton
Copy link
Member Author

I had a friend report the same problem to me on July 18th - but I didn't believe him since I thought the core upgrade path was "well tested".

The core/update.php failed due to an error in the filter module: An AJAX HTTP error occurred. HTTP Result Code: 500 Debugging information follows. Path: https://autocomplete-deluxe.ddev.site/core/update.php?op=selection&token=i28rLEA3CZNwW83fEJs_oe1wZ6IT0kNg38Vh-AH-JtI&id=2&op=do_nojs&op=do StatusText: error ResponseText: Error: Undefined constant "VIEWS_STORAGE_DEFAULT" in filter_update_1004() (line 490 of /var/www/html/docroot/core/modules/filter/filter.install).

Why doesn't the core upgrade test catch this problem if it happens on a clean install of D7 standard profile? @herbdool are you also using ddev? I think he and I are both doing the upgrades on ddev. Maybe it has something to do with batching and how the updates are divided across batches?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment