UI for publishing/unpublishing block_content blocks

Attached patch does the following:

Changes to block_content view:

• Added a new Published column
• Added a new exposed filter for Published
• Added integration with bulk operations for publishing / unpublishing block_content blocks

Changes to the block_content form:

• Added buttons for saving as published / unpublished (following the same pattern we have in nodes).

It was missing the publish & unpublish actions configuration schemas...

The rest of the failing tests are probably because this patch requires #2820848: Make BlockContent entities publishable to be in (I hope).

Another question is whether we need permissions to deal with this? Like "View unpublished content", etc.

re #13: we should imho, but now I'm thinking we should tackle this globally for all "unpublishable entities", possibly as part of #2809177: Introduce entity permission providers or?

We should probably follow what's being done on the added related issue, and use a "Published" checkbox instead.

Postponing until BlockContent entities are publishable. We should focus for now on that.

Also, #2068063: Change "Save and keep un-/published" buttons to a "Published" checkbox and an included "Save" button is now in, so we must use a checkbox for publishing blocks here.

#2820848: Make BlockContent entities publishable is getting really close I think, so lets work on this a bit.

First a straight reroll of #8, manually fixed conflicts on:

• core/modules/block_content/tests/src/Functional/BlockContentListViewsTest.php
• core/modules/block_content/src/BlockContentViewsData.php
• core/modules/block_content/src/BlockContentForm.php

• Switching to the published checkbox as we do now on node forms.
• Using the methods setPublished / setUnpublished on the actions and avoiding saving the blocks if there is no need for it.
• Some other minor cleaning up.

It's working perfectly already, so that's good news =)

It'd be great if we could get #2892304: Introduce footer region to ContentEntityForm in, to accomodate for the published checbox here as well, for now adding another region like we do on node.

Self review:

+++ b/core/modules/block_content/src/Plugin/Action/PublishBlockContent.php
@@ -20,8 +20,10 @@ class PublishBlockContent extends ActionBase {
-    $entity->status = 1;
-    $entity->save();
+     if ($entity->isPublished()) {
+      return;
+    }
+    $entity->setPublished()->save();

Indentation is off here, oops..

Updating issue summary with latest screenshot.

Added screenshot of the block content view showing the bulk operations for publishing/unpublishing and the new column showing the published state.

Here is the combined patch to see what the bot would say.

This was exactly the approach I would propose, following the node method should suffice quite well.

However how do we represent this on the place blocks page, and we should expose it in the contextual sidebar?

Re-roll of #25, before I start trying to fix the issues.

Combined patch includes #148 from #2820848: Make BlockContent entities publishable.

Now to try and fix the test fails.

This should fix the failing tests and the coding standards issues.

Added [PP-1] because this is really blocked / postponed on #2820848: Make BlockContent entities publishable.

1. +++ b/core/modules/block_content/config/install/system.action.block_content_publish_action.yml
   @@ -0,0 +1,10 @@
   +plugin: block_content_publish_action
   
   +++ b/core/modules/block_content/config/install/system.action.block_content_unpublish_action.yml
   @@ -0,0 +1,10 @@
   +plugin: block_content_unpublish_action
   
   +++ b/core/modules/block_content/src/Plugin/Action/PublishBlockContent.php
   @@ -0,0 +1,40 @@
   +class PublishBlockContent extends ActionBase {
   
   +++ b/core/modules/block_content/src/Plugin/Action/UnpublishBlockContent.php
   @@ -0,0 +1,40 @@
   +class UnpublishBlockContent extends ActionBase {
   

   We should have some kind of EntityPublishedActionBase base class for all these action plugins.

2. +++ b/core/modules/block_content/src/BlockContentForm.php
   @@ -33,6 +33,16 @@ public function form(array $form, FormStateInterface $form_state) {
   +    $form['#entity_builders']['update_status'] = '::updateStatus';
   
   @@ -86,4 +96,30 @@ public function save(array $form, FormStateInterface $form_state) {
   +  /**
   +   * Entity builder updating the block content status with the submitted value.
   +   *
   +   * @param string $entity_type_id
   +   *   The entity type identifier.
   +   * @param \Drupal\block_content\BlockContentInterface $block_content
   +   *   The block content updated with the submitted values.
   +   * @param array $form
   +   *   The complete form array.
   +   * @param \Drupal\Core\Form\FormStateInterface $form_state
   +   *   The current state of the form.
   +   *
   +   * @see \Drupal\block_content\BlockContentForm::form()
   +   */
   +  public function updateStatus($entity_type_id, BlockContentInterface $block_content, array $form, FormStateInterface $form_state) {
   +    $element = $form_state->getTriggeringElement();
   +    if (isset($element['#published_status'])) {
   +      if ($element['#published_status'] == TRUE) {
   +        $block_content->setPublished();
   +      }
   +      else {
   +        $block_content->setUnPublished();
   +      }
   +    }
   +  }
   

   We are now displaying the status field in entity form, so this shouldn't be needed anymore?

3. +++ b/core/modules/block_content/src/BlockContentForm.php
   @@ -33,6 +33,16 @@ public function form(array $form, FormStateInterface $form_state) {
   +    $form['footer'] = [
   +      '#type' => 'container',
   +      '#weight' => 99,
   +      '#attributes' => [
   +        'class' => ['block-content-form-footer']
   +      ]
   +    ];
   +    $form['status']['#group'] = 'footer';
   

   I'd vote for postponing this issue on #2892304: Introduce footer region to ContentEntityForm as well, no need to introduce code that will be removed shortly after.

4. +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -193,6 +203,17 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    $fields['revision_created'] = BaseFieldDefinition::create('created')
   +      ->setLabel(t('Revision create time'))
   +      ->setDescription(t('The time that the current revision was created.'))
   +      ->setRevisionable(TRUE);
   +
   +    $fields['revision_user'] = BaseFieldDefinition::create('entity_reference')
   +      ->setLabel(t('Revision user'))
   +      ->setDescription(t('The user ID of the author of the current revision.'))
   +      ->setSetting('target_type', 'user')
   +      ->setRevisionable(TRUE);
   

   #2820848: Make BlockContent entities publishable is removing these definitions because they are already provided by EditorialContentEntityBase, why are we adding them back?

#32.1 - Added #2907075: Add EntityPublishedActionBase as a base class for all publishing status actions with we can do as a follow up / soft dependency.
#32.2 - Removed.
#32.3 - I'd kinda hope #2892304: Introduce footer region to ContentEntityForm is committed before this issue, so sounds fair.
#32.4 - Not sure, removed.

Just adding a combined patch for now.

The dependency needed a reroll and #2892304: Introduce footer region to ContentEntityForm was committed so we can fix #32.3 now :)

Brilliant, thanks all for pushing this forward!

I think what we've got looks great, only thing I think we should work on (though it could perhaps be a followup), is what Bojhan mentioned on #28:

However how do we represent this on the place blocks page, and we should expose it in the contextual sidebar?

Some thoughts on this:

1. It would make sense to show the published status of blocks on this page. At least seeing which blocks are unpublished should be clear to the user.
2. Allowing users to place unpublished blocks makes sense in case you want to see the block before publishing it, so I would not restrict that.
3. Adding the publish/unpublish to the operations dropdown might be useful.

Options I see:

• Add a published column to that table (might be too much?)
• Show (unpublished) next to the block name, so it looks like Site branding (unpublished) (might be too little?).

Blocker is in

Awesome thanks @larowlan!
Queued a test for #34 let's see where we stand =)

#2820848: Make BlockContent entities publishable is in.

It doesn't need a reroll, there's a patch in #34 that applies to 8.5.x :) That's why I'm posting two patches every time, with and without the dependent issue.

+++ b/core/modules/block_content/src/Plugin/Action/PublishBlockContent.php
@@ -0,0 +1,40 @@
+class PublishBlockContent extends ActionBase {

+++ b/core/modules/block_content/src/Plugin/Action/UnpublishBlockContent.php
@@ -0,0 +1,40 @@
+class UnpublishBlockContent extends ActionBase {

Let's use \Drupal\Core\Field\FieldUpdateActionBase just like \Drupal\node\Plugin\Action\PublishNode and \Drupal\node\Plugin\Action\UnpublishNode

@amateescu++
@Wim Leers--

Sorry!

+++ b/core/modules/block_content/src/Plugin/Action/UnpublishBlockContent.php
@@ -0,0 +1,40 @@
+ * Unpublishes a node.

s/node/block content/

This is the only thing I'm able to find, and it's a silly nit!

Yeah latest patch is ok and already passing (was up to date with #2820848: Make BlockContent entities publishable) =)

Addressing #40 and #42.

Looks like we're good to go.

--- /dev/null
+++ b/core/modules/block_content/config/install/system.action.block_content_publish_action.yml

--- /dev/null
+++ b/core/modules/block_content/config/install/system.action.block_content_unpublish_action.yml

--- a/core/modules/block_content/config/optional/views.view.block_content.yml
+++ b/core/modules/block_content/config/optional/views.view.block_content.yml

We are adding two actions and updating an existing view. Do we need an update path to add the actions and update the existing view?

Well spotted, @jibran!

As far as I know, we still don't have a way to update configuration provided by a module (#2684081: Modules need a way to keep their configuration up to date) so I'm not sure there's anything we can do about the existing view.

I think #2898344: Add filters to the comments administration pages is relevant here, see comments #7.2, #9 #10, #11 and #13 in that issue for further details.

TL;DR we didn't add an upgrade path for that view.

But let me quote #10

Let's assume this won't make it to 8.4.x, but rather just in 8.5. IN that case we could use the config hash stored in _core to determine whether things got changed.

and #11

Just for the reference core default_config_hash is:

_core:
  default_config_hash: CTRys5yrLceyvD5kd4rr-XLUceQHt-9x9C3Hnn8K5Rg

#2560049: Incorrect capitalisation of translatable strings, #2600576: MIME, not Mime / mime and #2712647: Update Symfony components to ~3.2 all made changes to the file admin default view without updating the existing view in storage. We can totally drop upgrade path for it if it won't make it to 8.4.x.

This is not going to be in 8.4.x this is just going to be in 8.5.x so by the same logic we don't need an upgrade path for the view.

I asked the upgrade path question in #45 because we are adding two new actions to the existing view if we would have been importing the new view like comment_post_update_enable_comment_admin_view then we would have to import the actions as well but is it true here.
Let's discuss the two cases:

• On a fresh 8.5.0 install, this won't be a problem because when block_content is installed everything from config/install would be installed and when the views module is installed we'll install the new updated view from config/optional.
• After the upgrade from 8.4.x to 8.5.0, the view in storage will be updated (I think, not sure about this. We have to find out.) but action instances will not be created and we'll end up with broken view if the view will be updated so we need an upgrade path to import the actions maybe?

1. +++ b/core/modules/block_content/src/BlockContentViewsData.php
   @@ -30,6 +30,15 @@ public function getViewsData() {
   +    $data['block_content']['block_content_bulk_form'] = [
   +      'title' => $this->t('Block content operations bulk form'),
   +      'help' => $this->t('Add a form element that lets you run operations on multiple block content.'),
   +      'field' => [
   +        'id' => 'block_content_bulk_form',
   +      ],
   +    ];
   

   FYI, we need a cache clear post-update hook for this change only.

2. +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -165,6 +165,16 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    $fields['status']
   +      ->setDisplayOptions('form', [
   +        'type' => 'boolean_checkbox',
   +        'settings' => [
   +          'display_label' => TRUE,
   +        ],
   +        'weight' => 120,
   +      ])
   +      ->setDisplayConfigurable('form', TRUE);
   

   This change will add an update to EUDM but we only added the status field in #2820848: Make BlockContent entities publishable so we don't need update hook for this.

PS: I think we shouldn't do views and action changes here because this is about updating the UI. We can update the admin view in followup and add actions there because the above discussion seems out of scope here.

I took a look to see where we stand when it comes to the block_content view moving from 8.4 to 8.5.

Steps taken:

1. git checkout 8.4.x
2. drush si -y
3. git checkout 8.5.x
4. git apply --index 2834546-43.patch
5. drush updb -y

Result:
The block_content view does not show the published column nor the bulk actions to publish/unpublish custom blocks.

So clearly the view changes require an upgrade path. I vote for splitting those changes into a follow up, unless someone is willing to do this here...

Here is a patch with just the UI changes related to the block content form (no actions or view changes).

I will create the follow up with the view and actions patch shortly.

Well, before I run and create a follow up, let's hear what others have to say about it, because we could/should also be making changes to the block layout screen.. (see #35), so we may want to split that into another followup perhaps.

In any case, here is the starting point adding what I removed on #50.

I think its the right idea to provide an update path:

• It does not change visitor's behaviour
• It provides a new feature for editors, without impacting existing functionality

The update path should update individual bits of the view, aka. set the additional field and add the filters instead of replacing the entire view.
The update path test should then test some basic behaviour of the view.

+++ b/core/modules/block_content/src/BlockContentViewsData.php
@@ -30,6 +30,15 @@ public function getViewsData() {
+
+    $data['block_content']['block_content_bulk_form'] = [
+      'title' => $this->t('Block content operations bulk form'),
+      'help' => $this->t('Add a form element that lets you run operations on multiple block content.'),
+      'field' => [
+        'id' => 'block_content_bulk_form',
+      ],
+    ];

+++ b/core/modules/block_content/src/Plugin/views/field/BlockContentBulkForm.php
@@ -0,0 +1,21 @@
+<?php
+
+namespace Drupal\block_content\Plugin\views\field;
+
+use Drupal\system\Plugin\views\field\BulkForm;
+
+/**
+ * Defines a block content operations bulk form element.
+ *
+ * @ViewsField("block_content_bulk_form")
+ */
+class BlockContentBulkForm extends BulkForm {
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function emptySelectedMessage() {
+    return $this->t('No block content selected.');
+  }
+
+}

I just added a follow up issue which would have avoided all this additional code: #2909423: Get rid of BulkForm subclasses when they just override emptySelectedMessage

I think its the right idea to provide an update path:

• It does not change visitor's behaviour
• It provides a new feature for editors, without impacting existing functionality

The update path should update individual bits of the view, aka. set the additional field and add the filters instead of replacing the entire view.
The update path test should then test some basic behaviour of the view.

Let's create a follow-up with whole views related changes then so that we don't block this feature.

Alright then, I've opened up the follow up for the view / actions part:
#2909435: Update block library page to adapt publishable block content implementation

Not 100% sure of the issue description there, please review =)

Now the patch #50 is simple enough, since we've split off the changes to views.

Here is test to check for the new field (and test-only patch is the diff file).

Thanks @vijaycs85 for the test!
Just cleaning up unrelated changes to the test file on this patch. Anything else we should be doing here?

Fixing failing tests...

Should we postpone this on #2909435: Update block library page to adapt publishable block content implementation?
And maybe on #2809177: Introduce entity permission providers? Do we need any tweaks to \Drupal\block_content\BlockContentAccessControlHandler? Should users with the "administer blocks" permission be able to view unpublished blocks?

Re #65:

As far as postponing on #2909435: Update block library page to adapt publishable block content implementation - We purposely split that into its own issue so as to not to block this one (see #53, #54).

As far as postponing on #2809177: Introduce entity permission providers - If I understand correctly, that adds a view own unpublished $entity_type, but nothing related to publishing/unpublishing entities, so I'm not sure how it relates to this issue?

Users with 'administer blocks' permission are already able to view unpublished blocks, and I think this is to be expected. So no need to change BlockContentAccessControlHandler in my opinion.

One small item, otherwise looks good:

+++ b/core/modules/block_content/tests/src/Functional/BlockContentCreationTest.php
@@ -51,6 +51,14 @@ public function testBlockContentCreation() {
+    $this->assertSession()->statusCodeEquals(200);
...
+    $this->assertSession()->fieldExists('edit-status-value');
+    $this->assertSession()->checkboxChecked('edit-status-value');

We don't need to load WebAssert three times, we can use load it once into a variable and use it again.

Good point, addressing #67 here.

Thanks, looks good

Fixing failing test.

Back to RTBC then.

Does this need an update for the entity form display, or has that been avoided intentionally? I see discussion about whether or not to update the view here, but not the form display.

I don't think there's any reason not to update the form display, good point :) We just need to copy and adapt the code we used for updating form displays for nodes: node_post_update_configure_status_field_widget().

Thanks both, I think that makes sense, hopefully I got it right, adding the update and accompanying test.

Oops.

Thanks @Manuel Garcia

While in practice form displays should be quite finite, in general we should be batching config updates, since we've had problems with some going out of memory. I think we should do that here even though it's unlikely to be a problem, since it also helps if someone copies and pastes the update to implement their own in a situation that would be a problem.

I've just committed #2949351: Add a helper class to make updating configuration simple which makes that a lot easier to implement.

Good idea @catch

Let's see if I got it right :)

Actually the field placement is simpler nowadays, so adjusting this to be like how we're doing it on other editorial content entities.

Updated issue summary with what we are actually proposing in the patch.

+++ b/core/modules/block_content/src/BlockContentForm.php
@@ -36,8 +36,6 @@ public function form(array $form, FormStateInterface $form_state) {
-    $form['status']['#group'] = 'footer';

Are you sure about this? NodeForm still does it..

You're right, its been a while since i looked at this issue... re-uploading #80 for clarity.

Great!

+++ b/core/modules/block_content/tests/src/Functional/BlockContentCreationTest.php
@@ -44,12 +44,21 @@ protected function setUp() {
     $edit['info[0][value]'] = 'Test Block';
     $edit['body[0][value]'] = $this->randomMachineName(16);
+
+    $this->drupalGet('block/add/basic');
+    $assert->statusCodeEquals(200);
+
+    // Make sure status field exist.
+    $assert->fieldExists('edit-status-value');
+    $assert->checkboxChecked('edit-status-value');
+

That's not that much test coverage on the implications of this. While that technically doesn't change by making it visible, we AFAIK pushed back quite a few questions on this issue when adding the status originally, like:

* No way to see if a block is published or not in the overview nor being able to filter on it
* Weren't 100% sure on whether an admin should see a block with an unpublished block_content entity (right now they can)
* If they can (I guess that makes sense, even though a bit confusing), there is no class nor any resulting visual indication that this element is visible only for users with the administer blocks permission. Nodes have the node--unpublished and the very pretty background color (which I just researched to use a name, but turns out it's not an exact match for anything but close to "Snow"...)
* Translations is also interesting, did we check that the status is then not duplicated? \Drupal\content_translation\ContentTranslationHandler::entityFormAlter() defines a translation status field, which for example \Drupal\node\NodeTranslationHandler::entityFormAlter() hides again. (with one wrong key, see #2971390: Make exposure of translation meta fields conditional, considering to generalize that logic because we actually can do that now)

Are we really sure that we want to expose this functionality to users without having that resolved?

I guess one big advantage would be that users where block_content_8400() failed would have a way to fix their content in the UI, but it will only by in 8.6 anyway ;)

No way to see if a block is published or not in the overview nor being able to filter on it

We have an issue for this already: #2909435: Update block library page to adapt publishable block content implementation

So we will need two more issues to handle the visual indication for unpublished blocks throughout the site and another one for the content translation form alter :)

+++ b/core/modules/block_content/tests/src/Functional/Update/BlockContentUpdateTest.php
@@ -66,4 +67,26 @@ public function testStatusFieldAddition() {
+    // Run updates.
+    $this->runUpdates();
+
+    $query = \Drupal::entityQuery('entity_form_display')
+      ->condition('targetEntityType', 'block_content');
+    $ids = $query->execute();

Can we test the state before the update too?

Just in case someone updates the dump file, which would render this test invalid.

We typically do that

Good idea @larowlan, so... something like this would do I suppose?

OK now I'm confused... how can it be that the status form component is there before the updates are run? 0_o

Because base field default form/view display configuration is merged in from the field definitions if nothing is specified yet.

Second, you are anyway not allowed to use the config entity API before running updates, you must use config API directly. Doing that will also fix the first problem as that config-entity level logic will not run.

Thanks @Berdir!

+++ b/core/modules/block_content/block_content.post_update.php
@@ -88,3 +89,22 @@ function block_content_post_update_add_views_reusable_filter(&$sandbox = NULL) {
+    $entity_form_display->setComponent('status', [

We need to bring this in line with what we're doing in #2899923: UI for publishing/unpublishing taxonomy terms and don't update the 'status' component if it already exists on the form display.

Addressing #100 :)

Nice! We still need to work on the followups from #87 / #88, but this is a good small step.

This looks ready to go in my book, however I think we need a change record *and* I think @Berdir's concerns in #87/#88 would be semi-addressed if we timed this to go in at the same time as #2909435: Update block library page to adapt publishable block content implementation, which still needs work to address committer feedback on the update path.

Let's get the change record ready to go, and then see how we go on the other one. It would be good to coordinate a commit of this with a subsequent commit of #2909435: Update block library page to adapt publishable block content implementation

Thanks for your work here, this is a big feature.

Thanks @larowlan - I have created a draft change record, combing the two issues into it, let me know what you think :)
https://www.drupal.org/node/3001390

Tested manually on both scenarios (install with patch & install, apply patch & drush updb) and can see the publish checkbox on both cases. +1 to RTBC.

The change record seems to have a screenshot of admin/content and not of the new block content library with publishing status included. I have to agree with @Berdir / #87 - I'm struggling to see how we can be doing this and #2909435: Update block library page to adapt publishable block content implementation in separate issues. Leaving at RTBC for other committers to decide but the CR does look like it needs attention. And if we are going to do them separately then I think we need #2909435: Update block library page to adapt publishable block content implementation close to rtbc to get this in.

Yes I agree with alexpott, we should combine this issue with #2909435: Update block library page to adapt publishable block content implementation and commit the change together to keep the block UI as a whole consistent.

Thanks for the great work here. I just encountered, that enabling content translation on a block type and making the "Published" field translatable causes the "Published" field to not save changes anymore, not in the host language, not in the translated ones.

I'm not sure if that is part of the UI issue here, or a bug in the underlying non-ui implementation.

Referring to #108, here is a short screencast. Actually the "Published" checkbox is completely effectless on the translated block_content form and the "This translation is published" is the only one working. It seems both have the seem meaning and have side effects on each other.

Actually I found, that we have the same issue as reported in #108 and #109 also with #2899923-76: UI for publishing/unpublishing taxonomy terms.

I fixed it by using the same approach as in the other issue (thanks for @ManuelGarcia). There might be a better solution, because we're doing the exact same thing then for terms and block content.

Rerolled, manually fixed conflicts on:

• core/profiles/demo_umami/config/install/core.entity_form_display.block_content.banner_block.default.yml
• core/profiles/demo_umami/config/install/core.entity_form_display.block_content.basic.default.yml
• core/profiles/demo_umami/config/install/core.entity_form_display.block_content.disclaimer_block.default.yml
• core/profiles/demo_umami/config/install/core.entity_form_display.block_content.footer_promo_block.default.yml

Also trying to fix the tests, interdiff is after the reroll.

Still needs work because it's been requested that we merge #2909435: Update block library page to adapt publishable block content implementation into this one. FWIW, we had previously split that into its own issue so as to not to block this one (see #53, #54).

Looks like the interdiff on #110 was incomplete, reverting the changes to changes to core.entity_form_display.block_content.basic.default.yml added in that patch, and adding the fixture yml file back in, hopefully getting this back to green.

A heads up on the patch, not sure why git diff thinks I'm doing this:

copy from core/profiles/demo_umami/config/install/core.entity_form_display.block_content.basic.default.yml
copy to core/modules/block_content/tests/fixtures/update/core.entity_form_display.block_content.basic.default_2834546.yml

I copied core/profiles/standard/config/install/core.entity_form_display.block_content.basic.default.yml over and modified it to create the new file for the fixture...

I can't figure out why this test is failing:

There was 1 error:

1) Drupal\Tests\demo_umami\Functional\DemoUmamiProfileTest::testConfig
Exception: core.entity_form_display.block_content.banner_block.default: Drupal\Component\Diff\Engine\DiffOpDelete::__set_state(array(
   'type' => 'delete',
   'orig' => 
  array (
    0 => '  translation:',
    1 => '    weight: 10',
    2 => '    settings: {  }',
    3 => '    third_party_settings: {  }',
    4 => '    region: content',
  ),
   'closing' => false,
))

/var/www/html/core/tests/Drupal/KernelTests/AssertConfigTrait.php:38
/var/www/html/core/profiles/demo_umami/tests/src/Functional/DemoUmamiProfileTest.php:77
/var/www/html/core/profiles/demo_umami/tests/src/Functional/DemoUmamiProfileTest.php:54

Anyone has an idea why?

For now just reverting another unwanted change introduced in #110.

OK so I think I finally figured it out... the installed configuration is actually the same as the default, but the order of the items in the file is different, which results in test failure.

I found out by installing demo_umami profile, exporting the configuration, and doing a diff between the default file and the exported file.

The latest feedback from #107 was that we should combine this issue with #2909435: Update block library page to adapt publishable block content implementation. That's not going to be easy since both of them have 100 comments already, but I guess it can be done :)

@Manuel Garcia, what do you think?

Guess we should merge these two back into one, let's focus on getting a green patch again on #2909435: Update block library page to adapt publishable block content implementation and merge it into here then.

Is there any interest in resurrecting this? If so, I can test the last patch (and the patch in https://www.drupal.org/project/drupal/issues/2909435).

Rerolled the patch from #118 for 9.3.x.

Re-rolling the patch in #118 for 9.2.x

Starting at #126

rerolled for 9.5 with a few phpcs fixes.

This is going to need a heavy code review.

I removed translation and language fields from the core.entity_form in the fixtures folder but kept them in the demo_umami folder because it didn't seem to make sense to remove them for the status field.

Updated one of the failing tests and fixed a deprecation call.

With regards to combining with https://www.drupal.org/project/drupal/issues/2909435 I would vote to keep them separate. That other ticket is far from ready and this one seems closer at least. Easier to get things in smaller chunks then a huge one. Hate to see this feature delayed while we try to fix the other.

Looking great, a couple of observations

1. +++ b/core/modules/block_content/src/BlockContentForm.php
   @@ -36,6 +36,8 @@ public function form(array $form, FormStateInterface $form_state) {
   +    $form['status']['#group'] = 'footer';
   +
   

   for super-safety we should probably check $form['status'] exists first here.

   there will be the odd site that didn't run the update hook properly, or export their config properly and this will cause some oddities in their form/possibly raise warnings

2. +++ b/core/modules/block_content/src/BlockContentTranslationHandler.php
   @@ -19,4 +20,23 @@ protected function entityFormTitle(EntityInterface $entity) {
   +  public function entityFormAlter(array &$form, FormStateInterface $form_state, EntityInterface $entity) {
   +    parent::entityFormAlter($form, $form_state, $entity);
   +    $form['content_translation']['status']['#access'] = !isset($form['content_translation']);
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function entityFormEntityBuild($entity_type, EntityInterface $entity, array $form, FormStateInterface $form_state) {
   +    if ($form_state->hasValue('content_translation')) {
   +      $translation = &$form_state->getValue('content_translation');
   +      $translation['status'] = $entity->isPublished();
   +    }
   +    parent::entityFormEntityBuild($entity_type, $entity, $form, $form_state);
   

   I have no idea about this, so will need to do some research into what this does

3. +++ b/core/modules/block_content/tests/fixtures/update/core.entity_form_display.block_content.basic.default_2834546.yml
   --- /dev/null
   +++ b/core/modules/block_content/tests/fixtures/update/drupal-8.entity_form_display_block_content-2834546.php
   
   +++ b/core/modules/block_content/tests/fixtures/update/drupal-8.entity_form_display_block_content-2834546.php
   @@ -0,0 +1,22 @@
   + * Contains database additions to drupal-8.bare.standard.php.gz for testing the
   

   is the 8 in the name accurate here now?

4. +++ b/core/modules/block_content/tests/src/Functional/BlockContentTranslationUITest.php
   @@ -150,4 +150,41 @@ protected function doTestTranslationEdit() {
   +      // (Un)publish the block content translations and check that the translation
   ...
   +        // The block content is created as unpublished thus we switch to the published
   

   nit, > 80 chars in this comment

5. +++ b/core/modules/block_content/tests/src/Functional/BlockContentTranslationUITest.php
   @@ -150,4 +150,41 @@ protected function doTestTranslationEdit() {
   +      $storage->resetCache([$this->entityId]);
   +      $entity = $storage->load($this->entityId);
   

   we can use ->loadUnchanged here and avoid the resetCache call, one less line

6. +++ b/core/modules/block_content/tests/src/Functional/BlockContentTranslationUITest.php
   @@ -150,4 +150,41 @@ protected function doTestTranslationEdit() {
   +        $status = !$index;
   

   not sure about this magic

7. +++ b/core/modules/block_content/tests/src/Functional/Update/BlockContentUpdateTest.php
   @@ -0,0 +1,68 @@
   +      __DIR__ . '/../../../../../system/tests/fixtures/update/drupal-9.0.0.bare.standard.php.gz',
   +      __DIR__ . '/../../../fixtures/update/drupal-8.entity_form_display_block_content-2834546.php',
   

   yeah the 9.0.0 here makes me think we should rename the fixture to be 9 instead of 8 and update the comment

8. +++ b/core/profiles/demo_umami/config/install/core.entity_form_display.block_content.basic.default.yml
   @@ -36,10 +36,16 @@ content:
   -hidden:
   -  moderation_state: true
   +hidden: {  }
   

   is this a regression?

9. +++ b/core/profiles/demo_umami/config/install/core.entity_form_display.block_content.disclaimer_block.default.yml
   @@ -43,6 +43,13 @@ content:
   +    weight: 120
   
   +++ b/core/profiles/demo_umami/config/install/core.entity_form_display.block_content.footer_promo_block.default.yml
   @@ -61,6 +61,13 @@ content:
   +    weight: 120
   
   +++ b/core/profiles/standard/config/install/core.entity_form_display.block_content.basic.default.yml
   @@ -29,4 +29,11 @@ content:
   +    weight: 120
   

   120 feels high, are these shown below or above the submit button.

   I know there's a bug in core that puts all new fields on taxonomy form displays under the button 🤦

#133
1. Added an if statement to check
2. Not too sure either. From the original patch
3. Updated to 9
4. Fixed
5. Fixed in 2 spots for that function
6. Wasn't sure either. Is it checking that the status is FALSE? we could just remove and use assertFalse
7. Fixed
8. Possibly? Not sure why it would need to be removed. So added back
9. It appears above the save button

bumping

I re-read all the comments here, and in #106 and #107 we have two committers asking for this to be combined with #2909435: Update block library page to adapt publishable block content implementation

In addition in #87 Berdir had a number of questions about test coverage and the implication for translations which I don't think have been addressed yet.

It also looks like #13 is unresolved

I've updated the issue summary with all those items

Moving over credit from #2909435: Update block library page to adapt publishable block content implementation

Combined the changes from #2909435: Update block library page to adapt publishable block content implementation

Still need work for the additional tests and translation coverage.

@berdir would you mind taking a relook about what could be needed please? Know things have changed in 5 years

I do think we will need a view unpublished permission we may be able to get this ready but ultimately may be blocked by #2809177: Introduce entity permission providers

Closed https://www.drupal.org/project/drupal/issues/2821174 as a duplicate of this.

Might be worth noting, at the moment it is not possible to create a View of blocks and filter it by 'Enabled'. E.g. if a have a number of Custom Blocks which I have added to my Block Layout and some of them are Enabled and some are Disabled. If I create a View of Custom Blocks and filter them by Published, all the blocks appear in the list as all are considered Published by Views. There is no filter option to filter by Enabled.

Even if I remove a Custom Block from the Block Layout - it will still appear in my Views list.

If this issue is fixed, will it solve this problem too? Or is that a separate problem that I need to create a new issue?

Hi @frankdesign I think you're conflating two different concepts there

We have Block placements - these are config entities that you see at Admin > Structure > Block. These are configured block plugins that include the block-type, the region it is placed in and the theme. They can be blocks made from content (more on that below) but they can also be system blocks like menus, views etc

We also have Block content entities, these are what you see at Admin > Structure > Block > Block Library on 10.0 and below and Admin > Content > Blocks for 10.1 and above.

Block placements can be disabled and enabled

Block content entities can be published and unpublished (and even have drafts).

This is about adding a UI for publishing and unpublishing the block content entities.

They are two different concepts and unfortunately views has no visibility of the block placements which is where the enabled/disabled state comes from. It can only track the content entities.

1. Add additional test coverage for the UI implications of what happens if a block content entity is unpublished (see #87)
TODO - dependent on answer to 2-4

2. Confirm how this impacts translations (see #87)
Not sure how to verify this. But translation appears to be working

3. Confirm what occurs if a block config entity (placement) references an unpublished block
Updated getEntity() to check status before returning. So unpublished content won't appear.

4. Consider if we need to add a 'view unpublished' permission per #13
Not sure the use case.

Updated issue summary.

With the changes to the tests I wonder if that covers the change of the block not appearing when status is unpublished.

Putting to review for general feedback.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Moved logic to blockAccess ready for review again.