Problem/Motivation

This is a follow-up of #2851529: DatabaseCacheTagsChecksum is misbehaving because of the static cache. Currently DbUpdateController::triggerBatch is only flushing all the caches and invalidating the container and relies on the fact that there will be a new request where the container will be newly build, however in order to unify the update process we should directly rebuild the container.

This is postponed on #2853152: drupal_rebuild should rebuild the container instead of invalidating it only in order to make it possible to get the new container in the same process/request.

Proposed resolution

After drupal_flush_all_caches rebuild the container explicitly in order to be clear that the container has to be rebuild before executing the post updates.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#6 interdiff-4-5.txt576 byteshchonov
#6 2853153-5.patch1.63 KBhchonov
#4 2853153-4.patch1.94 KBhchonov
#2 2853153-2.patch867 byteshchonov
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hchonov created an issue. See original summary.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
FileSize
2853153-2.patch867 bytes

Something like this.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented 
+++ b/core/modules/system/src/Controller/DbUpdateController.php
@@ -611,7 +611,9 @@ protected function triggerBatch(Request $request) {
+      require_once $this->root . '/core/includes/utility.inc';

Instead of requiring the file directly this should be added as $batch['file'] so it is included before actually executing the operation.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
Category: Bug report » Task
Priority: Major » Minor
Issue summary: View changes
Status: Postponed » Needs review
FileSize
2853153-4.patch1.94 KB

Actually I think we could do it the same way as you've done in your pull request for drush and close the other issue. This is also only an improvement for better understanding the update process so I think it is a task and minor.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented

Looks good to me, but should probably be looked at by someone a bit more versed with the update / batch system.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
FileSize
2853153-5.patch1.63 KB
interdiff-4-5.txt576 bytes

I've added the interface by mistake.

The last submitted patch, 4: 2853153-4.patch, failed testing.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented

Hmm this is strange - the update tests are now failing. I thought there should be no difference if we rebuild the container in the request prior to the first post update or in the request of the first post update.

Status: Needs review » Needs work

The last submitted patch, 6: 2853153-5.patch, failed testing.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented

@hchonov, do you still think this should be done or can we close it?

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented

@amateescu, I don't think that it is necessary. The only reason for rebuilding the container explicitly would be for consistency with the accepted change in Drush. See https://github.com/drush-ops/drush/pull/2620/files

However I don't have strong opinion about this and if someone considers this an unnecessary change, then the issue can be closed.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs work » Closed (outdated)

Ok, let's close it then. Our update path tests are quite sensitive to container rebuilds (as discovered in #3006086: update.php should not process path aliases), so let's not try to fix something that works :)