Problem/Motivation

This is a followup of #2810381: Add generic status field to ContentEntityBase where the status field is now introduced in CEB and there is an update function to update the field storage definition comment_update_8300 which however is retrieving the old cached definition and is passing it directly to EntityDefinitionUpdateManager::updateFieldStorageDefinition without making the needed changes on it.

Proposed resolution

Make the needed changes on the cached field storage definition before passing it to EntityDefinitionUpdateManager::updateFieldStorageDefinition.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#19 2841614-19.patch974 bytesamateescu
#18 2841614-18.patch591 bytesprash_98
#5 interdiff-3-5.txt1 KBhchonov
#5 2841614-5.patch974 byteshchonov
#3 2841614-3.patch668 byteshchonov
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hchonov created an issue. See original summary.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Issue tags: +Workflow Initiative
hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
Status: Active » Needs review
FileSize
2841614-3.patch668 bytes
amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs review » Needs work 
+++ b/core/modules/comment/comment.install
@@ -190,6 +190,8 @@ function comment_update_8200() {
   $field_definitions = \Drupal::service('entity_field.manager')->getBaseFieldDefinitions('comment');
+  $field_definitions['status']->setDescription(new TranslatableMarkup('A boolean indicating the published state.'))
+    ->setRevisionable(TRUE);

We need to get the field storage definition from the entity definition update manager :)

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
Status: Needs work » Needs review
FileSize
2841614-5.patch974 bytes
interdiff-3-5.txt1 KB
amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

Looks great now!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Obviously missing test coverage then.

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.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
Version: 8.4.x-dev » 8.3.x-dev
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests

I don't see how this could be tested. The current update function uses the latest definition of the 'status' field from code, so something can fail only when that field definition is changed again.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

I'd think we could test this by introspecting the database schema after the update?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Priority: Normal » Critical

Also bumping this to critical - doesn't it mean a site gets stuck with pending schema update warnings?

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Priority: Critical » Normal

doesn't it mean a site gets stuck with pending schema update warnings?

No, the update is performed correctly, the only problem is that the "initial state" of the field definition is taken from the wrong place. An analogy to D7 would be something like: don't call hook_schema() in an update function to get the schema for creating a table, but hardcode the new table schema instead :)

hchonov’s picture

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

But after the update has ran there will be still pending updates, right? I am pretty sure that drush entity-updates will return an inconsistency and that the status field has to be updated.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented

@hchonov nope :) If that were the case, our existing update path tests would show the problem immediately.

hchonov’s picture

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

@amateescu do our update tests do the same like drush entity-update and check for inconsistencies between the cached field definitions the ones defined in the Entity::baseFieldDefinitions?

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented

@hchonov, exactly: http://cgit.drupalcode.org/drupal/tree/core/modules/system/src/Tests/Upd...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 2841614-5.patch, failed testing.

prash_98’s picture

prash_98 CreditAttribution: prash_98 commented
Status: Needs work » Needs review
FileSize
2841614-18.patch591 bytes

Please review the changes...

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community
FileSize
2841614-19.patch974 bytes

@prash_98, I don't understand that patch at all.

Re-uploading #5, which is the one that is ready for commit.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@amateescu but @catch's and @hchonov's concerns are still valid - ie. is there any way that we can have a test that fails if we make the same mistake in the future?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

  • catch committed ce215b1 on 8.4.x 
    Issue #2841614 by hchonov, amateescu: comment_update_8300 is not...
catch’s picture

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

I've opened a follow-up here #2850076: Write test coverage to demonstrate why schema definitions should not be got from code in updates.

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

  • catch committed d0b9e1c on 8.3.x 
    Issue #2841614 by hchonov, amateescu: comment_update_8300 is not...

Status: Fixed » Closed (fixed)

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