Problem/Motivation

There is currently no way to unpublish BlockContent entities. Now with #2810381: Add generic status field to ContentEntityBase we have an easy way to do this.

Proposed resolution

Add EntityPublishedTrait to BlockContent

Remaining tasks

User interface changes

None here. The patch does not make use of the publishing status, it just adds it. The default is published, so all BlockContent entities will continue to be published just as they are now. Once this is in @timmillwood plans to work on a follow up patch, but that has a lot more complexities around it.

  • How do we unpublish a block? A radio button on the BlockContent entity form? a drop button on the BlockContent entity form?
  • Permissions? Who can unpublish BlockContent? Who can view unpublished BlockContent?
  • Views integration.
  • What happens when you unpublish an already placed block?
  • Can you place unpublished blocks in a region? But they just don't show up until published.

Followup to be opened.

API changes

Data model changes

Files: 

Comments

timmillwood created an issue. See original summary.

timmillwood’s picture

Status: Active » Needs review
FileSize
1.18 KB

Initial patch to get the ball rolling, will fail due to upgrade path.

Status: Needs review » Needs work

The last submitted patch, 2: 2820848-2.patch, failed testing.

larowlan’s picture

+1 from my end

timmillwood’s picture

Status: Needs work » Needs review
FileSize
2.73 KB
1.55 KB

This should do the trick.

Will create a follow up patch to start making use of the status field on BlockContent entities.

googletorp’s picture

The patch looks good, but I can't see that the published status is actually being used for anything. Shouldn't we add some logic that unpublished blocks are invisible or is that handled by ContentEntityBase already?

timmillwood’s picture

@googletorp - You are correct, this patch does not make use of the publishing status, it just adds it. The default is published, so all BlockContent entities will continue to be published just as they are now. Once this is in I will work on a follow up patch, but that has a lot more complexities around it.

  • How do we unpublish a block? A radio button on the BlockContent entity form? a drop button on the BlockContent entity form?
  • Permissions? Who can unpublish BlockContent? Who can view unpublished BlockContent?
  • Views integration.
  • What happens when you unpublish an already placed block?
  • Can you place unpublished blocks in a region? But they just don't show up until published.
googletorp’s picture

Status: Needs review » Reviewed & tested by the community

@timmilwood Makes sense to split it up.

The patch itself looks good. Unless test bot finds something, this is good to go IMO.

Status: Reviewed & tested by the community » Needs work

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

timmillwood’s picture

Status: Needs work » Needs review
FileSize
2.54 KB
1.01 KB

$entity_type->getRevisionDataTable() returns null, but I think it's pretty safe to assume the table names are known.

googletorp’s picture

Status: Needs review » Needs work

@timmilwood

I think it would be a better option to add the revision data table to the entity type definition, that is why it's return null.

If you add

* revision_data_table = "block_content_field_revision",

to patch 5, it should work. Also has the benefits that other stuff can access the info if needed.

timmillwood’s picture

FileSize
3.27 KB
1.75 KB

That makes the update hook a little bit more complex, but should be ok.

timmillwood’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 12: 2820848-12.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
2.54 KB

The fails in #12 are due to "The SQL storage cannot change the schema for an existing entity type (block_content) with data." Therefore I think it's out of the scope of this issue to add the table name to the entity type annotation.

We should just go with the patch from #10.

Re-uploading.

timmillwood’s picture

googletorp’s picture

Status: Needs review » Reviewed & tested by the community

Patch is green and looks good. Ready to go imo.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Can't you set this via FieldableEntityInterface::set() once this patch lands? In which case code could do it and expect the block to not be shown?

Also not sure how this works between the content entity and the configuration entity for the block. Even if we don't add getters/setters, it feels in scope to sort out what actually happens once status = 0 here.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
4.92 KB
2.43 KB

In this update to the patch it prevents unpublished blocks from being rendered. There is also a simple test for this.

Berdir’s picture

+++ b/core/modules/block_content/src/Plugin/Block/BlockContentBlock.php
@@ -158,7 +158,11 @@ protected function blockAccess(AccountInterface $account) {
   public function build() {
     if ($block = $this->getEntity()) {
-      return $this->entityManager->getViewBuilder($block->getEntityTypeId())->view($block, $this->configuration['view_mode']);
+      if ($block->isPublished()) {

shouldn't this be part of accessing checking instead?

timmillwood’s picture

FileSize
4.79 KB
1.21 KB

@Berdir I guess it should, good thinking!

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

amateescu’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/block_content/block_content.install
@@ -61,3 +62,21 @@ function block_content_update_8003() {
+function block_content_update_8004() {

Actually, we need an update test that should check the existence of the new field and its default value.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
6.29 KB
1.55 KB

Adding test mentioned in #23.

timmillwood’s picture

Status: Needs review » Postponed
amateescu’s picture

Status: Postponed » Needs review
FileSize
7.39 KB
2.34 KB

That issue just got it so here's an updated patch that uses EntityPublishedInterface.

Status: Needs review » Needs work

The last submitted patch, 26: 2820848-26.patch, failed testing.

dawehner’s picture

+++ b/core/modules/block_content/src/Plugin/Block/BlockContentBlock.php
@@ -147,7 +147,8 @@ public function blockSubmit($form, FormStateInterface $form_state) {
+    $block = $this->getEntity();
+    if ($block->isPublished()) {
       return $this->getEntity()->access('view', $account, TRUE);

Should we use $block->access() here? here?

+++ b/core/modules/block_content/src/Entity/BlockContent.php
@@ -48,7 +49,8 @@
  *     "label" = "info",
  *     "langcode" = "langcode",
- *     "uuid" = "uuid"
+ *     "uuid" = "uuid",
+ *     "published" = "status",
  *   },

I would have also expected that we need to update the entity type: \Drupal\Core\Entity\EntityDefinitionUpdateManager::updateEntityType

amateescu’s picture

Status: Needs work » Needs review
FileSize
7.74 KB
1.68 KB

#28.1: Yup, we should :)
#28.2: Also yes, I was already on it when I saw the test failures.

Now there's one more thing to fix: since the new 'status' field is an entity key, it has to be created with an initial value that's not NULL, so we're bumping into #2346019: [PP-2] Handle initial values when creating a new field storage definition again :/

Status: Needs review » Needs work

The last submitted patch, 29: 2820848-29.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
7.76 KB
754 bytes

Fixing issue with \Drupal\block_content\Plugin\Block\BlockContentBlock::blockAccess returning boolean.

Status: Needs review » Needs work

The last submitted patch, 31: 2820848-31.patch, failed testing.

Berdir’s picture

Not very pretty but you can solve that with a custom storage schema class: http://cgit.drupalcode.org/file_entity/tree/src/FileEntityStorageSchema....

While adding that, we might also want to ensure we have an index on the status column so we can efficiently filter on that? Sites likely won't have a number of custom blocks where this will matter (or they will get in trouble with the amount of plugin derivatives..) but still..

timmillwood’s picture

Status: Needs work » Needs review
FileSize
8.12 KB
1.77 KB

Adding BlockContentStorageSchema.

timmillwood’s picture

Missing the storage schema, re-rolling.

The last submitted patch, 34: 2820848-34.patch, failed testing.

Gábor Hojtsy’s picture

Issue summary: View changes
FileSize
9.62 KB
1.04 KB

Reviewed this issue expecting a UI component. I see the reasons in #7, which I'm copying to the issue summary too.

Also found two nits. Instead of posting them here, I fixed them directly.

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/block_content/src/BlockContentStorageSchema.php
    @@ -0,0 +1,41 @@
    +
    +    if ($storage_definition->getName() == 'status') {
    +      $schema['fields']['status']['initial'] = 1;
    

    can we add a comment for this?

  2. +++ b/core/modules/block_content/src/BlockContentStorageSchema.php
    @@ -0,0 +1,41 @@
    +
    +    $schema['block_content_field_data']['indexes'] += [
    +      'block_content__status' => ['status']
    +    ];
    +    $schema['block_content_field_revision']['indexes'] += [
    +      'block_content__status' => ['status']
    +    ];
    

    wondering if we even need to define that explicitly, actually. we get a not null constraint on it because it is an entity key, not sure if we also get an index automatically?

  3. +++ b/core/modules/block_content/src/Plugin/Block/BlockContentBlock.php
    @@ -147,8 +147,8 @@ public function blockSubmit($form, FormStateInterface $form_state) {
       protected function blockAccess(AccountInterface $account) {
    -    if ($this->getEntity()) {
    -      return $this->getEntity()->access('view', $account, TRUE);
    +    if ($block = $this->getEntity()) {
    +      return AccessResult::allowedIf($block->isPublished() && $block->access('view', $account, TRUE));
         }
         return AccessResult::forbidden();
       }
    

    Actually looking at this, shouldn't the status check be inside BlockContentAccessControlHandler then?

    Right now we don't have the necessary cacheablity metadata on this.

amateescu’s picture

+++ b/core/modules/block_content/src/BlockContentStorageSchema.php
@@ -0,0 +1,41 @@
+  protected function getSharedTableFieldSchema(FieldStorageDefinitionInterface $storage_definition, $table_name, array $column_mapping) {
+    $schema = parent::getSharedTableFieldSchema($storage_definition, $table_name, $column_mapping);
+
+    if ($storage_definition->getName() == 'status') {
+      $schema['fields']['status']['initial'] = 1;
+    }
+
+    return $schema;
+  }

I don't think an index like this is useful on its own, instead we want one which also contains the bundle and ID keys, like Node has.

Opened #2834291: Add a SQL index for entity types that are using EntityPublishedInterface for that :)

heddn’s picture

Rather than assuming that unpublished should have access denied, I think node module adds a permission. Can we do similar? I can easily see where someone might want to assign this as a permission to certain roles of users.

Berdir’s picture

This is a bit different though. We're not denying access to anyone who can access them in the backend (there is only one administer-level permission for this anyway).

And most sites will have their node listings/views configured to not show unpublished, even if the current user has access. I think it is the same here, if you want to hide a block then it should be hidden for everyone.

heddn’s picture

Thanks for that clarification. I just got bitten by block module's rather restrictive access check and didn't want to see the same thing surfaced here.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
9.18 KB
2.62 KB

Quick update addressing items from #38.

heddn’s picture

+++ b/core/modules/block_content/src/BlockContentAccessControlHandler.php
@@ -19,7 +19,7 @@ class BlockContentAccessControlHandler extends EntityAccessControlHandler {
+      return AccessResult::allowedIf($entity->isPublished());

After looking at the code here, I still stand by my earlier comment. If I place the block unpublished on a page, I want to send that page's link to my editor and have them see the page, even the unpublished block. But still hide the block for anonymous users and even the node from anonymous users. From my looking at the code and what I'm facing with Block module, I do not think this is possible without overriding the access control handler and removing the status check. Basically reversing this logic.

dixon_’s picture

Issue summary: View changes
dixon_’s picture

If I place the block unpublished on a page, I want to send that page's link to my editor and have them see the page, even the unpublished block.

The idea with the Workflow Initiative is to introduce "workspaces" in order to more efficiently provide full site preview. This has several advantages over managing access control on a "live" page.
So the idea is that you'd be sending a link to a "stage" workspace where your block is published. After verification you'd then be publishing/replicating the stage workspace to the "live" workspace.

While workspaces aren't in core yet, you can use the Workspace contrib module to do this already.

timmillwood’s picture

I see this as a first step. As you see we're not even adding a UI to unpublish block content.

We could look at adding a view unpublished block content permission in a follow up.

heddn’s picture

I'm willing to not bikeshed, especially if the follow-up is created. Does this need a CR?

andypost’s picture

I'd better introduce the permission here because it looks incomplete to add feature without any way for user to get why the block is not displayed

Also why no changes in block listing? I suppose there should be a filter and column to display status/published (what about draft state?)

Maybe better to disallow placing unpublished block? Or just use some CSS like unpublished nodes using?

timmillwood’s picture

I'd really like to see all these as folllow up issues. Lets get the API change in, and make sure if if something uses the API to unpublish block content it doesn't get seen, then go through all the follow up items:

  • UI to unpublish blocks
  • UI to publish blocks
  • Permission to view unpublished block content
  • Views integration
  • Bulk operation to publish / unpublish blocks
  • Don't show unpublished blocks in the block listing?
  • Display the publishing status in the block listing?
  • Disallow placing unpublished blocks?
  • Show unpublished blocks with a red background?
  • etc
  • etc
Manuel Garcia’s picture

Although the permission could be added here, to me it would make more sense to do so on the issue for the UI, since that's where the permission would be checked, not if you do $block->setPublished(FALSE); in your code.

I agree this definitely needs followups, and it'd be great to open some of them.

To me it's fine if we get the patch in as is, because:

  • Since the default is published, all BlockContent entities will continue to work just as they are now, unless you execute code to unpublish it.
  • We can then work on all the follow ups in parallel, for example, views integration and UI to publish/unpublish + permissions could definitely be done at the same time.
dixon_’s picture

Status: Needs review » Reviewed & tested by the community

The code looks good, and all the review items are addressed. There's consensus about breaking out remaining bits into follow-up issues. So I created these issues:

claudiu.cristea’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/block_content/block_content.install
    @@ -61,3 +62,26 @@ function block_content_update_8003() {
    +function block_content_update_8004() {
    

    This cannot be 8004. According to hook_update_N() docs, should be 8300 as is the first for 8.3.x branch.

    Also, I think we should include the function in @addtogroup block, see system.install for an example.

  2. +++ b/core/modules/block_content/block_content.install
    @@ -61,3 +62,26 @@ function block_content_update_8003() {
    +  $entity_type = \Drupal::entityDefinitionUpdateManager()->getEntityType('block_content');
    ...
    +  \Drupal::entityDefinitionUpdateManager()->updateEntityType($entity_type);
    ...
    +  \Drupal::entityDefinitionUpdateManager()
    

    We call this service 3 times. Let's set it in variable $manager at the top and reuse it.

  3. +++ b/core/modules/block_content/block_content.install
    @@ -61,3 +62,26 @@ function block_content_update_8003() {
    +
    

    Extra new line.

  4. +++ b/core/modules/block_content/src/BlockContentAccessControlHandler.php
    @@ -19,7 +19,7 @@ class BlockContentAccessControlHandler extends EntityAccessControlHandler {
    -      return AccessResult::allowed();
    +      return AccessResult::allowedIf($entity->isPublished());
    

    Nice that is tested.

  5. +++ b/core/modules/block_content/src/Tests/Update/BlockContentUpdateTest.php
    @@ -0,0 +1,41 @@
    + */
    ...
    +    $this->assertIdentical(NULL, $field);
    

    $this->assertNull() ?

  6. +++ b/core/modules/block_content/src/Tests/Update/BlockContentUpdateTest.php
    @@ -0,0 +1,41 @@
    +   * @see block_content_update_8004()
    

    See ^^^

  7. +++ b/core/modules/block_content/tests/src/Functional/UnpublishedBlockTest.php
    @@ -0,0 +1,46 @@
    +class UnpublishedBlockTest extends BrowserTestBase {
    

    The test looks good by I wonder if we need a browser test (which is expensive) or we can just test this in a kernel test, by using only the renderer service. I'm not saying it works, just thinking...

timmillwood’s picture

Status: Needs work » Needs review
FileSize
9.22 KB
2.45 KB

Addressing all items in #53.

#53.7 - BrowserTestBase allows us to test BlockContentAccessControlHandler as it's called when rendering the page, so I think it gives the best coverage.

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me if bot agrees.

claudiu.cristea’s picture

Assigned: timmillwood » Unassigned
Berdir’s picture

Status: Reviewed & tested by the community » Needs work

One thing that I just noticed, how does this interact with translation status? content_translation has some interesting dynamic logic around metadata fields like status, created/changed and so on. Basically, if it's not defined, then it defines custom content_translation fields automatically. So if you start to define it, then it affects those additional base fields.

Looking at \Drupal\content_translation\ContentTranslationHandler::getFieldDefinitions(), it actually looks at the status key, not the new published, despite actually calling it "hasPublishedStatus()" :)

That means multilingual sites end up with status and content_translation_status and that could lead to some fun problems/data duplication.

I think we should consider to add the status key as well and deal with that. That IMHO means writing an optional data migration/update function for the existing content_translation_status field if it exists. That said, is that a BC problem? I guess we also just found out that the status key *is* pre-exsting and *is* actually used for content entities, and adding a new published ey might not have been the best idea after all? :)

PS: Sorry.

PPS: This affects comment too. Sorry again. Edit: Not sure about comment actually, we didn't add the new status field, but it also didn't have the status key before. So on the plus side, likely not a regression of the EntityPublished trait issue, but likely broken/weird anyway.

timmillwood’s picture

I've been looking into the content_translation that @Berdir discovered and have some good news, and bad news.

Good news
It's not an issue for comments. This is because \Drupal\content_translation\ContentTranslationHandler::hasPublishedStatus doesn't look for the 'status' entity key, it just looks for a field called 'status'.

Bad news
This is an issue for block_content. The patch in #54 adds the 'published' entity key with a value 'status'. So block_content will get a field called 'status' and \Drupal\content_translation\ContentTranslationHandler::hasPublishedStatus will return TRUE. This means we will have a content_translation_status field on block_content that isn't used / needed anymore. What do we do?

Should we:

  • Set the value of 'status' to the value of 'content_translation_status' and remove the 'content_translation_status' field?
  • Set the value of 'status' to the value of 'content_translation_status' and leave the 'content_translation_status' field there?

Then, do we update \Drupal\content_translation\ContentTranslationHandler::hasPublishedStatus to use the 'published' entity key if it has one, or 'status' if it hasn't (just for BC reasons)?

Gábor Hojtsy’s picture

IMHO we should remove the now unneeded translation status field after data is moved over. Not sure how changing the logic in \Drupal\content_translation\ContentTranslationHandler::hasPublishedStatus change things for the future given that we are newly adding a publication status field here, so this problem would have shown up either way? It does not seem to be a related problem that that code does not check the entity key but hardcodes a name.

timmillwood’s picture

@Gabor - The logic in \Drupal\content_translation\ContentTranslationHandler::hasPublishedStatus only becomes an issue if the field is not called 'status' for example if the entity key was "published" = "published", then 'content_translation_status' would still get added, even though there was a published status field.

Gábor Hojtsy’s picture

Right, which is why its unrelated here IMHO.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
9.87 KB
2.52 KB

Using 'content_translation_status' as the initial from field if it exists, then dropping it once we're done.

Berdir’s picture

+++ b/core/modules/block_content/block_content.install
@@ -89,6 +89,13 @@ function block_content_update_8300() {
+  $db_schema = \Drupal::database()->schema();
+  if ($db_schema->fieldExists('block_content_field_data', 'content_translation_status')) {
+    $db_schema->dropField('block_content_field_data', 'content_translation_status');
+  }
+  if ($db_schema->fieldExists('block_content_field_revision', 'content_translation_status')) {
+    $db_schema->dropField('block_content_field_revision', 'content_translation_status');
+  }

Don't we need to do this with uninstallFieldStorageDefinition(), otherwise Drupal will think that they are still installed?

Berdir’s picture

Status: Needs review » Needs work

Applying this patch and running the updates without clearing the cache first results in this error:

Exception thrown while performing a schema update. SQLSTATE[22004]: Null value not allowed: 1138 Invalid use of NULL value: ALTER TABLE {block_content_field_data} CHANGE `status`[error]
`status` TINYINT NOT NULL; Array
(
)

That's because installing the field storage definition still uses the old cached entity entity type without the new storage schema class. We need to clear the cached entity type definitions first. We don't have this in the tests because our tests start with cold caches, which I think is wrong, we had this problem before.

Might not actually be true, see below.

After cache clear, I still get that error, but on the revision table now:

Do you wish to run all pending updates? (y/n): y
Exception thrown while performing a schema update. SQLSTATE[22004]: Null value not allowed: 1138 Invalid use of NULL value: ALTER TABLE {block_content_field_revision} CHANGE [error]
`status` `status` TINYINT NOT NULL; Array
(
)

I do have content_translation installed and enabled for block_content. It did run through on the second try. But that's just because it just failed on making the field not null. The reason why this happens is that content_translation_status is not "not null" and doesn't need initialization and also doesn't have it. So because I only enabled content translation after I already had blocks, I had NULL in there for an existing block. That was used as source for the new status field, and then it fails on that.

That''s a pretty annoying problem. I think we need to do a conditional update query to set it to 1 if it is NULL first if the content_translation_status field exists (this is probably a bug in content_translation, but we need to deal with that data now anyway unless we fix this first in a separate issue and block it on that)

Also, I do get this after it did run through:

$ drush entity-updates
The following updates are pending:

block_content entity type :
The Translation status field needs to be uninstalled.
The Publishing status field needs to be updated.

So as I assumed above, it thinks that the translation status is still there, we must use uninstallFieldStorageDefinition().

That said, the migration of the data seems to have worked as expected, I had a custom block with original language EN and an unpublished DE translation. The status there is now correct for that.

Another weird thing is that my pre-existing block that now has status NULL seems to be confused and doesn't even allow me to translate it, despite having a language set.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
10.62 KB
1.72 KB

This patch updates all NULL content_translation_status fields to 1, and uninstalls the field storage definition at the end.

claudiu.cristea’s picture

  1. +++ b/core/modules/block_content/block_content.install
    @@ -61,3 +62,59 @@ function block_content_update_8003() {
    +      ->where('content_translation_status', NULL)
    ...
    +      ->where('content_translation_status', NULL)
    

    We use isNull('content_translation_status') for such conditions.

  2. +++ b/core/modules/block_content/block_content.install
    @@ -61,3 +62,59 @@ function block_content_update_8003() {
    +  $entity_type->setHandlerClass('storage_schema', 'Drupal\block_content\BlockContentStorageSchema');
    

    Let's not hard-code the namespaced class name. Use BlockContentStorageSchema::class.

Status: Needs review » Needs work

The last submitted patch, 65: 2820848-65.patch, failed testing.

timmillwood’s picture

Fixing failing tests and items from #66

claudiu.cristea’s picture

Status: Needs work » Needs review
claudiu.cristea’s picture

Issue tags: +Needs manual testing

In #64, @Berdir has tested manually and found errors not reported by our update test. I would RTBC this again but we still need a confirmation from a manual test as long as this kind of errors are not poping-up in automated tests. Tagging with 'Needs manual testing'

Berdir’s picture

  1. +++ b/core/modules/block_content/block_content.install
    @@ -61,3 +64,61 @@ function block_content_update_8003() {
    +  if ($db_schema->fieldExists('block_content_field_data', 'content_translation_status')) {
    +    $db->update('block_content_field_data', 'bcfd')
    +      ->fields(['content_translation_status' => 1])
    +      ->isNull('content_translation_status')
    +      ->execute();
    +  }
    +  if ($db_schema->fieldExists('block_content_field_revision', 'content_translation_status')) {
    

    Can we add a comment to this?

    I also think that the 'bcfd' is not necessary and a bit confusing. We also afaik usually don't use $db but $database. On the other side, I'd be OK with just $schema.

  2. +++ b/core/modules/block_content/block_content.install
    @@ -61,3 +64,61 @@ function block_content_update_8003() {
    +  if ($db_schema->fieldExists('block_content_field_data', 'content_translation_status')) {
    +    $db_schema->dropField('block_content_field_data', 'content_translation_status');
    +  }
    +  if ($db_schema->fieldExists('block_content_field_revision', 'content_translation_status')) {
    +    $db_schema->dropField('block_content_field_revision', 'content_translation_status');
    +  }
    +}
    

    the uninstall above should take care of this.

Manually tested with the following steps:

1. new installation with EN
2. Created a custom block
3. Added DE and FR
4. Made custom blocks translatable.
5. Added a DE translation. Was forced to have it published, got this message: "Only this translation is published. You must publish at least one more translation to unpublish this one.". Except that's weird because the default translation is supposed to be published. This is what #2346019: [PP-2] Handle initial values when creating a new field storage definition should address by ensuring that the existing default translation is published when making something translatable and there is no existing status/published key. I think we should also have another follow-up to support the published entity key and not hardcode the "status" field in Content Translation. Of course that will be tricky if there is an existing entity type with published that is not status.
6. Added a FR translation, published. Still getting that message because I apparently need to have more than one existing published, I think this isn't working correctly for new translations at all :)
7. Created a second block, DE.
8. Created an EN translation. Now I can select unpublished, already on the first, because the original is published, this was cloned from it, so technically, with the new not yet saved translation, we *do* have two published translations. This is much fun ;) And I guess means that fixing the initial value will fix this bug too. So unpublished it is.
9. Created a FR translation, published.
10. Apply the patch, remove the explicit dropField() calls to ensure that I'm right there.
11. Run drush updb, BOOM. db_update() does not have an alias argument, So 1. above is not just unnecessary, it is clearly wrong.
12. Removed, executed again. next BOOM: Unable to delete a field (content_translation_status in block_content entity) with data that cannot be purged. That means we now actually need to explicitly set that column to NULL, so we can delete it.
13. Trying that, commenting out the installFieldStorageDefinition() call because that executed and would explode now.
14. Third time is a charm: drush updb did run through this time. Status is displayed correctly in the UI and is correct in the DB in both tables. content_translation_status field is gone in both tables.

You're going to .. not like me for that very much, but given all those pitfalls/bugs/complexities, I don't think we can get this in without actual test coverage for an existing site with custom block translations?

Berdir’s picture

Status: Needs review » Needs work
timmillwood’s picture

Status: Needs work » Needs review
Issue tags: -Needs manual testing
FileSize
12.28 KB
5.17 KB

Started looking at the items from #71 and how we can test this.

Still lots to do.

timmillwood’s picture

FileSize
13.13 KB
6.7 KB

Manually adding the content_translation_status field at the start of the test, then testing it is removed as part of the update.

This still doesn't test the full process from #71, this proved to be pretty complex to replicate, but I think this goes a good way towards testing the update path.

dawehner’s picture

This is just some quick review. IMHO its a bit confusing that block config entities aren't publish/unpublishable but the underlying custom content types are. That's a bit less obvious maybe for someone new who comes into the system.

  1. +++ b/core/modules/block_content/src/BlockContentStorageSchema.php
    @@ -0,0 +1,32 @@
    +
    +    if ($storage_definition->getName() == 'status') {
    +      if (\Drupal::database()->schema()->fieldExists('block_content_field_data', 'content_translation_status')) {
    +        $schema['fields']['status']['initial_from_field'] = 'content_translation_status';
    +      }
    +      else {
    +        // Set all block content as published.
    +        $schema['fields']['status']['initial'] = 1;
    +      }
    +    }
    +
    

    This IMHO needs some serious explanation of what is going on.

  2. +++ b/core/modules/block_content/src/Tests/Update/BlockContentUpdateTest.php
    @@ -0,0 +1,73 @@
    +    $this->databaseDumpFiles = [
    +      __DIR__ . '/../../../../system/tests/fixtures/update/drupal-8-rc1.filled.standard.php.gz',
    +    ];
    

    Isn't it funny how we still migrate starting from a 8.0-rc1 instance?

timmillwood’s picture

#75.1 - ah yes, I'll add some comments.
#75.2 - is it, but not something to fix in this issue.

timmillwood’s picture

FileSize
13.18 KB
1.55 KB

Updating comments in BlockContentStorageSchema

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This looks rocksolid for me now. I wondered for a while whether removing the field is the right idea, but indeed that's totally fine due to

  if (!$this->hasPublishedStatus()) {
      $definitions['content_translation_status'] = BaseFieldDefinition::create('boolean')
        ->setLabel(t('Translation status'))
        ->setDescription(t('A boolean indicating whether the translation is visible to non-translators.'))
        ->setDefaultValue(TRUE)
        ->setRevisionable(TRUE)
        ->setTranslatable(TRUE);
    }

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 77: 2820848-77.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
13.23 KB
814 bytes

Allowing users with the "Administer blocks" permission to view unpublished block content. This kinda makes sense seeing as they need to administer them. It also fixes the failing test in #77 from \Drupal\content_moderation\Tests\ModerationStateBlockTest::testCustomBlockModeration which assumes a "root user" can view unpublished blocks.

Manuel Garcia’s picture

Status: Needs review » Reviewed & tested by the community

#80 makes sense, back to RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 80: 2820848-80.patch, failed testing.

timmillwood’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 80: 2820848-80.patch, failed testing.

timmillwood’s picture

Status: Needs work » Reviewed & tested by the community

Unrelated fail.

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

Status: Reviewed & tested by the community » Needs review

The update function makes a few risky assumptions: that the block_content entity is translatable, stored in SQL and that it is using the default table names.

I'd like to take a closer look at the content_translation_status situation before this gets in..