Problem/Motivation

We switched update.php from a front controller to a route so that it has a full environment to run in.

This however means that inbound path processors relying on a database table that has a schema change will not be able to update themselves - because they'd break update.php which is required to run the updates.

Proposed resolution

Move update.php back to a front controller, so that it doesn't have a direct dependency on inbound path processing or routing.

Additionally document systems that update.php relies on and the fact that they should only make backwards compatible schema changes, at least until #2547507: Consider adding hook_update_early_N() support is resolved.

Remaining tasks

Reviews, Commit

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#101 interdiff.txt2.21 KBdawehner
#101 2540416-101.patch25.42 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,840 pass(es). View
#98 interdiff.txt4.25 KBdawehner
#98 2540416-98.patch24.26 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,842 pass(es). View
#95 interdiff.txt473 bytesdawehner
#95 interdiff.txt473 bytesdawehner
#95 2540416-95.patch23.42 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,831 pass(es). View
#89 interdiff.txt1.75 KBdawehner
#89 2540416-89.patch23.27 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,839 pass(es). View
#82 interdiff.txt657 bytesdawehner
#82 2540416-82.patch22.12 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,754 pass(es). View
#79 interdiff.txt5 KBdawehner
#79 2540416-79.patch22.26 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,776 pass(es), 1 fail(s), and 0 exception(s). View
#68 interdiff.txt31.07 KBdawehner
#68 2540416-68.patch46.72 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,992 pass(es), 1,269 fail(s), and 289 exception(s). View
#66 2540416-66.patch18.13 KBhussainweb
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,553 pass(es). View
#66 interdiff-62-66.txt1.94 KBhussainweb
#62 interdiff.txt1.44 KBdawehner
#62 2540416-62.patch17.83 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,454 pass(es). View
#61 2540416-60.patch17.64 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,452 pass(es). View
#61 interdiff.txt5.36 KBdawehner
#58 interdiff.txt5.82 KBdawehner
#58 2540416-57.patch12.89 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,457 pass(es), 7 fail(s), and 0 exception(s). View
#56 interdiff.txt8.71 KBdawehner
#56 2540416-56.patch14.05 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,237 pass(es), 2 fail(s), and 0 exception(s). View
#54 interdiff.txt7.28 KBdawehner
#54 2540416-54.patch10.72 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,422 pass(es), 21 fail(s), and 0 exception(s). View
#51 2540416-51.patch11.94 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,052 pass(es), 0 fail(s), and 12 exception(s). View
#42 interdiff.txt7.72 KBdawehner
#42 2540416-42.patch26.79 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,868 pass(es), 34 fail(s), and 11 exception(s). View
#36 2540416-36.patch24.56 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2540416-36.patch. Unable to apply patch. See the log in the details link for more information. View
#36 interdiff.txt17.7 KBdawehner
#28 interdiff.txt14.89 KBdawehner
#28 2540416-28.patch16.07 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,641 pass(es), 24 fail(s), and 9 exception(s). View
#23 interdiff.txt4.64 KBdawehner
#23 2540416-23.patch6.44 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,633 pass(es), 23 fail(s), and 8 exception(s). View
#18 2540416-17.patch4.68 KBdawehner
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

dawehner’s picture

One thing we need to keep in mind that the container is fundamental, so fundamental that we even need it in order to get to upgrade.php, so
for example we should not couple the container to the cache system, as otherwise the cache table cannot ever be changed.

It is also doesn't make things necessarily easier for people writing update functions as they need to be aware of these 3 different steps. If you are honest though,
they implicit already need to know it.

chx’s picture

I do not want, badly so, stir up controversy. Still, I believe have some expertise in update matters which might help. So take everything below with a huge grain of salt and add "I cautiously think..." everywhere.

  1. Users might get confused by having two front controllers.
  2. update.php in D7 had a clunky hardwired piece of code to get Drupal 7 to bootstrap. This was roughly equivalent to the 1. in the issue summary but it was for major version upgrades only
  3. Having an update kernel / container makes sense -- what we want for that is an in memory version of everything. We have the know-how in kernel tests.
  4. Separating the "bootstrap broken" upgrades from trivial updates is a good idea for developers.

So: start update.php with in memory services borrowed (reused?) from kernel tests, run hook_upgrade_N (indeed no batch support -- we have in memory storage only so batch would be tricky) and then run hook_update_N. I think displaying the update page / form before hook_upgrade_N is doable so users don't accidentally mess up their database. If we aren't worried about that then just run hook_upgrade_N the moment update.php is loaded. Keep a list of upgrades in sites/default/files/.ht.upgrades -- there's noone else to keep it. I am not worried about information disclosure much, tbh but still .ht should be enough. The files directory placement is a settings override isn't it? So it should be doable w/o the database.

dawehner’s picture

So what we basically need to be able in a state where running rebuild.php would be doable without breaking anything.

dawehner’s picture

I totally agree that we should have something like hook_update_post_N() or something like that, but it is not clear for me whether we will certainly need a hook_update_pre_N().
Should we instead better try to promise that we are not changing storage mechanisms until you come to update.php?

On the other hand now that update.php is a front controller potentially all code, due to the existence of event subscribers, could be executed before we hit the DbUpdateController
so having a upgrade.php (or how we name it), would be a great reduction of fragility.

effulgentsia’s picture

Stage 2 we already have in HEAD. Should we do Stage 3 in #2538108: Add hook_post_update_X() for data value updates to reliably run after data format updates and repurpose this issue for a discussion/implementation of Stage 1?

catch’s picture

Title: Figure out if we need a three-stage update API » Decide whether we need hook_upgrade_N()/upgrade.php front controller
Category: Plan » Task
Issue summary: View changes

#5 makes sense to me, re-titling.

Should we instead better try to promise that we are not changing storage mechanisms until you come to update.php?

That would be good, and it's what I hoped would happen by blocking providing an upgrade path on 'critical issues needing an upgrade path', but #2532476: Menu links should use a TranslationWrapper to encapsulate safe translatable strings from YAML files is a borderline candidate for this already.

Gábor Hojtsy’s picture

dawehner’s picture

IMHO the point is that you potentially end up in a non recoverable state, as update.php will never be able to even start.
Step 3 is an API which gives you a tool for the case your system is already in the state where it can run already again. Additional updates, like unpublishing some blocks is something which could also be done in Step 2, there is "just" the problem of not throwing some events/hooks.

Gábor Hojtsy’s picture

According to #2538108-3: Add hook_post_update_X() for data value updates to reliably run after data format updates (the issue summary I cited there) that could also end up in WSODs in update and the kind AFAIS. I can see how these are different problems, so not objecting to maintain them as different issues, just trying to figure out the different priorities.

dawehner’s picture

Sure.

We should take care about not using the config API/container to fetch active modules but rather use the bootstrap config factory in order to get the list of active modules.

MiroslavBanov’s picture

Can you please explain this issue a bit more?

The Problem/Motivation is: There are different types of updates that can conflict with each other.
And the proposed solution is: To create a new subsystem, with several new hooks.

How does this solution help the stated problem/motivation? I am trying to be constructive, not wanting to argue at all. Maybe it's just the issue summary that is incorrect?

catch’s picture

So the idea is that hook_upgrade_N() would allow you to make changes to low level services (for example any service that update.php relies on which would result in errors when you try to get there).

Once those have run, you should be able to visit update.php which will allow you to run hook_update_N() - which can for example make updates to config/entity schema.

Then at the end of that, you might want to update actual configuration or entity values using those APIs, and by that point the schema is up-to-date with the code base so that you can.

Right now, there is only a single set of updates, and we only allow a linear set within a module.

This means that

mymodule_update_8001() -> updates schema.
mymodule_update_8002() -> updates values
mymodule_update_8003() -> updates schema

These often only work if mymodule_update_8003() runs in a separate site update to the first two, but when updating a module, it's possible all three updates need to run and they conflict.

You can just about work around this with hook_update_dependencies() and rewriting earlier updates to account for later changes, but the three-stage update API would mean you're always guaranteed that all schema changes are finished before value updates happen.

pwolanin’s picture

Right now we have /update.php as a route which is a bit weird and mostly to benefit people who read old docs, or something.

Why don't we make update.php (or redirect to core/update.php) a real front controller again and then redirect and use a regular route + path like /admin/system/update to run hook_update_N

effulgentsia’s picture

#6 claims that #2532476: Menu links should use a TranslationWrapper to encapsulate safe translatable strings from YAML files was a potential need for this issue, but it's not, so I think we should still consider #4's option of figuring out what schema is actually called upon prior to the invocation of a routed (non-front) controller, and if we can live with not changing any of that schema in a BC-breaking way throughout 8.x.

But, if we decide that we do have a need for some very early running update functions, then I think something like #13 makes sense: update.php as a front controller that invokes hook_update_early_N() followed by a redirect to /admin/update for invoking hook_update_N().

njbarrett’s picture

Issue summary: View changes

Fix a typo in issue summary

dawehner’s picture

These are the tables used by /update.php by default:

  • key_value, key_value_expire
  • cache_config, cache_data, cache_discovery, cache_bootstrap
  • router
  • cachetags
MiroslavBanov’s picture

@catch
So this issue about fixing potentially broken update.php.

Would it apply to some conflict between different modules, like this Real world example? If not, is there different issue that does?

Right now, such issues should be solved with hook_update_dependencies. But there is no limit to the ways that updates can conflict, and CATCH-22 situations can happen. Updates can do anything and can depend on anything.

Thanks for the answers, and sorry if I am writing in the wrong issue, making noise.

dawehner’s picture

FileSize
4.68 KB

Just gave it a try in order to be able to judge how feasible it is to do.
One problem you run into when doing that is that low level functionality (Extension discovery) is bundled with caching directly.

effulgentsia’s picture

Re #16, what if you truncate all your cache tables before hitting /update.php? I suspect some more tables would show up in that list.

catch’s picture

#2532476: Menu links should use a TranslationWrapper to encapsulate safe translatable strings from YAML files it turned out the notices on update.php were a bug in the hook_update_N() (so that notices were thrown after the update), not with update.php or the update tests themselves. So yes I think that's not relevant here, and after #356399: Optimize the route rebuilding process to rebuild on write it shouldn't be possible to trigger a router rebuild just by visiting update.php at all so should also not be relevant for the empty cache scenario hopefully.

#13 sounds good. I can't remember why we left the path as update.php, but moving that back to a controller and having a route-looking route would be the least change and make more sense.

dawehner’s picture

Good question! Btw. here is a query to clear all cache_ tables: SELECT concat('TRUNCATE TABLE `', TABLE_NAME, '`;') FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_NAME LIKE 'cache_%' AND TABLE_SCHEMA='dev_d8'; (just in case someone wondered whether this is possible).

One interesting sideeffect: I realized /update.php is currently page cached. Are we sure we want to do that?

catch’s picture

dawehner in irc pointed out that one option here is not to do anything until we have an update that changes one of the things that update.php currently relies on.

fwiw I'm OK if we collectively decide that, as long as we know that once we hit the problem this issue will be a blocker to the issue that requires it.

Although this also means that any contrib implementations of services and etc. that update.php relies on (say an inbound path processor) will have the same limitation, and they won't be able to add the API they need.

dawehner’s picture

Status: Active » Needs review
FileSize
6.44 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,633 pass(es), 23 fail(s), and 8 exception(s). View
4.64 KB

Just some work in the meantime. This basically now has all the basic steps needed.

Recarding critical vs. not critical, I think the contrib point is quite strong if you think about contrib/custom code doing all kind of crazy stuff, so
they need to be able to repair, if possible.

Status: Needs review » Needs work

The last submitted patch, 23: 2540416-23.patch, failed testing.

effulgentsia’s picture

Re #21, when I try (logged in as user 1), I also see:

  • sessions
  • users_field_data
  • user__roles

I haven't tried #23, but I suspect that it would also need to query those tables, plus the cache and kv ones. So potentially the only difference between routed controller and front controller is router, url_alias, and whatever else contrib does in middlewares and request listeners.

dawehner’s picture

I actively used $update_free_access, because I think being able to even authenticate is way too much risk to get to an update.

catch’s picture

With Drupal 7 this was also the case - you either have to be logged in already to run the update, or use $update_free_access. Requiring $update_free_access all the time would be unfortunate, but authentication providers provide a much wider area where 'being logged in' could go wrong in the update.

If we had a front controller, we could try to authenticate + check permissions in a try catch and then eat the exceptions and tell people to use $update_free_access if it fails?

dawehner’s picture

Status: Needs work » Needs review
FileSize
16.07 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,641 pass(es), 24 fail(s), and 9 exception(s). View
14.89 KB

Let's authenticate.

Status: Needs review » Needs work

The last submitted patch, 28: 2540416-28.patch, failed testing.

Berdir’s picture

I'm working on updating our sites to beta13. Started with the newest site that is currently on beta11, also have some sites that are still on beta8. No idea about those yet..

The biggest problem that I encountered so far is the rename of the plugin_selector module to plugin (thank you tim :p), which is problematic because its interfaces are used, very, very early. In plugin managers, which are included in a compiler pass.

I've managed to work around that for now by manually adjusting the classloader in autoload.php: $loader->setPsr4('Drupal\plugin\\', 'modules/plugin/src');.

I don't really know how to solve this in a generic way, how can we load anything or run update functions without a container which would be the only way to around something like this? I guess as a start, we should make compiler passes that load services/classes as error-resilient as possible, but this is a php fatal error on a missing interface. Only thing I can think of is to remove the interface check there and do it on runtime, in that case it is not in the critical path.

Everything else went pretty smooth, the move of entity manager updates to the end helps *a lot*, compared to the trouble I had especially with beta6 -> 7 and also 7 to 8.

dawehner’s picture

@berdir
Thank you for sharing your "interesting" experience. If I understand correctly, you are basically argueing that given that still APIs are changed a bit, relying on a working container/routing
would not fly at all. In the current iteration of the patch you could not login as user, plugin_selector would have kept a update_early.inc file. In there it could drop the existing container instance, then manipulate the config.extension database entry to contain the new module and afterwards rebuild the container. Shouldn't this then boot up the right version of the container?

Berdir’s picture

  1. +++ b/core/update.php
    @@ -0,0 +1,158 @@
    +  // Scan the module list.
    +  // We don't support install profiles at that point?
    +  $extension_discovery = new ExtensionDiscovery($kernel->getAppRoot(), FALSE, []);
    +  $module_extensions = $extension_discovery->scan('module');
    

    which point exactly and support how?

    This worked fine for me for modules inside the install profile. The install profile itself AFAIK can't have update functions, though, would make sense, though. I currently have a separate module just for that.

  2. +++ b/core/update.php
    @@ -0,0 +1,158 @@
    +    $kernel->boot();
    +    $container = $kernel->getContainer();
    +    /** @var \Drupal\Core\Authentication\AuthenticationManager $authentication_manager */
    +    $authentication_manager = $container->get('authentication');
    +    $account = $authentication_manager->authenticate($request);
    

    This still gives me a fatal error without my autoload.php hack. That can't be try/catch'd, unfortunately.

    What about checking for update_free_access *first* and don't even try if it that's set to TRUE?

  3. +++ b/core/update.php
    @@ -0,0 +1,158 @@
    +  $response = new Response();
    +  $log_filename = PublicStream::basePath() . '/.ht.update_early';
    +
    

    .ht is a apache thing I think.

    Do we really need this? we have an example update early function that writes to database, can't we use a key_value collection and store this information there?

I commented out the auth code and tried it then:

1. run core/update.php
2. That runs and redirects to admin/update. that explodes completely because it doesn't know that route yet, so it tries to display a 404 error page and that explodes due to invalid contexts.
3. drush cr
4. drush en -y beta2beta + drush ev "beta2beta_set_beta_version(11)"
5. Then the page works, I can see the pending updates, but trying to apply gives me this error: "Recoverable fatal error: Argument 1 passed to Drupal\Core\ProxyClass\Render\BareHtmlPageRenderer::renderBarePage()"
6. drush updb to avoid that.

dawehner’s picture

This still gives me a fatal error without my autoload.php hack. That can't be try/catch'd, unfortunately.

What about checking for update_free_access *first* and don't even try if it that's set to TRUE?

Good idea, this should work, too bad that PHP cannot catch this.

.ht is a apache thing I think.

Do we really need this? we have an example update early function that writes to database, can't we use a key_value collection and store this information there?

Well relying on as few as possible things would be nice, just imagine that you might want to change KV, then you cannot query KV maybe before that. Yes we could store the data in a custom DB table which just never changes, but relying on just a file feels safer.

2. That runs and redirects to admin/update. that explodes completely because it doesn't know that route yet, so it tries to display a 404 error page and that explodes due to invalid contexts.

Puh, so we need to trigger a router rebuild already? PUH. Maybe we should enter a row inter {router} manually and clear the corresponding caches.

5. Then the page works, I can see the pending updates, but trying to apply gives me this error: "Recoverable fatal error: Argument 1 passed to Drupal\Core\ProxyClass\Render\BareHtmlPageRenderer::renderBarePage()"

Which code calls BareHtmlPageRenderer with the wrong parameters?

Berdir’s picture

2. Sorry, that wasn't clear. I don't think we need to do a router rebuild unless a specific early update forces that. This is a special case since this doesn't exist yet on the site that we are updating from. It is a situation that will only happen once. That said, since it could be done *after* early updates, it should be a safe thing to do since routes need to work anyway on the redirected page?

But it's possible we care about this one-time situtation too. Then maybe we could check before we redirect if the route exists and only do a router rebuild then before we redirect?

5. DbUpdateController i think, tests are failing for the same reason, from the first test fail: "Argument 1 passed to Drupal\Core\ProxyClass\Render\BareHtmlPageRenderer::renderBarePage() must be of the type array, boolean given, called in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/src/Controller/DbUpdateController.php on line 201 and defined..."

timmillwood’s picture

My nginx config (the default in Laravel's Homestead vagrant environment) didn't like update.php as it tried to find the file rather than pass it to index.php. Changing update.php to update will solve this for me and many more to come.

For now I just edited the nginx configuration.

dawehner’s picture

Status: Needs work » Needs review
FileSize
17.7 KB
24.56 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2540416-36.patch. Unable to apply patch. See the log in the details link for more information. View

But it's possible we care about this one-time situtation too. Then maybe we could check before we redirect if the route exists and only do a router rebuild then before we redirect?

I fear that doing the router rebuild will be tricky. Just imagine that a view might change the way how the router looks like, and well at that point in time, you can't really guarantee the state of config entities. I fear we need to not only provide the update_early functions as update.php but also all the other one, if we really require a router rebuild.
In general I try to understand why the router should be ever in an inconsistent state? It is not rebuild on runtime anymore.

Did some work to use an Update Kernel and extract some of the functionality in its own class. Don't look at it, unless you really want to.

Berdir’s picture

The rebuild is needed because this patch is changing the routes, so you need to rebuild them to be able to access the new route.

But yes, good points on views and stuff.

What if we keep the existing route and name the new file early_update.php or something like that? Then this doesn't introduce a change and there is no need to rebuild the routes.

Berdir’s picture

Also, I'm having a bit of trouble getting my early update that is supposed to enable a module working properly. Caching seems to interfere, as the caches aren't invalidated and I have no API to invalidate the cache.

I think this is moving toward avoiding to use caches during the updates, which might help but we need to make sure to clear the actual caches at the end.

timmillwood’s picture

+++ b/core/modules/system/system.routing.yml
@@ -449,7 +449,7 @@ system.batch_page.json:
+  path: '/admin/update/{op}'

+1 to /admin/update

You may think me weird, but I was dreaming about this, thinking if we move from update.php to update what if someone wants to add some content on /update, I panicked and woke in a sweat. Glad to see @dawehner had already thought about it.

catch’s picture

Rather than rebuilding, what about manually inserting the route into the router table in the early update?

That wouldn't work for an alternative route provider, but should let this run.

dawehner’s picture

That wouldn't work for an alternative route provider, but should let this run.

This is totally fine, they would provide an update early implementation as well.
I was thinking about rebuilding JUST the yml based ones and fire no event.

dawehner’s picture

FileSize
26.79 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,868 pass(es), 34 fail(s), and 11 exception(s). View
7.72 KB

Some work on some tests and insert the entry into the router.

Status: Needs review » Needs work

The last submitted patch, 42: 2540416-42.patch, failed testing.

catch’s picture

We discussed this on the core maintainers 8.x critical triage call, as well as on the EuroCriticals call this morning. Trying to summarize some of the discussion:

Both xjm and alexpott are concerned about adding hook_early_update_N() as an extra thing for contrib maintainers to think about. An alternative is to just say that changes that the update system itself depends on must be bc-compatible (at least enough that hook_update_N() can run). If we make the front controller change here, then we'll additionally reduce the surface area by not including inbound path/route processors in that.

So we should probably split this issue into two - one to add the front controller, and one to add hook_early_update_N(). The front controller change itself should be quite straightforward. I actually think #2538108: Add hook_post_update_X() for data value updates to reliably run after data format updates is where we'll run into many more problems, so we should possibly postpone hook_early_update_N() on that issue (i.e. add that first, and only then look at early updates).

Gábor Hojtsy’s picture

@catch: which parts remain critical? does the postponed part means #2538108: Add hook_post_update_X() for data value updates to reliably run after data format updates is critical too?

catch’s picture

Title: Decide whether we need hook_upgrade_N()/upgrade.php front controller » Move update.php back to a front controller
Issue summary: View changes

Opened #2547507: Consider adding hook_update_early_N() support and updated the title/issue summary.

dawehner’s picture

The remaining critical part is to have a dedicated front controller which ensures that you can run updates before having a working Drupal.

I'll work on this during drupalaton.

catch’s picture

@Gabor I think #2547507: Consider adding hook_update_early_N() support is something we can potentially just document, then add the early hook when we decide it's definitely necessary to do so (as an API addition in a minor release). There's a chance we'll get caught out and have to introduce it to fix an incoming 8.x critical of course, but again could bump it back then. The use-cases for that are fairly limited, just very, very hard to workaround once you have one.

I personally think that #2538108: Add hook_post_update_X() for data value updates to reliably run after data format updates is critical - at least in the sense that it could save us from many critical upgrade path bugs with dependencies etc. down the line, massively simplifies all the arguments about which APIs can and cannot be used in updates (and whether we should stop certain hooks/events from firing) - all of which to me is a bottomless rabbit hole filled with snakes which we've been crawling around in without a light for about 10 years. However we didn't really discuss bumping that either on the 8.x critical triage call or EuroCriticals this morning so it's up for discussion still.

dawehner’s picture

Well I don't care whether it is critical now or later. GIven how fragile a full website with all kind of subscribers I think we WILL run into the issue of not being able
to come to update.php. This will immediately be critical, so solving it now or later IMHO doesn't matter. If people, especially contrib will do stuff, we will have a critical issue sooner or later.

catch’s picture

This issue is definitely critical, it's just #2547507: Consider adding hook_update_early_N() support which we might defer - but yes it will very likely end up critical eventually even if we defer it now.

dawehner’s picture

Status: Needs work » Needs review
FileSize
11.94 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,052 pass(es), 0 fail(s), and 12 exception(s). View

While I totally agree that hook_update_early_N() is critical, here is a first version of something like update.php.

Gábor Hojtsy’s picture

Right, my question was more from a release management perspective. If the assessment of what we aim for for the 8.0.0 release changed, we should keep in check if what we wanted to originally is still critical for the initial release. (Not arguing if it is or not in fact).

Status: Needs review » Needs work

The last submitted patch, 51: 2540416-51.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
10.72 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,422 pass(es), 21 fail(s), and 0 exception(s). View
7.28 KB

Less failures, this is for sure.

Status: Needs review » Needs work

The last submitted patch, 54: 2540416-54.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
14.05 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,237 pass(es), 2 fail(s), and 0 exception(s). View
8.71 KB

Some documentation improvements.

Status: Needs review » Needs work

The last submitted patch, 56: 2540416-56.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.89 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,457 pass(es), 7 fail(s), and 0 exception(s). View
5.82 KB

Fixes the access parts, now we are dealing with the logo pointing to /update.php but so failing some tests.

Status: Needs review » Needs work

The last submitted patch, 58: 2540416-57.patch, failed testing.

mpdonadio’s picture

+++ b/core/modules/system/src/Controller/DbUpdateController.php
@@ -364,7 +366,8 @@ protected function selection() {
-      $url = new Url('system.db_update', array('op' => 'run'));
+      $base_url = str_replace('/update.php', '', $request->getBaseUrl());
+      $url = (new Url('system.db_update', array('op' => 'run')))->setOption('base_url', $base_url);
       $build['link'] = array(

Made #2548095: Add option to Url() to force the site base_url to be used to avoid shenanigans like this.

dawehner’s picture

Status: Needs work » Needs review
FileSize
5.36 KB
17.64 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,452 pass(es). View

There we go.

dawehner’s picture

FileSize
17.83 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,454 pass(es). View
1.44 KB

Added the @todo for #60. Thank you a lot @mpdonadio to add the issue.

The last submitted patch, 36: 2540416-36.patch, failed testing.

isntall queued 36: 2540416-36.patch for re-testing.

stefan.r’s picture

Just a spelling check :)

  1. +++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
    @@ -0,0 +1,164 @@
    + * to repair Drupal if its in a broken state.
    

    it is

  2. +++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
    @@ -0,0 +1,164 @@
    +   * This fake routing data is needed in order to make batch API working
    +   * properly.
    

    work properly

  1. --- /dev/null
    +++ b/update.php
    

    Missing @file comment

  2. +++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
    @@ -0,0 +1,164 @@
    +   * Ensures that the request has a fake routing data for update.php.
    

    s/a fake routing data/fake routing data/

  3. +++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
    @@ -0,0 +1,164 @@
    +   * Checks access and throws an access, in case it is not.
    

    throws an exception, in case it is not accessible

hussainweb’s picture

FileSize
1.94 KB
18.13 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,553 pass(es). View

Fixing for comments in #65, probably not in the same way as suggested but I think it will suffice.

dawehner’s picture

Thank you @stefan.r and @hussainweb
Will work now on making a dedicated, not compiled, container.

dawehner’s picture

FileSize
46.72 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,992 pass(es), 1,269 fail(s), and 289 exception(s). View
31.07 KB

This is some work to let update.php use its own container.

There are still things to be figured out:

  • How to swap to the correct kernel for drupal_flush_all_caches() at the end of the update. How can we leverage a DrupalKernel object for that.
catch’s picture

I understand why we'd want to use a custom container for hook_upgrade_N()/hook_early_update_N(), but not why we'd do that just to move it to a controller. Moving to a controller means no dependency on routing or inbound path processors, but we want a full container when running normal hook_update_N() I think.

Status: Needs review » Needs work

The last submitted patch, 68: 2540416-68.patch, failed testing.

dawehner’s picture

Theoretically it would have been possible with #68 to limit the available APIs so you could not call out the the entity manager but use the config API/state/KV

catch’s picture

That would take us back to #2001310: Disallow firing hooks during update. I don't think we can go there, or at least if we want to try, it needs to be postponed on #2538108: Add hook_post_update_X() for data value updates to reliably run after data format updates - since contrib/custom updates will use the entity API and in some cases absolutely have to.

Crell’s picture

Gabor pinged me for a review here. I've not read the full issue yet so if there's some previously discussed reason for something above, I missed it. :-) Overall I'm in favor of this shift as having Drupal updated from within itself is totally weird. A separate application, even a micro-one, is a more stable approach.

  1. +++ b/core/includes/bootstrap.inc
    @@ -554,7 +554,9 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
         // Mark this page as being uncacheable.
    -    \Drupal::service('page_cache_kill_switch')->trigger();
    +    if (\Drupal::hasService('page_cache_kill_switch')) {
    +      \Drupal::service('page_cache_kill_switch')->trigger();
    +    }
    

    If we're using an alternate container configuration anyway, why is this needed? Include a page_cache_kill_switch service in the update container and have it be a no-op. This code should not need to know about different "modes" (which is essentially what's being done here).

    That actually applies to a bunch of other things in this patch, too, it looks like. if (service exists Foo) { use Foo } is no different conceptually than if (MAINTENANCE_MODE), which is pushing the conditional far too far down into the system. Having the container means we don't need to do that, we just stub things out in the container.

  2. +++ b/core/lib/Drupal/Core/File/FileSystem.php
    @@ -55,9 +55,9 @@ class FileSystem implements FileSystemInterface {
    -  public function __construct(StreamWrapperManagerInterface $stream_wrapper_manager, Settings $settings, LoggerInterface $logger) {
    +  public function __construct(StreamWrapperManagerInterface $stream_wrapper_manager, Settings $settings, LoggerInterface $logger = NULL) {
         $this->streamWrapperManager = $stream_wrapper_manager;
         $this->settings = $settings;
         $this->logger = $logger;
    @@ -100,7 +100,9 @@ public function chmod($uri, $mode = NULL) {
    
    @@ -100,7 +100,9 @@ public function chmod($uri, $mode = NULL) {
           return TRUE;
         }
     
    -    $this->logger->error('The file permissions could not be set on %uri.', array('%uri' => $uri));
    +    if ($this->logger) {
    +      $this->logger->error('The file permissions could not be set on %uri.', array('%uri' => $uri));
    +    }
    

    Same idea here: There's a Null logger available that is a no-op. If the constructor parameter is null, default to a new NullLogger instance and then the second conditional is unnecessary.

  3. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -100,6 +100,7 @@ public function processAttachments(AttachmentsInterface $response) {
     
         $attached = $response->getAttachments();
    +    $attached += ['html_response_placeholders' => []];
     
    

    Why is this line not just moved into getAttachments()? It seems like this is a check we'd need to add in many places potentially, which means it should be in getAttachments() so we have a more normalized pseudo-value-object.

  4. +++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
    @@ -0,0 +1,236 @@
    +      $this->moduleList = isset($extensions['module']) ? $extensions['module'] : array();
    

    Total nit: []

  5. +++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
    @@ -0,0 +1,236 @@
    +    // Load each module's .update_early_services files.
    +    foreach ($module_filenames as $module => $filename) {
    +      $filename = dirname($filename) . "/$module.services.update.yml";
    

    Comment filename seems out of sync with the filename that's being included.

  6. +++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
    @@ -0,0 +1,236 @@
    +   * Boots up the session.
    +   *
    +   * bootSession() + shutdownSession() basically simulates what
    +   * \Drupal\Core\StackMiddleware\Session does.
    

    Is there a reason to not use it then? Or an alternate wrapping object instead of inlining? Splitting this out seems wise as it could then be used by other alternate kernels in the future. (The installer, etc.) If that's not possible for some reason, that should be documented.

  7. +++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
    @@ -0,0 +1,236 @@
    +    $request->attributes->set(RouteObjectInterface::ROUTE_OBJECT, new Route('/update.php/{op}', ['op' => 'info'], ['_access' => 'TRUE']));
    

    In most other cases I think we've been breaking a new Route() call to multiple lines for each parameter, for readability.

Also, it seems like the solo route information is spread out across 3 places. There's a RouteProvider, but the _controller equivalent is hard coded in the Kernel, in 2 places. Why not just move all of the info that would normally be on the route to the RouteProvider? We don't need to do full on routing then, but we can generalize the code better and keep the relevant information in one place. At most we hard code a single route name in the kernel but all the data about it is in the RouteProvider.

timmillwood’s picture

I have real concerns with adding yet another file outside /core.

Like install.php should it not go in /core? or like cron should it be a route (but without the php extension)?

catch’s picture

This issue needs to stop at #66 - that allows inbound path processors/route listeners to make non-bc schema changes.

#68 is in scope for #2538108: Add hook_post_update_X() for data value updates to reliably run after data format updates and/or #2547507: Consider adding hook_update_early_N() support but without the clarity on what is and isn't allowed that those issues provide, it's a rabbit hole here.

effulgentsia’s picture

Even if we use the same service definitions as the runtime container does (which makes sense until one of the two issues from #75 lands), don't we still need to ensure that during update.php, that container is always rebuilt from scratch (not compiled/cached)?

effulgentsia’s picture

However, #76 would mean that compiler passes (assuming we still want to run them, which I think we need to) must work regardless of db schema state (other than the enabled modules list), which I think is a really good idea anyway, but I'm not sure if core complies 100% with that.

catch’s picture

#77 :Compiler passes is the realm of early updates, until then schema changes if any have to be bc.

Note that container building not only depends on module list but also default langcode so that too can't change much.

#76: that makes sense but it doesn't have to be done here to fix the issue title and routing dependency. So we can add it to scope here or open a new critical issue to make that change.

dawehner’s picture

Status: Needs work » Needs review
FileSize
22.26 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,776 pass(es), 1 fail(s), and 0 exception(s). View
5 KB

I agree with catch that we should keep thing reviewable and #68 does not make that more simple.
#2550411: Let update.php use its own container in order to have more control of what is executed is a new issue for that.

I expanded the test coverage to test for broken routing, which was exactly the purpose of this issue.

Even if we use the same service definitions as the runtime container does (which makes sense until one of the two issues from #75 lands), don't we still need to ensure that during update.php, that container is always rebuilt from scratch (not compiled/cached)?

We either get rid of using any tags (as we require a compiled container) or accept compilation. I don't see actually a problem to have a different environment passed into the kernel.
This will result into a different compiled container which will not interfere with the existing one.

I have real concerns with adding yet another file outside /core.

Like install.php should it not go in /core? or like cron should it be a route (but without the php extension)?

Well, this is a good way to fix the problem without having to update existing documentation / change other internal stuff. Feel free to open up a new dedicated issue for discussing that. This does not belong IMHO into this particular issuel

effulgentsia’s picture

Re: update.php at the root level vs. in /core, I would have concerns about it being added at the root level in this issue if its contents had anything that we'd want the ability to change in future minor releases. /core/install.php does have such contents, so I'm glad it's in /core. But the update.php in this patch is the same kind of skeleton as index.php is, so I think it's ok for non-critical followups to discuss moving one or both elsewhere, because if we end up releasing 8.0.0 with them at the root level, I'm not too concerned about us needing to deploy changes to them to sites that install on 8.0.0. And if I'm wrong about that (e.g., if there's a risk of Symfony 3.FOO requiring us to make changes to index.php and update.php), then we should probably have a critical issue to discuss what to do with both, since it's the same problem for both.

Status: Needs review » Needs work

The last submitted patch, 79: 2540416-79.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
22.12 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,754 pass(es). View
657 bytes

Well let's skip the frontpage test, we just care about a running update.php.

mpdonadio’s picture

diff --git a/core/modules/system/src/Tests/Update/UpdatePathWithBrokenRoutingTest.php b/core/modules/system/src/Tests/Update/UpdatePathWithBrokenRoutingTest.php
index e9b37d1..f783608 100644
--- a/core/modules/system/src/Tests/Update/UpdatePathWithBrokenRoutingTest.php
+++ b/core/modules/system/src/Tests/Update/UpdatePathWithBrokenRoutingTest.php
@@ -32,9 +32,6 @@ public function testWithBrokenRouting() {
     \Drupal::state()->set('update_script_test_broken_inbound', TRUE);
 
     $this->runUpdates();
-    \Drupal::state()->set('update_script_test_broken_inbound', FALSE);
-    $this->drupalGet('<front>');
-    $this->assertResponse(200);
   }
 
 }

This just seems weird. We are testing that the update script can run with a busted router (simulated via an exception in an inbound path processor), but then we can't browse to the front page after we run updates? It really seems like

public function testWithBrokenRouting() {
    // Make sure we can get to the front page.
    $this->drupalGet('<front>');
    $this->assertResponse(200);

    // Simulate a broken router, and make sure the front page is
    // inaccessible.
    \Drupal::state()->set('update_script_test_broken_inbound', TRUE);
    $this->drupalGet('<front>');
    $this->assertResponse(404);

    $this->runUpdates();

    // Remove the simulation of the broken router, and make sure we can get to
    // the front page again.
    \Drupal::state()->set('update_script_test_broken_inbound', FALSE);
    $this->drupalGet('<front>');
    $this->assertResponse(200);
  }

should work, but it won't because UpdatePathTestBase::setUp() overrides WebTestBase::setUp() and the result is that the router isn't installed anyway.

I don't know if anything can be done about this, though. Maybe just a comment on the test to explain the situation?

?

dawehner’s picture

Well, this is basically caused by the fact that we never had an update path for #2509300: Path alias UI allows node/1 and /node/1 as system path then fatals so the configured frontpage is "node" and not "/node" which is what we would need.

timmillwood’s picture

dawehner’s picture

Issue summary: View changes

Updated the IS to make it clear what we need

catch’s picture

#84 should we update the db dump to start with the right configuration?

dawehner’s picture

dawehner’s picture

FileSize
23.27 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,839 pass(es). View
1.75 KB

There we go.

The last submitted patch, 36: 2540416-36.patch, failed testing.

Gábor Hojtsy’s picture

+++ b/core/modules/system/src/Tests/Update/UpdateScriptTest.php
--- a/core/modules/system/system.routing.yml
+++ b/core/modules/system/system.routing.yml

+++ b/core/modules/system/system.routing.yml
+++ b/core/modules/system/system.routing.yml
@@ -451,13 +451,9 @@ system.batch_page.json:

@@ -451,13 +451,9 @@ system.batch_page.json:
 system.db_update:
   path: '/update.php/{op}'
   defaults:
-    _title: 'Drupal database update'
-    _controller: '\Drupal\system\Controller\DbUpdateController::handle'
     op: 'info'
-  options:
-    _maintenance_access: TRUE
   requirements:
-    _access_system_update: 'TRUE'
+    _access: 'TRUE'

Sorry if I missed something, looked at the comments and did not find an explanation on why do we need this if we have our own front controller on update.php anyway?

dawehner’s picture

The reasons are URL generated pointing to /update.php

Gábor Hojtsy’s picture

That may be worth a comment in the routing.yml file, because that is not a route that would be served from Drupal anymore after this patch, so people looking for it would possibly be confused IMHO.

dawehner’s picture

Mh, its not different to , or in that sense.

dawehner’s picture

FileSize
23.42 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,831 pass(es). View
473 bytes
473 bytes

There we go.

jibran’s picture

I think this is ready.

+++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
@@ -0,0 +1,165 @@
+      // to use session API, see \batch_process().

\ can be removed.

larowlan’s picture

looks ready to me too, just some minor observations

  1. +++ b/core/modules/system/src/Controller/DbUpdateController.php
    @@ -207,7 +207,7 @@ public function handle($op, Request $request) {
    -  protected function info() {
    +  protected function info(Request $request) {
    

    nit:For some methods we updated the docblock params, but not for others.

  2. +++ b/core/modules/system/src/Controller/DbUpdateController.php
    @@ -497,7 +507,7 @@ protected function results() {
    -    $try_again_url = Url::fromRoute('system.db_update', $options)->toString(TRUE)->getGeneratedUrl();
    +    $try_again_url = Url::fromUri(\Drupal::request()->getUriForPath(''))->setOptions(['query' => $options])->toString(TRUE)->getGeneratedUrl();
    

    any reason why we didn't pass the $request along like we did for the other methods?

  3. +++ b/core/modules/system/src/Tests/Update/UpdatePathWithBrokenRoutingTest.php
    @@ -0,0 +1,62 @@
    +    $this->assertResponse(500);
    +
    +    // The exceptions are expected. Do not interpret them as a test failure.
    +    // Not using File API; a potential error must trigger a PHP warning.
    +    unlink(\Drupal::root() . '/' . $this->siteDirectory . '/error.log');
    +    foreach ($this->assertions as $key => $assertion) {
    +      if (strpos($assertion['message'], 'core/modules/system/tests/modules/update_script_test/src/PathProcessor/BrokenInboundPathProcessor.php') !== FALSE) {
    +        unset($this->assertions[$key]);
    +        $this->deleteAssert($assertion['message_id']);
    +      }
    +    }
    +
    +    $this->runUpdates();
    +
    +    // Remove the simulation of the broken router, and make sure we can get to
    +    // the front page again.
    +    \Drupal::state()->set('update_script_test_broken_inbound', FALSE);
    +    $this->drupalGet('<front>');
    +    $this->assertResponse(200);
    

    <3

dawehner’s picture

FileSize
24.26 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,842 pass(es). View
4.25 KB

Addressed the feedback.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

I tried this patch againest current HEAD + #2464427-157: Replace CacheablePluginInterface with CacheableDependencyInterface. update.php ran smoothly. CacheabilityMetadataUpdateTest also passed like all the other update path tests in HEAD. We discussed this in the critical discussion call today. All the reservations are addressed and remaining are moved to followup to keep the changes minimum here. Given all that this issue is ready. Thanks @dawehner for awesome work.

catch’s picture

Just some nits, leaving at RTBC since half of these are general questions, and the other half could probably be fixed on commit.

There's a couple of interesting things in here like the fake routing setup for batch API, but those are all good workarounds and very limited to the update system so good to get this in.

  1. +++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
    @@ -0,0 +1,164 @@
    +      // Handle the actual request. In order to have batch API working we need
    +      // to use session API, see batch_process().
    

    This is no longer needed for batch, but still for access? Comment needs updating at least.

  2. +++ b/core/modules/simpletest/src/AssertContentTrait.php
    @@ -378,6 +378,30 @@ protected function assertNoLinkByHref($href, $message = '', $group = 'Other') {
    +   *   messages: use format_string() to embed variables in the message text, not
    

    Docs say format_string() but the function body says SafeMarkup::format().

  3. +++ b/core/modules/system/src/Tests/Update/UpdateScriptTest.php
    @@ -153,7 +153,7 @@ function testNoUpdateFunctionality() {
    -    $this->assertNoLinkByHref('update.php', 0);
    +    $this->assertNoLinkByHrefInMainRegion('update.php', 0);
    

    What's the functional change that actually requires this? I'd expect if there are no pending updates, there'd also be no link?

  4. +++ b/core/modules/system/system.routing.yml
    @@ -448,16 +448,14 @@ system.batch_page.json:
    +# Note: This route just exists for generating URLs, the dedicated
    

    We could probably add a follow-up issue to use internal:// or whichever one it is for unrouted internal links, but this is fine I think for keeping the patch reviewable.

dawehner’s picture

FileSize
25.42 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,840 pass(es). View
2.21 KB

This is no longer needed for batch, but still for access? Comment needs updating at least.

Yeah its authentication and general other update.php see, see interdiff.

Docs say format_string() but the function body says SafeMarkup::format().

yeah this is just a copy from some other assertion ... I'll go with suggesting strtr().

What's the functional change that actually requires this? I'd expect if there are no pending updates, there'd also be no link?

The freaking logo now links to /update.php

We could probably add a follow-up issue to use internal:// or whichever one it is for unrouted internal links, but this is fine I think for keeping the patch reviewable.

Well, we also discussed to let URLs be generated as if you would visit index.php, see #2548095: Add option to Url() to force the site base_url to be used for more discussion.

catch’s picture

The freaking logo now links to /update.php

OK so #2548095: Add option to Url() to force the site base_url to be used would fix that too I guess.

I'd be tempted to remove the new assertion, and the asserts, and just add a @todo to assert no link once that issue is fixed. There are positive tests there which are good enough. But if you disagree that's fine too.

dawehner’s picture

I'd be tempted to remove the new assertion, and the asserts, and just add a @todo to assert no link once that issue is fixed. There are positive tests there which are good enough. But if you disagree that's fine too.

Well, I did not wanted to remove the test coverage that all updates got run but yeah I mean we have implicit test coverage, this is for sure.

jibran’s picture

#100 & #102 got addressed still RTBC.

The last submitted patch, 36: 2540416-36.patch, failed testing.

  • catch committed 4983036 on 8.0.x
    Issue #2540416 by dawehner, hussainweb: Move update.php back to a front...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Thinking about it more, if the logo is going to link anywhere, linking to the front page when the site might be broken is not so great anyway.

Committed/pushed to 8.0.x, thanks!

The last submitted patch, 36: 2540416-36.patch, failed testing.

dawehner’s picture

swentel’s picture

Quick follow-up for file permissions: #2560603: update.php has 755 file permission (unless I'm missing something stupid)

Status: Fixed » Closed (fixed)

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