After #2233403: Let update.php invoke hooks again and now that update.php doesn't have any major update responsibilities, we could run update.php in a full environment.

Copy/pasting from #2233403-30: Let update.php invoke hooks again

Since we don't need to support major updates, if we wanted a full environment we could do the following:

1. Move it from a separate file to just a route.

2. Have that route
- (optionally?) put the site into maintenance mode
- collect updates + figure out order/dependencies similar to now
- run the updates in a batch

Would simplify the update runner a lot.

If you had an update that's going to completely break an install, then you'd need to tell people to do an 8-8 migration.

CommentFileSizeAuthor
#15 interdiff-2250119.txt2.34 KBParisLiakos
#15 drupal-update_php_route-2250119-15.patch70.33 KBParisLiakos
#12 drupal-update_php_route-2250119-12.patch69.96 KBdevin carlson
#8 drupal-update_php_route-2250119-8.patch70.91 KBParisLiakos
#8 interdiff.txt15.91 KBParisLiakos
#5 drupal-update_php_route-2250119-5.patch59.34 KBParisLiakos
#4 drupal-update_php_route-2250119-4.patch36.07 KBParisLiakos

Comments

catch’s picture

catch
he/him
Primary language English
commented
Priority: Major » Critical

Has to happen before release because:

1. it's a UI change

2. It'll be subtle API changes for update handlers

3. I'm not convinced update.php even works right now - given we removed all the core updates we only have some very limited test coverage.

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: +beta target

Also beta target since at that point modules can theoretically add hook_update_N().

ParisLiakos’s picture

ParisLiakos commented
Assigned: Unassigned » ParisLiakos

I already started working on this locally

ParisLiakos’s picture

ParisLiakos commented
Status: Active » Needs review
StatusFileSize
new drupal-update_php_route-2250119-4.patch36.07 KB

lets see what bot says

ParisLiakos’s picture

ParisLiakos commented
StatusFileSize
new drupal-update_php_route-2250119-5.patch59.34 KB

hm, last patch was incomplete, disregard

The last submitted patch, 4: drupal-update_php_route-2250119-4.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 5: drupal-update_php_route-2250119-5.patch, failed testing.

ParisLiakos’s picture

ParisLiakos commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt15.91 KB
new drupal-update_php_route-2250119-8.patch70.91 KB

alright.
thats a lot of deletions :)

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: +D8 upgrade path
moshe weitzman’s picture

moshe weitzman commented

Just seeing this. I'm happy to review it, if we can get a reroll. No chance it still works as well as it did in June. Thanks!

ParisLiakos’s picture

ParisLiakos commented
Assigned: ParisLiakos » Unassigned
Issue tags: +Needs reroll

wont be able to reroll till i am back, next week

devin carlson’s picture

devin carlson commented
Issue tags: -Needs reroll
StatusFileSize
new drupal-update_php_route-2250119-12.patch69.96 KB

Attempting a reroll. The only change is that DbUpdateNegotiator's methods are now passed $route_match instead of $request.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Needs work

Thanks! Just minor nits.

  1. +++ b/core/includes/theme.inc
@@ -1885,6 +1885,44 @@ function template_preprocess_container(&$variables) {
+function theme_task_list($variables) {

    Seems like this could use theme_item_list(). NIH so can be a follow-up 

  2. +++ b/core/modules/system/src/Controller/DbUpdateController.php
@@ -0,0 +1,606 @@
+  public function handle($op, Request $request) {

    $op needs Doxygen 

  3. +++ b/core/modules/system/src/Controller/DbUpdateController.php
@@ -0,0 +1,606 @@
+    // Output a list of queries executed.

    NIH, as of D7 we display messages, not queries. Would be nice to clean up code comment and variable name.

andypost’s picture

andypost
he/him
Primary language Russian
commented

For #13.1 #1222254: Remove theme_task_list() and call theme('item_list__tasks') instead.

ParisLiakos’s picture

ParisLiakos commented
Status: Needs work » Needs review
StatusFileSize
new drupal-update_php_route-2250119-15.patch70.33 KB
new interdiff-2250119.txt2.34 KB

thanks moshe for the review and Devin for the reroll!

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

Thx for fixing up the nits. This one is ready.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Looks good, nice to get this in sooner than later so we can be running with this when we start to add minor updates. Committed/pushed to 8.0.x, thanks!

  • catch committed 3afec50 on 8.0.x 
    Issue #2250119 by ParisLiakos, Devin Carlson: Run updates in a full...
scor’s picture

scor commented

Doesn't look like UPGRADE.txt was updated with this patch. It's still referring to update.php.

Nevermind, didn't realize the path was added to system.routing.yml and remained the same to match the old update.php file.

Status: Fixed » Closed (fixed)

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

kay_v’s picture

kay_v commented

Looks like update.php file is removed from /core directory and example.com/update.php returns a 404. As things stand, to invoke a database update, users need to add a trailing slash (example.com/update.php/ will work where example.com/update.php with no trailing slash returns 404).

I've poked around the issue queue a fair bit without seeing specific discussion. Looks like @scor's comment above suggests path shouldn't have special requirements. Anyone know whether there's more to consider?

Since the unique requirement may lead to some confusion, I've opened an new issue #2353907: update.php requires trailing slash or returns 404 with nginx and related it to this one.