#Contributed project blocker

Opened 📌 Remove deprecated moved_files entries in core Active too since those entries should be removed from 11.0.0

Opened 📌 Validate libraries.yml files Active as followup.

Committed and pushed d40360cb42 to 11.x and 1bf09e7490 to 11.0.x and 344c809e05 to 10.4.x and 0f4bafdf2b to 10.3.x. Thanks!

Can we define/provide a schema for libraries.yml? (or any other custom YAML, for that matter)

Follow-up could be useful, at least a test to iterate not in production

This code only actually gets executed if there is a theme overriding the moved file, this is why it didn't get caught until it was found in the stable theme.

If we wanted to add asserts/validation, we'd need to loop over every library definition when a file is parsed, and check the structure there. We don't do that for the rest of the library definition but I guess we could.. maybe a follow-up to validate everything?

Is there a way to test it to prevent this kind of errors? for example assert() in parser...

I also came across issue with webform submissions, but I suppose the patch from 165/166 is going to far, probably rendering the whole checking for recursion useless.

I suppose webform is really doing here something that is to be considered recursive rendering, see

https://git.drupalcode.org/project/webform/-/blob/03b6e07416480ad802f453...

This is ending up in using #theme => 'webform_submission_data' and then having this again in $variables['elements']['#theme'].

So I think the code here is still valid for checking for recursion, but we can maybe provide a possibility to circumvent the webform issue by providing a possibility to vary the render recursion key by something else than entity type, entity id and view mode. This would allow "intentionally" recursive rendering.

With this change it would be possible to solve the webform issue by using hook_preprocess_hook like this:

/**
 * Implements hook_preprocess_HOOK().
 *
 * fix regression (no submission data shown) due to patch from:
 * Can only intentionally re-render an entity with references 20 times -
 * https://www.drupal.org/project/drupal/issues/2940605
 */
function yourmodule_preprocess_webform_submission_data(&$variables) {
  if (isset($variables['elements']['#theme']) && $variables['elements']['#theme'] === 'webform_submission_data') {
    $variables['elements']['#recursion_key_variator'] = 'wrapped';
  }
}

The name of the variator can be anything, it just distinguishes the wrapped render element from the outer one.

Providing an interdiff to #155. (the patch from 155 is made of multiple commits, so when comparing the patch files you will note a lot more changes, but this interdiff is showing the real difference by comparing the effective changes via two git branches → ).

Not changing the MR right now. Not sure if maintainers agree on this, so letting MR out for the moment.

Already removed from 11.x so we just need a 10.4 MR here. Originally thought this was an error with the bc layer itself and was confused because it's tested - it's tested with a theme that correctly implements the syntax.

catch → changed the visibility of the branch 3449302- to hidden.

catch → changed the visibility of the branch 3449302-core-libraries to hidden.

Hello everyone. I ran into this very problem when migrating from drupal 9 to drupal 10 and running updates.
Is there a workaround in the meantime to "unbreak" the site or prevent it in the first place?

Updated comment to stay below 80 character limit and renamed the CR so that the title reflects the changes properly. Looks great to me now, if I hadn't been so involved myself I'd RTBC.

AFAICT this will block ✨ JSON-based data storage proposal for component-based page building Active , which is how I ended up reviewing this :)

Lots of comment nits plus a few actual questions. I think the change record should also be updated to be more explicit about what we expect field type maintainers to do: what are the updated best practices? (It captures that in a brief sentence or two, I'd like to see a concrete and , because that'll make it MUCH more likely that the ecosystem will adopt the new best practices: more explicitness.)

Ok both MRs are ready for review, I updated the CR again.

I'll update the MRs

I can see there being some valid cases for this but I don't see how they could be common enough to justify the complexity involved in supporting custom theming for the update.php. Also, the fact that maintenance_theme applies to update.php has been a surprise at least to me in the past because I expected it to only impact the maintenance page which is displayed for users.

Adding tag per 51.

As mentioned in slack, as a developer I'd support using stark on update.php, but that may be more extreme than needed.

Claro is definitely safer than trying to negotiate themes in the update page load.

Yeah I think I agree with @nicxvan - the only real reason to theme this page is if you are trying to brand some white label site builder and there is no branding on the claro version so I think this is fine and way way safer. Claro is not exactly the lightest theme but it is at least tested doing updates on every Drupal test run. I think we might need consensus from product managers to remove this capability.

Re #49 hard-coding Claro for update.php wouldn't necessarily be a bad thing either. We're actually lying right now by saying update.php's theme cannot be changed:

/**
 * A custom theme for the offline page:
 *
 * ...
 *
 * Note: This setting does not apply to installation and update pages.
 */
# $settings['maintenance_theme'] = 'claro';

History of the setting applying to update.php:

1. #2250119: Run updates in a full environment →
2. #3279640: Standard install profile uses Olivero for update.php →
3. #3304285: Remove Seven from core →

Where 1. basically converted the old drupal_maintenance_theme() calls to checking the setting, even though the setting says it does not apply to update.php

The amount of affected sites might go up, though. Then again, it's quite reasonable to argue update.php should be as stable and minimalistic as possible because your website is unstable when you visit it.

I think if the plan is to be consistent and only allow the system module, I'm curious why not enforce the same thing on the theme side and only allow Claro?

It's not a particularly strong opinion, but if this is trying to make updates more stable, custom themes are a place where people many times have hidden dependencies. I know most people don't add module dependencies to their themes and keep that up to date.
Would that possibly add instability if the theme is calling modules that are not loaded?

There is nothing preventing a custom theme from having a theme hook that calls field schema too which would cause the same issue that views is causing.

If you keep the theme negotiation rather than just setting Claro for the update screen, then I think we would need a test where the theme is set with a dependency and we check that update is setting Claro, I don't see a test for that at the moment unless I'm missing it.

Done, also updated the CR.

I feel that only allowing core and system theming in update.php is fine. Maybe a BC break is what is needed here. Maybe we limit to system. Have no new settings at all. If a theme depends on a module it not a good candidate for update.php anyway so instead of the exception we should check this in \Drupal\system\Theme\DbUpdateNegotiator and fallback to claro if the theme has dependencies. And then we are done. It's super rare that a module returns themed output from it's update function anyway.

P.S.: Once there is a consensus, I will update the 10.3 branch also.

Pushed a change to the 11.x branch to see what tests have to say.

This does lead to a minor contradiction in a sense that we're introducing a setting that allows you to choose which modules are active on update.php, but then immediately ban you from using any theme that depends on a module. Why even offer the choice at that point?

I'm wondering if we shouldn't just hard-code the module limit to 'system' then instead of providing a setting that allows you to choose which modules remain active. Although that would obviously be a BC break.

I tend to agree with 2 as 3 would put us right back at square one if one of these dependencies turns out to be unstable.

That's because you don't have

$settings['maintenance_theme'] = 'gin';

in your settings.php. This works due to \Drupal\system\Theme\DbUpdateNegotiator::determineActiveTheme()

I think this unfortunately adds a layer of complexity on top of this change. We have some options:

1. Only allow this if the active theme has no module dependencies (this would work for gin fwiw as it has no module dependencies)
2. Error if the active theme has module dependencies
3. Add the active theme's module dependencies

I think 2 or 3 are best. I started preferring 3 but actually I think I prefer 2 - I don't think we should allow maintenance themes that require modules. But maybe a frontend maintainer would disagree.

I just tried this by setting Gin as the admin theme and it does not seem to style update.php at all.

Discussed this briefly with @alexpott and he wondered what happens with this MR if you have gin + gin_toolbar installed - i.e. does gin work fine on update.php without gin_toolbar's stuff?