Make BlockContent entities publishable

+1 from my end

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?

@timmilwood Makes sense to split it up.

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

@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.

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

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.

+++ 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?

Looks good to me.

+++ 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.

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

+++ 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

#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: Handle initial values when creating a new field storage definition again :/

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..

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.

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.

+++ 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 :)

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.

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.

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.

+++ 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.

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.

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

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?

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.

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:

• #2834546: UI for publishing/unpublishing block_content blocks
• #2834555: Views integration for block publishing status

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...

Looks good to me if bot agrees.

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.

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.

Right, which is why its unrelated here IMHO.

+++ 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?

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.

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.

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'

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: 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?

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?

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);
    }

#80 makes sense, back to RTBC

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..

Ok, I posted #2854732: Use initial values for content translation metadata fields and fix existing data for handling the 'content_translation_status' problem generically, so I'm afraid this will need to be postponed on that one and its chain of dependencies :(

Now that we have good patches for all the dependencies, I also updated this one and cleaned up the entity type and the update function. I didn't look at the tests yet though.

This is postponed on:
#2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field
#2346019: Handle initial values when creating a new field storage definition
#2856808: Break out the 'entity_test_update' entity type into its own module and add additional test db dumps
#2854732: Use initial values for content translation metadata fields and fix existing data
#2825973: Introduce a EditorialContentEntityBase class for revisionable and publishable entity types

Counted an issue twice :/

All the dependencies have patches that are passing tests, so let's try a combined patch here as well.

This is #89 + the combined patch from #2854732-9: Use initial values for content translation metadata fields and fix existing data and #2825973-31: Introduce a EditorialContentEntityBase class for revisionable and publishable entity types.

One of the problems is #2858109: The BlockContent entity type does not specify its revision data table, another one is fixed in the patch attached (there's no reason to update a field that we just installed) and there's one more bug that will probably need to be fixed in #2346019: Handle initial values when creating a new field storage definition, still investigating that one.

+++ b/core/modules/block_content/src/Tests/Update/BlockContentUpdateTest.php
@@ -0,0 +1,73 @@
+ * Tests that block content settings are properly updated during database updates.

Line contain more than 80 characters

There is one minor of "line containing more than 80 characters" please review.

Applying patch for comment #95, please review

Added new line between the description and @group.

There is a reason why I didn't set the patch from #93 to be tested, and that's because it won't work without all the issues listed in #89.

Can we please stop making cosmetic changes like this to a patch when they really are not necessary? A one-line review would have been more than welcome..

Let's see how far this gets us. It includes the latest patch from #2346019-43: Handle initial values when creating a new field storage definition and also fixes the tests that were added here previously, some of which didn't make sense at all.

One of the dependencies was committed: #2858109: The BlockContent entity type does not specify its revision data table. We're... "getting there" :)

#2856808: Break out the 'entity_test_update' entity type into its own module and add additional test db dumps is fixed.

#2825973: Introduce a EditorialContentEntityBase class for revisionable and publishable entity types is in!

#2346019: Handle initial values when creating a new field storage definition and #2854732: Use initial values for content translation metadata fields and fix existing data are the two dependencies left, so here's #105 combined with them.

There's only one dependency left.

#2854732: Use initial values for content translation metadata fields and fix existing data is in now, so this is finally unblocked!

@Berdir, note that the issue mentioned above is the one that fixed the problem(s) you outlined in the manual testing from #64 / #71! You're more than welcome to take a look at the test coverage added in there and see if it addresses all your concerns :)

Whoa, hold on there :) I didn't get to review the entire patch myself yet until now, so here's some fixes for what I could find.

Also, the most important part is that I think we need a review from a block system maintainer. We need to be sure that the new concept of a custom block being un/publishable in the API works well together with the rest of the block system - block plugins, disabled regions or whatever mechanism exist these days for disabling a block on a page. Trying @tim.plunkett first :)

FWIW in #4 @larowlan already +1 the idea. He is a maintainer for block content.

We need to be sure that the new concept of a custom block being un/publishable in the API works well together with the rest of the block system - block plugins, disabled regions or whatever mechanism exist these days for disabling a block on a page.

I think this is worth exploring.

re #112 the UI side of this will be done over at #2834546: UI for publishing/unpublishing block_content blocks - I got started on that a while back, but postponed until we get this =)

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

This is missing cacheability metadata.

I suggest:

return AccessResult::allowedIfHasPermission($account, 'administer blocks')
  ->orIf(AccessResult::allowedIf($entity->isPublished())->addCacheableDependency($entity));

EDIT: or this, which puts the more likely thing first, hence slightly faster for the common case:

return AccessResult::allowedIf($entity->isPublished())->addCacheableDependency($entity)
  ->orIf(AccessResult::allowedIfHasPermission($account, 'administer blocks'));

@Wim Leers, I was also looking at that access handler and wondered about cacheability metadata but then I got distracted and went on to fix something else. Glad you were vigilant though :)

@jibran, it's great that @larowlan +1`ed the idea initially, but we still need a careful review for the API-facing parts of this patch.

+++ b/core/modules/block_content/src/BlockContentAccessControlHandler.php
@@ -19,7 +19,8 @@ class BlockContentAccessControlHandler extends EntityAccessControlHandler {
-      return AccessResult::allowed();
+      return AccessResult::allowedIf($entity->isPublished())->addCacheableDependency($entity)
+        ->orIf(AccessResult::allowedIfHasPermission($account, 'administer blocks'));

However, looking at the diff again, where did the new 'administer blocks' permission requirement come from? And why wasn't it needed before this patch?

Oh, that I have no idea about. I was just reviewing #113 :)

core/modules/block_content/src/Entity/BlockContent.php line 34 has * admin_permission = "administer blocks",

\Drupal\Core\Entity\EntityAccessControlHandler::checkAccess() has this line:
return AccessResult::allowedIfHasPermission($account, $this->entityType->getAdminPermission());

+++ b/core/modules/block_content/src/BlockContentAccessControlHandler.php
@@ -19,7 +19,8 @@ class BlockContentAccessControlHandler extends EntityAccessControlHandler {
     if ($operation === 'view') {
-      return AccessResult::allowed();
...
     return parent::checkAccess($entity, $operation, $account);

In HEAD, this skipped the permission check for $operation == 'view', which was correct (anyone could view a content block)
Now this is allowing those with admin perms to bypass the publish check. Which is good.

This whole thing could be rewritten to:

$access = parent::checkAccess($entity, $operation, $account);
if ($operation === 'view') {
  $access->orIf(AccessResult::allowedIf($entity->isPublished())->addCacheableDependency($entity));
}
return $access;

This was added in #80.

However, I'm wondering if we really want that? That means if I unpublish a block_content entity then I will not be able to see a difference to me as an admin, it will still be displayed.

Yes, we do the same with nodes, but only if you specifically view them, the default /node view excludes unpublished content. And I'm sure practially all public lists of content are configured like that. And /node/ID has a background to indicate that it is unpublished (yeah, it is ugly but that's another story).

So maybe just add permission "view unpublished blocks" for parity.
the same pattern used for menu
Also that will make edotor's exp a bit happy

No, I'm saying don't add anything. At least not here. Unpublished fields still show up in the block content overview and can be seen and edited by anyone with administer blocks just fine, which is IMHO enough for this issue.

Re permissions, there's this ongoing effort fyi: #2809177: Introduce entity permission providers

@timmillwood, on the contrary, with this patch applied $content_translation_manager->getSupportedEntityTypes() does not return entity_test_update entity type in content_translation_update_8400(), even though it is very much supposed to do so.

The problem stems from the new block_content_update_dependencies() function, which seems to make the entity manager use some stale definitions. I tried a lot to debug what is going on but the call stacks are way too high to be understandable. It can be easily fixed by clearing the entity type definitions cache though :)

👍

+++ b/core/modules/block_content/src/BlockContentAccessControlHandler.php
@@ -19,7 +19,8 @@ class BlockContentAccessControlHandler extends EntityAccessControlHandler {
   protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
     if ($operation === 'view') {
-      return AccessResult::allowed();
+      return AccessResult::allowedIf($entity->isPublished())->addCacheableDependency($entity)
+        ->orIf(AccessResult::allowedIfHasPermission($account, 'administer blocks'));
     }

We still have this part now, as commented, my recommendation would be to remove that.

There are various options, like visually marking unpublished blocks for admins, but for now, we don't offer a UI for this anyway, so it seems like the safest option is to just not change it here?

That's what I thought as well, we don't need this until we figure out the UI bits.

Berdir++ for simplifying access control handler/keeping scope tight.

So \Drupal\Tests\content_moderation\Functional\ModerationStateBlockTest::testCustomBlockModeration() is on purpose creating a draft/unpublished block_content entity and then expects that the user can see it.

Which why @timmillwood added the permission check there.

So 1 million dollar question is... is that really what you expect to happen or not? :)

In #122 I agreed that the permission check is good. I think the test is correct.

Discussed a bit with @timplunkett a bit about that. Lets keep the check to keep the test passing for now, and then we can figure out something that makes sense in #2834546: UI for publishing/unpublishing block_content blocks. For now, creating a new draft custom block and then immediately doing a block placement of that seems like a very edge-cache situation either way.

Also when you add workspaces to the mix and you are looking at a future draft revision of a block...

Back to RTBC for the patch in #129, just re-uploading to make it clear for testbot.

That was a HEAD fail from #2894499-17: Rename 'Editorial workflow' to 'Editorial'.

Similarly to #2880154-26: Convert comments to be revisionable, we'll evaluate it for backport after commit. This is another strategic priority for workflow so assuming we don't identify any risks during review, I'd definitely consider this for backport if it lands in time for the beta freeze deadline.

+++ b/core/modules/block_content/src/BlockContentAccessControlHandler.php
@@ -19,7 +19,8 @@ class BlockContentAccessControlHandler extends EntityAccessControlHandler {
+      return AccessResult::allowedIf($entity->isPublished())->addCacheableDependency($entity)
+        ->orIf(AccessResult::allowedIfHasPermission($account, 'administer blocks'));

I think we're missing explicit coverage for the admin portion of this

Other than that, agree - this is low risk.

Note: needs reroll, no longer applies