Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
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. |
Comment | File | Size | Author |
---|---|---|---|
#81 | interdiff.txt | 2.02 KB | kim.pepper |
#81 | 2375589-block-library-views-81.patch | 23.37 KB | kim.pepper |
#75 | views-custom-block.75.patch | 23.01 KB | larowlan |
#65 | views-custom-block.65.patch | 22.95 KB | larowlan |
Comments
Comment #1
jhodgdonAs 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).
Comment #2
larowlanThere 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
Comment #3
askibinski CreditAttribution: askibinski commentedI 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.
Comment #4
dawehnerAdded 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.
Comment #5
jibranComment #6
webchickComment #7
dasjoHaving 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.
Comment #8
pameeela CreditAttribution: pameeela commentedWorking on this at DrupalSouth
Comment #9
pameeela CreditAttribution: pameeela commentedPatch 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):
Comment #10
pameeela CreditAttribution: pameeela commentedComment #11
kim.pepperNeed to remove the old code, and include additional columns ('block type', 'updated') in the test.
Comment #12
kim.pepperTalked with @jibran and agreed we need to leave the existing code and provide a new test for the view.
Comment #13
larowlanstart on tests
Comment #15
larowlanfinalized tests
Comment #16
jibranI think this is ready. We just need a usability review. Screenshots are in #9.
Comment #17
Wim LeersUsability reviewers: look at
/admin/structure/block/block-content
.Looking at the code, this is the only nit I could find:
s/views/Views/
(because referring to the module, not *a* view or a set of views)
Comment #18
Bojhan CreditAttribution: Bojhan commentedDoes not follow the empty pattern https://www.drupal.org/node/1146122
Comment #19
jibranThanks @Wim Leers and @Bojhan for the review. NW as per #17 and #18.
Comment #20
kim.pepperTaking a look.
Comment #21
kim.pepperFixes 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
Comment #22
Bojhan CreditAttribution: Bojhan commentedYup, they fail - and should be fixed.
Comment #23
pameeela CreditAttribution: pameeela commentedCan 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"?
Comment #24
jibran+1 to #23.
Comment #25
Bojhan CreditAttribution: Bojhan commentedI 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.
Comment #26
larowlanfor 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.
Comment #27
larowlanfixing UX issues
Comment #28
larowlanSo 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.
Comment #29
dawehnerI'm curious, can we generalize this area?
Comment #30
Wim LeersEyeballed it, and:
You'll want to pass
TRUE
for the final parameter, so that you get anAccessResult
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:
Comment #31
larowlanComment #32
larowlanOpened #2453733: Create a general 'add new {entity_type}' area plugin and fix the empty behaviour for nodes, comments and files to match UX guidelines for existing issues and @dawehner's comment at #29
Comment #33
pameeela CreditAttribution: pameeela commentedJust 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.
Comment #34
jibranThis issue is ready. Thanks for the patch and doc updates.
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.
Comment #35
larowlanCan I do an inline link without SafeMarkup::set or a twig tpl and theme hook?
Just using t?
Comment #36
jibranYou can always use inline twig template.
Comment #37
larowlanWould rather not
Comment #38
jibranWe don't need #36 to fix #34. I think with that we are RTBC.
Comment #39
jibranAnd screenshot.
and it allowed under #1823450: [Meta] Convert core listings to Views see https://www.drupal.org/node/1823450#beta-evaluation.
Comment #41
jibranTest fix.
Comment #42
kim.pepperJust a couple of nits, then +1 RTBC from me:
Provides a link to add a new block.
Defines an area plugin to display a block add link.
Comment #43
jibranThanks for the review. Fixed #42
Comment #44
kim.pepperComment #45
kim.pepperRe-invoking testbot
Comment #46
pameeela CreditAttribution: pameeela commentedI 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.
Comment #49
larowlanoptional config now goes in optional
Comment #50
kim.pepperBack to RTBC
Comment #52
Wim LeersFailing 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:
These should now be
url
.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 :)
Comment #53
jibran#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
Comment #54
larowlanI reckon this should be major because views conversions
Comment #55
larowlanre-export
Comment #56
larowlanback to rtbc anyone? green again
Comment #57
pameeela CreditAttribution: pameeela commentedVery 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!
Comment #59
pameeela CreditAttribution: pameeela commentedOops, ended up with curly quotes, fixed in this patch.
Comment #60
pameeela CreditAttribution: pameeela commentedComment #61
kim.pepperComment #62
jibran#52 is fixed and patch is green so RTBC +1.
Comment #64
kim.pepperComment #65
larowlanreroll
Comment #66
larowlanRandom failure
Comment #69
jibranComment #70
Wim LeersNot random; testbot is currently broken. Will likely fail again. No point re-testing right now.
Comment #74
Wim LeersComment #75
larowlanComment #76
larowlanback to rtbc
Comment #79
larowlanRandom fall
Comment #80
alexpottMissing @param for $current_user.
Drupal\Core\Cache\CacheableInterface does not exist anymore.
Not used
Comment #81
kim.pepperFixes for #80
Comment #82
larowlanThanks!
Comment #83
alexpottCommitted 0a89a7a and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.
Comment #84
jibranNot pushed yet.
Comment #86
alexpott