Update block library page to adapt publishable block content implementation

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

Let's add delete action as well.

Probably we have to make changes to reflect the changes in IS (just added) as part of this ticket.

Please don't implement your own action plugins. Better wait and push #2916740: Add generic entity actions and #2670730: Provide a delete action for each content entity type

Unpostponed since #2916740: Add generic entity actions landed

Nice one 👏

#2670730: Provide a delete action for each content entity type is really close, so should a delete action implemented here as well?

+++ b/core/modules/block_content/config/schema/block_content.schema.yml
@@ -16,3 +16,11 @@ block_content.type.*:
+
+action.configuration.block_content_publish_action:
+  type: action_configuration_default
+  label: 'Publish selected block content configuration'
+
+action.configuration.block_content_unpublish_action:
+  type: action_configuration_default
+  label: 'Unpublish selected block content configuration'

Could also be deleted

Delete actions are in.

I have added the bulk actions to the install process and move bulk selections fields to the top.

I also considered now that #2670730: Provide a delete action for each content entity type is in, whether we should also add a "Delete" action to the Custom block library. Theoretically it could be considered out of scope, but since this already contains an upgrade path to enable the bulk form, this would just be another action we would have to add as part of the upgrade path. Thoughts?

Oops, didn't mean to change the status. Also removing "Needs update path" tag, as that seems to be part of the patch by now.

1. +++ b/core/modules/block_content/block_content.install
   @@ -138,3 +138,236 @@ function block_content_update_8400() {
   +function block_content_update_8600() {
   

   I think this should be done in a post_update hook where we are able to use the full API

2. +++ b/core/modules/block_content/block_content.install
   @@ -138,3 +138,236 @@ function block_content_update_8400() {
   +  $publish_action = \Drupal::service('config.factory')->getEditable('system.action.block_content_publish_action');
   +  $config_data_publish = [
   +    "langcode" => "en",
   +    "status" => true,
   +    "dependencies" => [
   +      "module" => [
   +          "block_content"
   +        ]
   +    ],
   +    "id" => "block_content_publish_action",
   +    "label" => "Publish block content",
   +    "type" => "block_content",
   +    "plugin" => "entity:publish_action:block_content",
   +    "configuration" => []
   +  ];
   +  $publish_action->setData($config_data_publish);
   +  $publish_action->save();
   

   Not sure about writing the complete config object here again. May we could do something like this:

   $value = $this->extensionConfigStorage->read($full_name);
   
   $entity_storage = $this->entityManager->getStorage($type);
   $entity = $entity_storage->createFromStorageRecord($value);
   $entity->save();
   

   This code block is copied from config_update module.

Re #25: Generally we try to avoid reading runtime information during updates, because you don't know when they will be run. I.e. some system may not run the update until months/years later when the runtime information (in this case the block library view) has changed again. That is definitely the case for normal (non-post-)update functions. I can see how it is less relevant for post-updates, but I haven't seen something like #25 in core before. Maybe someone more into the update system (@alexpott ?!) can clarify.

In general though I think you are absolutely correct that this makes sense to do in a post-update.

Tss... somehow my browser reeallly thinks this issue needs review... sorry for the noise.

Loading, updating and then saving views in a post update function has proven to be problematic (see #2917006: Views referencing missing roles fail views_post_update_revision_metadata_fields(), which is being caused by the upgrade path added in #2248983: Define the revision metadata base fields in the entity annotation in order for the storage to create them only in the revision table) because some views might be in a broken state and as a consequence they can not be saved via the (config) entity API.

So the patch from #23 is correct on both points from #25:
- it updates the views config files directly using a regular update function
- it doesn't use runtime information for generating the action config entities

Looks great & no reason in upgrade tests

The patch looks great to me! I just found a few nitpicks :)

1. +++ b/core/modules/block_content/block_content.install
   @@ -138,3 +138,236 @@ function block_content_update_8400() {
   +      'exclude' => false,
   ...
   +        'word_boundary' => true,
   

   We need to convert all the lowercase true and false to uppercase :)

2. +++ b/core/modules/block_content/block_content.install
   @@ -138,3 +138,236 @@ function block_content_update_8400() {
   +      'id' => 'status',
   ...
   +    'id' => 'status',
   ...
   +  $view->set('display.default.display_options.filters.status', $status_filter);
   

   Let's try to avoid a conflict with a possible pre-existing status field or filter, and use ViewExecutable::generateHandlerId() to generate the views handler ID. You can see an example of that in the interdiff from #2880149-115: Convert taxonomy terms to be revisionable.

3. +++ b/core/modules/block_content/tests/src/Functional/Update/BlockContentUpdateTest.php
   @@ -66,4 +67,75 @@ public function testStatusFieldAddition() {
   +    $edit =[];
   ...
   +    $edit =[];
   ...
   +    $edit =[];
   ...
   +    $edit =[];
   

   Coding standards nit, we need a space before [].

4. +++ b/core/modules/block_content/block_content.install
   @@ -138,3 +138,236 @@ function block_content_update_8400() {
   +function block_content_update_8600() {
   +
   +  $view = \Drupal::service('config.factory')->getEditable('views.view.block_content');
   

   No need for this blank line.

5. +++ b/core/modules/block_content/tests/src/Functional/Update/BlockContentUpdateTest.php
   @@ -66,4 +67,75 @@ public function testStatusFieldAddition() {
   +    $user =  $this->drupalCreateUser(['administer blocks', 'administer block_content display']);
   

   Extra space here after =.

6. +++ b/core/modules/block_content/tests/src/Functional/Update/BlockContentUpdateTest.php
   @@ -66,4 +67,75 @@ public function testStatusFieldAddition() {
   +    $this->drupalPostForm('block/add/basic', $edit, t('Save'));
   ...
   +    $this->drupalPostForm('admin/structure/block/block-content', $edit, t('Apply to selected items'));
   ...
   +    $this->drupalPostForm('admin/structure/block/block-content', $edit, t('Apply'));
   ...
   +    $this->drupalPostForm('admin/structure/block/block-content', $edit, t('Apply'));
   

   We don't need to use t() in tests :)

Nice! Just a few things left:

1. +++ b/core/modules/block_content/block_content.install
   @@ -138,3 +139,238 @@ function block_content_update_8400() {
   +  $definition_update_manager = \Drupal::entityDefinitionUpdateManager();
   +  $entity_type = $definition_update_manager->getEntityType('block_content');
   

   Since you are not using $definition_update_manager again in this function, these two lines can be merged into one :)

2. +++ b/core/modules/block_content/block_content.install
   @@ -138,3 +139,238 @@ function block_content_update_8400() {
   +      'id' => ViewExecutable::generateHandlerId($published_key, $view->get("display.default.display_options.fields")),
   

   You can use the $fields variable defined above instead of getting it again from the config object.

3. +++ b/core/modules/block_content/block_content.install
   @@ -138,3 +139,238 @@ function block_content_update_8400() {
   +      'entity_field' => 'status',
   ...
   +    'entity_field' => 'status',
   

   You can use $published_key here as well.

I don't have anything else to complain about :)

Overall this is looking great, however it seems we are lacking explicit test coverage for the bulk operations we are adding. Actually, we have it in the update test but not in BlockContentListViewsTest. Maybe we could add a trait to perform the same assertions in both places?

I think deferring bulk deletion to a follow-up is a good idea, we can modify the logic in block_content_update_8600() to cover also that, as long as this happens before 8.6.0 is released.

Aside from that, here's a code review, mostly nitpicks:

1. +++ b/core/modules/block_content/block_content.install
   @@ -138,3 +139,236 @@ function block_content_update_8400() {
   +  $fields_to_set = array_merge(array_slice($combined_fields, 0, $position), $status, array_slice($combined_fields, $position));
   

   Can't we just use array_splice width length 0 here?

2. +++ b/core/modules/block_content/block_content.install
   @@ -138,3 +139,236 @@ function block_content_update_8400() {
   +      "module" => [
   +          "block_content"
   +        ]
   

   Wrong indentation.

   There are also quite a few missing trailing commmas. It would be nice to fix them for consistency :)

3. +++ b/core/modules/block_content/tests/src/Functional/Update/BlockContentUpdateTest.php
   @@ -66,4 +67,75 @@ public function testStatusFieldAddition() {
   +    $this->assertText('Yes', "Block displayed as published");
   

   Not a huge fan of raw-asserting yes/no this way: it feels fragile. Can we use xpath with some more specific selector instead?

   Also, we're mixing quote types here :)

Manual testing

Existing site

1. Installed 8.6.x HEAD without patch
2. Applied patch (from #50)
3. Ran drush updb(screenshot )
4. Created two blocks and placed them in sidebar first
5. Bulk publish/unpublish works as expected(both published, both unpublished and one published via bulk operation)

Fresh install with patch

1. Installed 8.6.x HEAD with patch
2. Followed step 4 and 5 as per existing site instructions above and produced same results.

Other observations

1. Revision tab looks bit weird when checkbox is not ticked. screenshot
2. Labels and fields have mismatch with content list page. screenshot

We might not able to address them in this issue, just adding here to discuss.

Code review

1. +++ b/core/modules/block_content/tests/src/Functional/BlockContentTestBulkOperationsTrait.php
   @@ -0,0 +1,45 @@
   +trait BlockContentTestBulkOperationsTrait {
   

   nice!

2. +++ b/core/modules/block_content/tests/src/Functional/BlockContentTestBulkOperationsTrait.php
   @@ -0,0 +1,45 @@
   +  protected function assertUnpublishUsingBulkAction() {
   ...
   +  protected function assertPublishUsingBulkAction() {
   

   Minor: these assert methods are doing bit more than just asserting. they are doing certain action and asserting.

3. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6Test.php
   @@ -78,7 +78,7 @@ protected function getEntityCounts() {
   -      'action' => 23,
   +      'action' => 25,
   
   +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7Test.php
   @@ -78,7 +78,7 @@ protected function getEntityCounts() {
   -      'action' => 17,
   +      'action' => 19,
   

   Not sure, if they are automated update. if not, feels irrelevant to this issue.

@Manuel Garcia #54 looks good to me. +1 to RTBC.

+++ 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.');
+  }
+
+}

Do we really need that? We didn't add one for media recently as well. And with #2909423: Get rid of BulkForm subclasses when they just override emptySelectedMessage we would get a nicer message and would have to deprecate this class again.

#56 +1

1. +++ b/core/modules/block_content/block_content.install
   @@ -138,3 +139,236 @@ function block_content_update_8400() {
   +  $view->save();
   ...
   +  $publish_action->save();
   ...
   +  $unpublish_action->save();
   

   We should pass TRUE here to signal the fact that we're dealing with trusted data.

2. +++ b/core/modules/block_content/tests/src/Functional/BlockContentListViewsTest.php
   @@ -10,6 +10,7 @@
    class BlockContentListViewsTest extends BlockContentTestBase {
   +  use BlockContentTestBulkOperationsTrait;
   

   Could use an empty line :)

3. +++ b/core/modules/block_content/tests/src/Functional/BlockContentListViewsTest.php
   @@ -67,13 +68,27 @@ public function testListing() {
   +    $this->assertEqual(count($elements), 6, 'Correct number of table row cells found.');
   

   We could use assertCount() here.

4. +++ b/core/modules/block_content/tests/src/Functional/BlockContentTestBulkOperationsTrait.php
   @@ -0,0 +1,45 @@
   + * @package Drupal\Tests\block_content\Functional
   

   I don't we use the @package annotation in core, let's remove it.

5. +++ b/core/modules/block_content/tests/src/Functional/BlockContentTestBulkOperationsTrait.php
   @@ -0,0 +1,45 @@
   +   * Unpublish a block using the unpublish action.
   ...
   +   * Publish a block using the publish action.
   

   Needs the 3rd person form of the verb :)

6. +++ b/core/modules/block_content/tests/src/Functional/Update/BlockContentUpdateTest.php
   @@ -11,6 +13,7 @@
    class BlockContentUpdateTest extends UpdatePathTestBase {
   +  use BlockContentTestBulkOperationsTrait;
   

   Same as above, a new line will help with visibility.

Nice, I can't find anything else to complain about, so RTBC :)

Testbot Snafu.

Looks good since my last review. +1 to RTBC. did a manual test of one of the happy path (i.e. with update db) and it works as expected.

This needed a re-roll after #2886609: Migrate translations for D6 i18n taxonomy 'localized' terms landed.

Re-roll since #2976334: Allow Custom blocks to be set as non-reusable adding access restriction based on where it was used. was committed.

I'm not sure I agree with #29. Yes updates that fix views to work with the current code base should be hook_update_N() but updates that are adding things that you could do via the UI should be post updates. There is never a time that post updates should be working with broken views and if they are then something has gone wrong.

@alexpott, since Views doesn't enforce data integrity for the configuration of its plugins (like the problem discovered in #2917006: Views referencing missing roles fail views_post_update_revision_metadata_fields()), I don't think we can ever rely on being able to do CRUD operations on views' config entities in a post_update function, so the only option left is to update the view config objects "manually" in a regular hook_update_N() function.

+++ b/core/modules/block_content/block_content.install
@@ -138,3 +139,239 @@ function block_content_update_8400() {
+  $view->save(TRUE);
...
+  $publish_action->save(TRUE);
...
+  $unpublish_action->save(TRUE);

I think actions should be saved before the view is being updated, just like we normally do via UI. Am I missing something here?

since Views doesn't enforce data integrity for the configuration of its plugins

- but we need to move to a world where it does.

Agreed, but not in this issue I would hope :)

+++ b/core/modules/block_content/tests/src/Functional/Update/BlockContentUpdateTest.php
@@ -67,4 +71,61 @@ public function testStatusFieldAddition() {
+   * @see block_content_update_8600()

This needs to be updated to 8700 now..

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6Test.php
@@ -82,7 +82,7 @@ protected function getEntityCounts() {
-      'action' => 23,
+      'action' => 25,

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7Test.php
@@ -82,7 +82,7 @@ protected function getEntityCounts() {
-      'action' => 17,
+      'action' => 19,

unrelated?

@alexpott's comments from #72 haven't been addressed yet either

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.

Only difference between this issue and #2834546: UI for publishing/unpublishing block_content blocks is, this one runs update and #2834546: UI for publishing/unpublishing block_content blocks runs post-update.

@larowlan's feedback has been answered, back to RTBC :)

+++ b/core/modules/block_content/block_content.install
@@ -138,3 +138,236 @@ function block_content_update_8400() {
+  $view = \Drupal::service('config.factory')->getEditable('views.view.block_content');

It's possible the view has been deleted or does not exist.

Also this issue poses an interesting question. Why do we have an update path? It is possible the user has customised their view so that doing these updates is unnecessary and will cause duplicate columns. I think

• Maybe we should only updating if the view matches the original configuration - we can use the _core hash key to determine this.
• Consider whether we should be doing the update at all

Also I'm still not completely sold on @amateescu's arguments for this being hook_update_N - yes there are issues with views dependencies and they way some views plugin over store things in config but they are bugs that we should fix when we come across them.

Here is an update to check with the hash (as suggested in #85). Also created #3021158: Add an API to check configuration object status

Retesting the patch in #94 as the failing test (Drupal\Tests\block_content\Functional\Update\BlockContentUpdateTest::testBlockContentPublishableUIUpdate) doesn't exist in HEAD anymore.

#99my bad :)

Drupal\Tests\block_content\Functional\Update\BlockContentUpdateTest::testBlockContentPublishableUIUpdate is failing because the test is trying to use the DB dump from drupal-8.bare.standard.php.gz which has the config views.view.block_content and doesn't match with what we have current config (i.e. changes went in in config but weren't updated in drupal-8.bare.standard.php.gz)

I am attaching the views.view.block_content.yml for reference.

Next step would be sync the config except the changes introduced in this issue to drupal-8.bare.standard.php.gz and run tests again.

P.S: the config file generated by below code:

$str = '[serialized data from config table]';

$config = unserialize($str);

$yml = \Drupal\Core\Serialization\Yaml::encode($config);
print_r($yml);
// Save it as test.php
// then run
// drush scr test.php > views.view.block_content.yml.txt

after further testing found that the config in DB update doesn't match with the one installed on the HEAD and I created #3062291: Config entity changes as part of install which turns out to be a duplicate of #3059545: Improve \Drupal\KernelTests\Config\DefaultConfigTest to install all optional configuration. Probably a good idea to wait for #3059545: Improve \Drupal\KernelTests\Config\DefaultConfigTest to install all optional configuration to land. it is RTBC anyway.

#3059545: Improve \Drupal\KernelTests\Config\DefaultConfigTest to install all optional configuration is in.

Here is an update on the update test. Works locally. I am not sure how it would help for the review as the patch and interdiff with changes on gz file are not readable.

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/2834546).

This will need a reroll for 9.5

This one needs a lot of work still I think

The update hook failed.
The bulk feature for the view I think needs a handler.

But postponing until https://www.drupal.org/project/drupal/issues/2834546 is complete.

Combing with #2834546: UI for publishing/unpublishing block_content blocks

Will move over appropriate credit.