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

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 #3174924: Throw a better exception when a reference field can't find the target entity type, although to replace a more cryptic exception that existed before, maybe we can change this to log a warning instead?

#3380145: ViewsData should not cache by language 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?

Believe the unit failure will be fixed by a rebase.

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.

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.

Had a go at a re-title.

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.

I posted a comment in the wrong issue...

Took a pass at updating the CR.

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.

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.

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?

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

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.

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.

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!

Not sure if this is by design, but this change now prevents from sending emails through symfony_mailer within update functions since it uses a theme hook, which is now not available neither in hook_update_N() nor in hook_post_update_NAME(), so the only option is to do it with hook_deploy_NAME().