Problem/Motivation

We can convert custom block library page admin/structure/block/block-content to views

Proposed resolution

Add views integration for block content.
Replace custom block library page with view
Add test

Remaining tasks

Discuss and finalize

User interface changes

Custom block library page will be replaced by view.

API changes

API addition (views integration for block content)

Beta phase evaluation

Copied from https://www.drupal.org/node/1823450#beta-evaluation

This is appropriate for 8.0.x.

It is a major task that has impact greater than disruption, so it can proceed in 8.0.x.

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because the functionality of these listings is already in core (with custom code)
Issue priority Major because this Meta is about replacing core listings in many places, across systems, and has community consensus for being important. Not critical because we can release 8.0.0 without completing the conversion to views and do it in a later release. Keeping the custom code (not converting a listing) will not cause severe performance or usability regressions.
Prioritized changes A bit, it maybe would reduce fragility by re-using views code and getting rid of custom listing code.
Disruption It will not be disruptive for core/contributed and custom modules/themes because it will not require a BC break/deprecation/widespread changes since changes are isolated to core listing custom code.
CommentFileSizeAuthor
#81 interdiff.txt2.02 KBkim.pepper
#81 2375589-block-library-views-81.patch23.37 KBkim.pepper
#75 views-custom-block.75.patch23.01 KBlarowlan
#65 views-custom-block.65.patch22.95 KBlarowlan
#59 views-custom-block.59.patch22.93 KBpameeela
#57 views-custom-block.57.patch22.93 KBpameeela
#55 views-custom-block.55.patch22.95 KBlarowlan
#55 interdiff.txt2.48 KBlarowlan
#49 views-custom-block.49.patch22.13 KBlarowlan
#49 interdiff.txt345 byteslarowlan
#46 Screenshot 2015-03-23 08.54.52.png108.97 KBpameeela
#43 convert_custom_block-2375589-43.patch22.13 KBjibran
#43 interdiff.txt1.2 KBjibran
#41 convert_custom_block-2375589-41.patch22.13 KBjibran
#41 interdiff.txt590 bytesjibran
#39 screenshot-d8.dev 2015-03-21 08-22-37.png9.27 KBjibran
#38 convert_custom_block-2375589-38.patch22.14 KBjibran
#38 interdiff.txt2.39 KBjibran
#31 views-custom-block.31.patch21.88 KBlarowlan
#31 interdiff.txt1.26 KBlarowlan
#28 views-custom-block.28.patch21.62 KBlarowlan
#28 interdiff.txt5.48 KBlarowlan
#28 Screenshot 2015-03-17 19.03.33.png54.01 KBlarowlan
#21 interdiff.txt1.24 KBkim.pepper
#21 custom-block-views-2375589-21.patch17.55 KBkim.pepper
#21 comment-view.png31.72 KBkim.pepper
#21 files-view.png29.38 KBkim.pepper
#21 content-view.png32.49 KBkim.pepper
#15 custom-block-list-views-2375589.3.patch17.48 KBlarowlan
#15 interdiff.txt3.44 KBlarowlan
#13 custom-block-list-views-2375589.2.patch15.61 KBlarowlan
#13 interdiff.txt3.23 KBlarowlan
#9 block-type-listing-2375589-9.patch12.38 KBpameeela
#9 Screen Shot 2015-03-07 at 1.45.53 pm.png40.74 KBpameeela
#9 Screen Shot 2015-03-07 at 1.43.59 pm.png41.74 KBpameeela
#9 Screen Shot 2015-03-07 at 1.43.03 pm.png46.81 KBpameeela
#9 Screen Shot 2015-03-07 at 1.40.30 pm.png56.33 KBpameeela
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

As a note, since there is now a base class for entity views data, views integration for block content could be quite simple. It might even be able to just use the base class, which would mean the views integration would be only a matter of adding one line to the entity annotation, saying to use the base class. Or it might be an abstract class, in which case the specific entity views data class could be one line "class Foo extends EntityViewsData" (plus a few lines of namespaces etc and documentation).

larowlan’s picture

There is an existing (really old) issue for views integration but it was postponed on adding revision ui, we should rename that postpone it in favor of this and then only focus on revisions over there

askibinski’s picture

I would like to add that the block library page currently misses a crucial column (block type) which IMO should also be added to the default view. I made a patch for this in #2373283: Missing block type column in custom block library listing.

dawehner’s picture

Added to #1823450: [Meta] Convert core listings to Views

There is one big reason why this might make sense in general, sites have an incredible amount of custom blocks sometimes,
so using a proper view, is justifyable.

jibran’s picture

webchick’s picture

Component: custom_block.module » block_content.module
dasjo’s picture

Having built a customer site on D8, I know that this would allow us to improve UX quite a lot. Currently, the custom block library page doesn't provide any sorting & filtering capabilities. Having it backed by Views would allow for great customisation.

pameeela’s picture

Assigned: Unassigned » pameeela

Working on this at DrupalSouth

pameeela’s picture

Patch attached to make this a view. Also adds filters and columns (and I think this sorts #2373283: Missing block type column in custom block library listing too)

Before (no blocks):

Before (with blocks):

After (no blocks):

After (with blocks):

pameeela’s picture

Status: Active » Needs review
kim.pepper’s picture

Need to remove the old code, and include additional columns ('block type', 'updated') in the test.

kim.pepper’s picture

Talked with @jibran and agreed we need to leave the existing code and provide a new test for the view.

larowlan’s picture

start on tests

Status: Needs review » Needs work

The last submitted patch, 13: custom-block-list-views-2375589.2.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
3.44 KB
17.48 KB

finalized tests

jibran’s picture

Issue tags: +Needs usability review

I think this is ready. We just need a usability review. Screenshots are in #9.

Wim Leers’s picture

Usability reviewers: look at /admin/structure/block/block-content.

Looking at the code, this is the only nit I could find:

+++ b/core/modules/block_content/src/Tests/BlockContentListTest.php
@@ -12,8 +12,11 @@
+ * Tests the fallback block content list when views is disabled.

+++ b/core/modules/block_content/src/Tests/BlockContentListViewsTest.php
@@ -10,19 +10,20 @@
+ * Tests the views-powered listing of custom blocks.

s/views/Views/

(because referring to the module, not *a* view or a set of views)

Bojhan’s picture

Does not follow the empty pattern https://www.drupal.org/node/1146122

jibran’s picture

Status: Needs review » Needs work

Thanks @Wim Leers and @Bojhan for the review. NW as per #17 and #18.

Empty text
  • Use Text pattern "There are no [things] available. Add [a thing]".
  • The "Add a [thing]" link is the same action as the Local action link on the same page.
kim.pepper’s picture

Assigned: pameeela » kim.pepper

Taking a look.

kim.pepper’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
32.49 KB
29.38 KB
31.72 KB
17.55 KB
1.24 KB

Fixes nitpicks in #17 and removes and unused 'use' statements.

Re #18 the empty pattern is not followed by content, files, or comment views either. Are there issues to convert those? Otherwise, this is consistent with them.

Examples:

Content:

Files:

Comments

Bojhan’s picture

Yup, they fail - and should be fixed.

pameeela’s picture

Can we entertain the idea of changing the guidelines? I am guessing that the others are done this way because of content - "There are no content available." obviously doesn't work, and we don't want to use "nodes" - so why not stick with "No __ available"?

jibran’s picture

+1 to #23.

Bojhan’s picture

I assume we use common sense here, grammer is context dependant. The guidelines only state that it should conceptually begin with stating there are no items and then a link to add an item. How that is phrased largely depends on the context. So feel free to deviate here, its mostly about the concept - not the exact phrasing.

larowlan’s picture

+++ b/core/modules/block_content/config/install/views.view.block_content.yml
@@ -0,0 +1,441 @@
+          perm: 'administer blocks'

for others - note this is here, so same permission for 'adding' so we can put a link in the area, even though no other listings meet the guidelines and we have an 'add block' action link.

larowlan’s picture

Assigned: kim.pepper » larowlan

fixing UX issues

larowlan’s picture

Assigned: larowlan » Unassigned
Issue summary: View changes
FileSize
54.01 KB
5.48 KB
21.62 KB

So did this in the same fashion as the front-page.
To do it cleanly, we have to provide a plugin - else the link in the area won't resolve for sites in a sub-domain.

Screenshot.

dawehner’s picture

I'm curious, can we generalize this area?

Wim Leers’s picture

10:07:55 larowlan|away: @wim could you perchance eyeball https://www.drupal.org/node/2375589 and tell me what #cache stuff is needed on that area plugin?

Eyeballed it, and:

+++ b/core/modules/block_content/src/Plugin/views/area/ListingEmpty.php
@@ -0,0 +1,91 @@
+        '#access' => $this->accessManager->checkNamedRoute('block_content.add_page', array(), $this->currentUser),

You'll want to pass TRUE for the final parameter, so that you get an AccessResult object that contains cacheability metadata.

In #2444231: Fix CacheableInterface so it makes sense (was: "Make Config objects & Entities implement CacheableInterface + add BubbleableMetadata::createFromCacheableObject()"), we're making it simple to apply an access result to a render array. For now, you can do something like this:

$access_result = …

$element = array(
  …
  '#access' => $access_result->isAllowed(),
  '#cache' => [
    'contexts' => $access_result->getCacheContexts(),
    'tags' => $access_result->getCacheTags(),
    'max-age' => $access_result->getCacheMaxAge(),
  ],
  …
);
larowlan’s picture

FileSize
1.26 KB
21.88 KB
larowlan’s picture

pameeela’s picture

Just as a note on the empty text, I feel pretty strongly that "No custom blocks available" is better than "There are no custom blocks available" seeing as it conveys the same thing with two fewer words. I updated the wiki to reflect this: https://www.drupal.org/node/1146122

I made follow ups for content #2454309: Add "Add content" link to empty table text at admin/content and files #2454311: Add "Add a file" link to empty table text at admin/content/files to add an action link but not to update the text. But I don't think we need to add a link to prompt editors to "Add a comment"? Just doesn't seem necessary.

jibran’s picture

This issue is ready. Thanks for the patch and doc updates.

+++ b/core/modules/block_content/src/Plugin/views/area/ListingEmpty.php
@@ -0,0 +1,97 @@
+        '#links' => array(
+          array(
+            'url' => Url::fromRoute('block_content.add_page'),
+            'title' => $this->t('Add a custom block'),
+          ),
+        ),

You copied it form frontpage the ul makes sense there not here IMO. So can we stick with the pattern here by just using the inline link.
Just for the record I still want it to be #23.

larowlan’s picture

Can I do an inline link without SafeMarkup::set or a twig tpl and theme hook?
Just using t?

jibran’s picture

You can always use inline twig template.

larowlan’s picture

Would rather not

jibran’s picture

jibran’s picture

Status: Needs review » Needs work

The last submitted patch, 38: convert_custom_block-2375589-38.patch, failed testing.

jibran’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
590 bytes
22.13 KB

Test fix.

kim.pepper’s picture

Status: Needs review » Needs work

Just a couple of nits, then +1 RTBC from me:

  1. +++ b/core/modules/block_content/src/BlockContentViewsData.php
    @@ -40,6 +40,13 @@ public function getViewsData() {
    +      'help' => t('Provides a link to the add a new block.'),
    

    Provides a link to add a new block.

  2. +++ b/core/modules/block_content/src/Plugin/views/area/ListingEmpty.php
    @@ -0,0 +1,92 @@
    + * Defines an area plugin to display a block/add link.
    

    Defines an area plugin to display a block add link.

jibran’s picture

Status: Needs work » Needs review
FileSize
1.2 KB
22.13 KB

Thanks for the review. Fixed #42

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community
kim.pepper’s picture

Status: Reviewed & tested by the community » Needs review

Re-invoking testbot

pameeela’s picture

I am getting an error trying to install with this patch on simplytest. I tried three times with the same result, and it installs fine without the patch. Might be nothing but just thought I'd mention it, since it is related to views.view.block_content.

An AJAX HTTP error occurred.
HTTP Result Code: 200
Debugging information follows.
Path: /core/install.php?langcode=en&profile=standard&id=1&op=do_nojs&op=do
StatusText: OK
ResponseText: Drupal\Core\Config\UnmetDependenciesException: Configuration objects (views.view.block_content) provided by block_content have unmet dependencies in Drupal\Core\Config\UnmetDependenciesException::create() (line 89 of /home/s0e72ad578d553df/www/core/lib/Drupal/Core/Config/UnmetDependenciesException.php).

Status: Needs review » Needs work

The last submitted patch, 43: convert_custom_block-2375589-43.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
345 bytes
22.13 KB

optional config now goes in optional

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 49: views-custom-block.49.patch, failed testing.

Wim Leers’s picture

Failing due to incomplete schemas. Not sure what's causing this. I featured one of my recently committed patches caused this, but it did not.

In the mean time, a small review, pointing out things that are not causing the test failures:

  1. +++ b/core/modules/block_content/config/optional/views.view.block_content.yml
    @@ -0,0 +1,462 @@
    +        0: cache.context.url
    ...
    +        0: cache.context.url
    

    These should now be url.

  2. +++ b/core/modules/block_content/config/optional/views.view.block_content.yml
    @@ -0,0 +1,462 @@
    +        2: language
    ...
    +        2: language
    

    These should now be languages.

These wrong cache contexts will trigger validation errors once #2451679: Validate cache contexts (+ cache contexts in some views plugins wrong) lands — so then it won't be possible to have green tests with these bugs :)

jibran’s picture

#52 can be fixed after re-importing the view. Don't know about the schema issue I blame #2456707: Block Content views field handlers need to be replaced with entity-aware handlers

larowlan’s picture

Priority: Normal » Major

I reckon this should be major because views conversions

larowlan’s picture

Status: Needs work » Needs review
FileSize
2.48 KB
22.95 KB

re-export

larowlan’s picture

back to rtbc anyone? green again

pameeela’s picture

FileSize
22.93 KB

Very minor modification to update from "There are no custom blocks available" to "No custom blocks available" to match the other pages.

Please please let's get this in!

Status: Needs review » Needs work

The last submitted patch, 57: views-custom-block.57.patch, failed testing.

pameeela’s picture

FileSize
22.93 KB

Oops, ended up with curly quotes, fixed in this patch.

pameeela’s picture

Status: Needs work » Needs review
kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community
jibran’s picture

#52 is fixed and patch is green so RTBC +1.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 59: views-custom-block.59.patch, failed testing.

kim.pepper’s picture

Call to undefined method Drupal\node\NodeStorage::invokeFieldMethod()

larowlan’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
22.95 KB

reroll

larowlan’s picture

Random failure

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 65: views-custom-block.65.patch, failed testing.

Status: Needs work » Needs review
jibran’s picture

Status: Needs review » Reviewed & tested by the community
Wim Leers’s picture

Not random; testbot is currently broken. Will likely fail again. No point re-testing right now.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 65: views-custom-block.65.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 65: views-custom-block.65.patch, failed testing.

Wim Leers’s picture

Issue tags: +Needs reroll
larowlan’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
23.01 KB
larowlan’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 75: views-custom-block.75.patch, failed testing.

Status: Needs work » Needs review
larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Random fall

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/block_content/src/Plugin/views/area/ListingEmpty.php
    @@ -0,0 +1,92 @@
    +   * @param \Drupal\Core\Access\AccessManagerInterface $access_manager
    +   *   The access manager.
    +   */
    +  public function __construct(array $configuration, $plugin_id, $plugin_definition, AccessManagerInterface $access_manager, AccountInterface $current_user) {
    

    Missing @param for $current_user.

  2. +++ b/core/modules/block_content/src/Plugin/views/area/ListingEmpty.php
    @@ -0,0 +1,92 @@
    +      /** @var \Drupal\Core\Access\AccessResultInterface|\Drupal\Core\Cache\CacheableInterface $access_result */
    

    Drupal\Core\Cache\CacheableInterface does not exist anymore.

  3. +++ b/core/modules/block_content/src/Tests/BlockContentListViewsTest.php
    @@ -2,27 +2,27 @@
    +use Drupal\Core\Url;
    

    Not used

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
23.37 KB
2.02 KB

Fixes for #80

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0a89a7a and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

jibran’s picture

Status: Fixed » Reviewed & tested by the community

Not pushed yet.

  • alexpott committed 0a89a7a on 8.0.x
    Issue #2375589 by larowlan, pameeela, jibran, kim.pepper, Wim Leers,...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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