Allow update.php to load when entity type info needs to be updated

An example of this crash can be seen here: https://git.drupalcode.org/project/group/-/jobs/1337503#L914

Here's another WTF caused by Views that I stumbled upon 6 years ago: #2651974-44: field_ui_entity_operation() cannot respect route parameters because of incorrectly named routes →

At this point I'm starting to wonder if we should just take away Views' car keys because it's obviously drunk. It's asking for so much information that, whenever the info it wants hasn't been built yet or is unstable, your system crashes.

I think the first suggestion in the IS is probably the safest to explore, work with an allowlist rather than the inverse. But if we were going to go with a blocklist, we'd basically end up saying "Views bad" as it would be the only thing on that list and we'd run into the same issue again once another core (or contrib) modules starts asking for a lot of information.

For posterity, at the time of writing the first suggestion was:

Limit the hook_theme invocations when using the UpdateKernel to just the bare minimum: system and the active theme for update.php

kristiaanvandeneynde → changed the visibility of the branch 3441222-trying-to-repair to hidden.

kristiaanvandeneynde → changed the visibility of the branch 3441222-trying-to-repair to active.

The current MR allows me to at least visit update.php already. Will check if it works properly.

The general problem of update.php has been discussed in #2540416: Move update.php back to a front controller → and #2250119: Run updates in a full environment → .

The exception was added in 📌 Throw a better exception when a reference field can't find the target entity type Fixed , although to replace a more cryptic exception that existed before, maybe we can change this to log a warning instead?

🐛 ViewsData should not cache by language Needs work has some discussion about views_field_data() and the beginnings of ideas of making it use value objects (which could then produce the definitions when requested, not every cache rebuild). I feel like register_theme should not exist at this point, looks more suited to Drupal 7 theme functions than twig templates. Modules can define hook_theme() themselves, plugins produce render arrays that can set #theme, what is register_theme actually doing that's useful?

Re #8 I am fully onboard with your idea about making Views do less and make it smarter for the things it still does do.

I'd keep the exception rather than a warning because these fields should fail loudly if their schema has invalid pieces. But we need to be able to make the schema valid again, which is currently not possible without my MR or drush updb

Regarding the changes to update.php: It's actually helpful to have a separate kernel for update.php as I could use that to make this MR work.

This seems to be fully functional now. I added a new setting that you can use to either allow more modules than just system or simply allow ALL modules (old behavior).

Probably needs proper dependency injection for the kernel and settings services.

Seeing some random unrelated test fails

Believe the unit failure will be fixed by a rebase.

Still has the random failure and the sniffer failure relating to a mismatching DB version, but many people are reporting that one on Slack. Any idea regarding the random failure?

It's not a random failure, it's because you changed the default.settings.php

1) Drupal\Tests\ComposerIntegrationTest::testExpectedScaffoldFiles with data set #18 ('sites/default/default.settings.php', 'assets/scaffold/files/default...gs.php')
Scaffold source and destination files must have the same contents.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
$settings['update_free_access'] = FALSE;\n
\n
/**\n
- * Limits the modules to build the theme registry for on update.php.\n
- *\n
- * Because update.php needs to render things, the theme registry is built. Some\n
- * modules such as Views need to load a lot of data to build this registry and\n
- * some of that data may be unstable until the updates are ran. By default, the\n
- * list of modules is reduced to just system, but you can add to this list or\n
- * set the setting to FALSE to have all modules loaded.\n
- */\n
-# $settings['update_theme_registry_module_filter'] = ['system'];\n
-\n
-/**\n
* Fallback to HTTP for Update Manager and for fetching security advisories.\n
*\n
* If your site fails to connect to updates.drupal.org over HTTPS (either when\n

I reviewed this: https://git.drupalcode.org/project/drupal/-/merge_requests/4860/diffs

I updated default.settings.php with the same changes and the tests are passing locally.

Looks good to me, tests are passing now.

I ran test only as well and that fails as expected.

I did have one question, why don't you need to call $this->build(); again after the cache delete with the full module list?

Never mind I realized why I think. It's because it's only on update.php the next page load will hit the else.

Also it would defeat the purpose of this change.

I did have one question, why don't you need to call $this->build(); again after the cache delete with the full module list?

We don't cache the full result in the "light version". So next time someone requests the registry, it will be calculated anew. If that happens outside of the update page, then it will be with all modules and that result will be cached.

Thanks for the review and the fix, I completely overlooked that one. Only thing I can see now that might be holding this back is that we're not injecting 'settings' and 'kernel' via DI.

The title here is a statement of the problem, it would be better to describe what is being fixed. I am tired or I would make a suggestion. The proposed resolution here is a list of options so I didn't look a the code. I did leave some comments on the MR about improving the comments.

And why not use DI as mentioned in the previous comment.

Yeah didn't expect this to get set to RTBC without the DI being sorted out :) Currently a bit stretched for time, but will try to prioritize this when I do get some contrib/core time. Thanks for the reviews!

Had a go at a re-title.

Tried to squeeze this in between other tasks. Hopefully the docs are better now. Took care of DI and changed the logic slightly as suggested by @alexpott

I'm guessing this needs a change record for the new setting?

But the real question is, does the deprecation have to point to that CR or the issue here? If all the CR focuses on is the new parameter, then it's kind of moot to point to that CR for new constructor arguments.

Sorry about the premature rtbc. My understanding is you link to the CR, the CR has a link back to the issue if there are additional questions.

Also you can add helpful details to the CR.

All green now, just need some guidance on #23

Asked around on Slack and turns out we don't need two CRs, so I added one here: https://www.drupal.org/node/3445054 →

Will update the code to point to it.

Thanks! I also just want to make it clear, this probably wants to make it into a recommended release before Drupal 10.3 drops since that introduces the upstream change.

Took a pass at the CR.

Good changes IMO, thanks.

Title seems fine now so removing that tag

left 2 nitpicky stuff on MR but moving to NW if we can get 2 MRs now
1 for 11.x with deprecations removed
1 for 10.3 with deprecations still there (current MR).

Reason being we already did most removals for 11 of deprecated code so can commit to 11 with removals.

kristiaanvandeneynde → changed the visibility of the branch 10.3.x to hidden.

kristiaanvandeneynde → changed the visibility of the branch 3441222-11-without-deprecations to hidden.

Think we have a D11 and D10.3 ready MR now. Changed the order of the optional params so that the deprecation would be nicer for D11 where the optional param was still last.

Test fails in the D11 version seem completely unrelated, as if HEAD is broken

Ran the test-only job for the 11.x branch, won't post the entire output but can viewed here https://git.drupalcode.org/issue/drupal-3441222/-/jobs/1584320

Coverage appears to be there.

Reviewed the change record and adequately describes the issue that the new settings is overcoming, least to me.

Deprecations in 10.3 appear good to me as well with removals in the 11.x MR.

Believe this one is good +1 from me.

Looks good to me too.

It seems that @quietone's concerns about the comments have been addressed as well as they both read clearer.

Don't have time to commit this today (or probably this week - about to head afk for at least a couple of days), but +1 from me.

Thanks for the many reviews, much appreciated.

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?

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

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 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.

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.

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

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.

Done, also updated the CR.

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.

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.

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.

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.

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.

I'll update the MRs

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

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.

Reviewed this again.

Forcing Claro on update.php is good, we want as consistent an environment as possible and this is not like maintenance mode at all.

Restricting the theme hooks run should be fine because there are not really customisation points for update.php (and we would not want them to be), having to clear the cache afterwards is a bit of a workaround, but we can't use a null cache or similar here because that could make things even worse - we want as little theme logic running here as possible, not more.

I've added this to the 10.3.0-rc1 release notes doc.

Committed/pushed to 11.x/10.4.x and cherry-picked to 11.0.x and 10.3.x respectively, thanks!

Automatically closed - issue fixed for 2 weeks with no activity.