Problem/Motivation

There are many reports about block_content_update_8400() being broken because of

1) its dependency on content_translation_update_8400()
2) its usage of \Drupal::moduleHandler()->moduleExists('content_translation') which returns FALSE when run during the update process, but returns TRUE when run on the live codebase (e.g. via Drush)

Some of the problems with that update function were fixed in #2951242: Allow BaseFieldDefinition::setInitialValueFromField() to set a default value - this fixes issues with block_content_update_8400(), but the broken dependency on Content Translation is still there, and it still affects people who are updating to 8.5.6.

taxonomy_update_8601() will likely have the same kind of issues, so let's fix them both at the same time.

Proposed resolution

Do not rely on the return value of \Drupal::moduleHandler()->moduleExists('content_translation') in those two update functions, and keep only the check for the existence of a content_translation_status field.

Remaining tasks

Review.

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope.

CommentFileSizeAuthor
#2 2991902.patch2.3 KBamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu created an issue. See original summary.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Active » Needs review
FileSize
2991902.patch2.3 KB

Once this minimally invasive patch gets in, we should try to get to the bottom of the problem with \Drupal::moduleHandler()->moduleExists() returning conflicting values when run during database updates vs. regular site operation.

FWIW, I think this should be backported to the 8.5 branch if we ever release a final 8.5 patch version before 8.6.0.

0Sarah0Al’s picture

0Sarah0Al CreditAttribution: 0Sarah0Al commented

Thanks amateescu :)

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented 
+++ b/core/modules/block_content/block_content.install
@@ -9,22 +9,6 @@
- * Implements hook_update_dependencies().
- */
-function block_content_update_dependencies() {
-  // The update function that adds the status field must run after
-  // content_translation_update_8400() which fixes NULL values for the
-  // 'content_translation_status' field.
-  if (\Drupal::moduleHandler()->moduleExists('content_translation')) {
-    $dependencies['block_content'][8400] = [
-      'content_translation' => 8400,
-    ];
-

Hm. So the point of this is that if a site is updating from an old version, to make sure that we are fixing the content_translation_status fields first, and only after that make them required. AFAIK, if you manage to run e.g. block_content_update_8400() first, things would be really broken.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented

@Berdir, block_content_update_8400() doesn't depend on content_translation_update_8400() anymore since #2951242: Allow BaseFieldDefinition::setInitialValueFromField() to set a default value - this fixes issues with block_content_update_8400(), because NULL values in content_translation_status are now properly initialized with status = 1. This dependency should have been removed in that issue, we just forgot about it..

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

Good point. Then lets do this. As you said, lets avoid those crazy problems we had :)

  • catch committed df1de90 on 8.7.x 
    Issue #2991902 by amateescu: Fix block_content_update_8400() and...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.7.x-dev » 8.5.x-dev

Committed/pushed to 8.7.x and 8.6.x, leaving open against 8.5.x

  • catch committed d567122 on 8.6.x 
    Issue #2991902 by amateescu: Fix block_content_update_8400() and...

  • catch committed 4e720e9 on 8.5.x 
    Issue #2991902 by amateescu: Fix block_content_update_8400() to not rely...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

OK committed/pushed to 8.5.x (without the taxonomy hunk) too.

Status: Fixed » Closed (fixed)

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