Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
This is a followup issue for #1535868: Convert all blocks into plugins. #1874598: Add BlockTestBase should also be completed before or as a part of this issue.
- In
custom_block.module
, custom blocks are read from and written to the database table directly. - There are currently no hooks for contributed modules to respond to custom block CRUD.
Proposed resolution
- Convert custom blocks to the entity system.
- Add full test coverage, including unit tests for CRUD, test coverage for hook implementations, etc.
Remaining tasks
Sandbox is here:
http://drupalcode.org/sandbox/larowlan/1790736.git/tree/refs/heads/custo...
Convert custom block derivative plugins to be keyed by machine name, add machine name to {custom_block} - avoid use of bid where possibleAdd view mode to block instance config and use this in the block render callbackFinish the storage controller (not yet pushed)Tests, Tests, TestsForm alters to add in translation support switch to block type configuration form
Related issues
- {#1875252]
- #1871696: Convert block instances to configuration entities to resolve architectural issues
- #1874598: Add BlockTestBase
- #1874584: "Block Library" link label is unclear
- #1875058: Provide separate interface for editing custom blocks
- #1875780: "Configure block" button text in the Block Library is confusing
Follow up tasks
- #1919910: Refactor custom_block upgrade path to create tables using hook_schema_0() instead of in block_update_N()
- #1919914: Set page title when editing/managing custom block type or content type
- #1919916: Change label of custom block default body field from "Block body" to just "Body"
- #1919920: Investigate adding a _view hook for blocks and use that to set contextual links rather than in a _view_alter hook
- #1919922: Improve DX around setting the theme when adding a (custom) block
- #1919924: Combine theme_node_add_list, theme_custom_block_add_list, and theme_admin_block_content for a generic theme_admin_list
- #1919926: Simplify custom_block_menu_local_tasks_alter().
- #1919928: Resolve usage of 'view content' permission in modules other than Node
- #1919930: Bundle entity form IDs violate module namespaces (both on server-side + front-end CSS)
- #1919934: Change key of custom block vertical tabs to use 'advanced' instead of 'additional_settings'
Comment | File | Size | Author |
---|---|---|---|
#111 | custom-block-types-or-block-types.jpg | 117.17 KB | Dries |
#111 | adding-a-new-block.jpg | 319.37 KB | Dries |
#111 | quote-block-type.jpg | 203.15 KB | Dries |
#111 | add-custom-block.jpg | 390.89 KB | Dries |
#111 | add-custom-block-2.jpg | 515.1 KB | Dries |
Comments
Comment #1
David_Rothstein CreditAttribution: David_Rothstein commentedSee also: #430886: Make all blocks fieldable entities
Comment #2
andypostDepending on what the block is ... (content || config) ... I think #1833872: Allow ConfigEntity types to be fieldable needs to be fixed too
Comment #3
xjm#2 is not applicable here, since custom block definitions should be normal content entities. Block instances should be configuration entities (in a separate issue, #1871696: Convert block instances to configuration entities to resolve architectural issues), but they also don't need fields.
#430886: Make all blocks fieldable entities has been closed as a duplicate of this issue. Edit: Or someone keeps reopening it. But it is obsolete following the blocks plugin conversion.
Comment #3.0
xjmUpdated issue summary.
Comment #4
xjmComment #4.0
xjmUpdated issue summary.
Comment #4.1
xjmUpdated issue summary.
Comment #5
larowlanSo I wanted to start plugging around on this, so figured I'd clone the sandbox from #1535868: Convert all blocks into plugins and use the 1535868-blocks branch as the basis for my work.
Is the sandbox 1535868-blocks branch in the summary up to date?
I get a 'page not found' trying to edit a custom block (by clicking the edit link for a custom block on the custom block listing page at admin/structure/block/list/block_plugin_ui%3Abartik/add/custom_blocks_library)
Comment #6
xjmYes, the branch is up to date. I'm not able to reproduce the problem you describe. Is it a fresh install from a fresh clone? If so could you provide explicit STR?
Comment #7
xjmWait, I get the same message if I go to
admin/structure/block/list/block_plugin_ui%3Abartik/add/custom_blocks_library
by pasting it in, but I don't know how you got to that path.Comment #8
xjmAh, I found it; there's another local task:
Looks like this was going to be the separate admin interface for custom blocks, but it was never finished. In
custom_block_admin_custom_block()
there's only markup for this form, with no submit handlers or anything. I'll just remove this fragment of a UI from the plugins patch in favor of whatever interface we add here.Comment #9
larowlanNice, so that means the ui for the custom blocks is part of this issue ala admin/content/node or admin/content/blocks in bean land?
Comment #10
xjmSomething like that. The exact workflow is to be determined, but having them as entities will simplify creating the admin interface. Edit: see the followup issue: #1875058: Provide separate interface for editing custom blocks
Comment #11
xjmComment #12
larowlanSpoke with EclipseGC on irc.
Being bold.
If I get nowhere in a week, I'll unassign.
Comment #13
larowlanSome progress shots
Do we want the custom blocks to be revisionable? translatable?
Comment #14
hass CreditAttribution: hass commentedYes, for sure!
Comment #15
EclipseGc CreditAttribution: EclipseGc commentedAlso yes
Comment #16
larowlanAdded sandbox to issue summary
Comment #16.0
larowlanUpdated issue summary.
Comment #16.1
larowlanUpdated to-do and added sandbox
Comment #17
larowlanUpdated remaining tasks
Comment #18
larowlanWIP - making good progress, onto tests and then translations next.
@timplunkett has asked that the config entity (block type) be split into a separate follow up, will do that once tests are done.
Demo video of status
http://youtu.be/A5zcbAtAKJ0
Comment #18.0
larowlanupdated to-do
Comment #19
larowlanUpdated remaining tasks
Comment #20
hass CreditAttribution: hass commentedIs it possible to clear only this block from cache? Otherwise the complete site may be in performance troubles.
Node?
'Block with name %name does not exist. <a href="@url">Add custom block</a>.'
@url,
url()
seems missing?Comment #21
Gábor Hojtsy@larowlan: now that #1535868: Convert all blocks into plugins landed, is this still readily applicable?
Also wanted to add that on top of making blocks support content translation right away this would also be beautiful for making custom blocks readily in-place editable (as in #1882482: [meta] Unify editing approaches in Drupal).
Comment #22
Gábor HojtsyNever mind my #1535868: Convert all blocks into plugins applicability question, I looked at the video. Looks powerful :) :) Congrats.
Comment #23
xjm@Gábor Hojtsy, making custom blocks content entities was always part of the plan following #1535868: Convert all blocks into plugins. :)
Comment #24
larowlanAdded translation support by porting to use EntityNG et al (thanks @berdir for guidance).
Revision support appears to be working.
Lets see how much breakage there is against HEAD before I start on new tests.
Comment #25
larowlanAlso above fixes issues at #20
Comment #27
tkoleary CreditAttribution: tkoleary commented@larowlan This is awesome work and something that I think we have needed for a long time. However, in it's current state there are some pretty big usability problems with it which I have outlined in the attached image. I don't think I'm introducing any fundamentally new ideas here, just trying to better employ existing patterns and create consistency across UIs. Also a chunk of this probably applies more to eclipseGC and his implementation of plugin blocks which I am also taking apart.
Comment #28
tim.plunkettIt seems like that conflates making a new block type with making a new block entity.
Also, the description part with the checkbox, for example, is not a pattern found anywhere that I know of outside the Views UI)
I think that a custom bundle UI should be separate from this issue.
Comment #29
tim.plunkettI guess what I mean is, let's just transition the backend over to using content entities, and worry about the UI separately, like we have for every other conversion.
Comment #30
tkoleary CreditAttribution: tkoleary commented@tim.plunkett I'm ok with that as long as it gets done. :)
Comment #31
tkoleary CreditAttribution: tkoleary commented@tim.plunkett Should I start a new issue for that and link to this one?
Comment #32
larowlanthanks @tkoleary, I agree with much of your analysis. You are correct in saying much of this is down to existing state. I think the best approach is to file follow ups for each of the issues. The description of block types will need to be in a follow up (assuming the block type concept is removed from this patch as per @timplunkett's request to keep this patch smaller).
Comment #33
tkoleary CreditAttribution: tkoleary commented@tim.plunkett But re: "seems like that conflates making a new block type with making a new block entity."
I don't follow. The design maps exactly to the node flow.
Comment #34
EclipseGc CreditAttribution: EclipseGc commentedActually the problem with the mockups is that it conflates creating new custom blocks with placing new block instances, and more importantly, puts those forms on the same page (which is a technical limitation I ran into LAST time I tried to do this, and one I'm not willing to be blocked on). That's why I asked for it to be wizarded together the way I did. Otherwise it seems pretty close to what you've outlined here.
Eclipse
Comment #35
Gábor HojtsyWhere is that problem? When I try to add a new custom block now it asks me the block description, title, body AND the region on the page at
admin/structure/block/list/block_plugin_ui%253Abartik/add/custom_blocks
. It clearly does not currently create a new instance, because although it looks like it does on the outset it says on the form fields, that it would merely change the one existing custom block body and description if I enter something, so essentially current Drupal 8 can only have up to one custom block on the whole site(?).Is this what got you to the technical limitation you have run into earlier?
Comment #36
tkoleary CreditAttribution: tkoleary commented@eclipseGC The mockup assumes the user is creating a new custom block from an (already existing) custom block type. If it is necessary to also create a new instance of the (first) use of this custom block then that should be done in the background IMHO.
Comment #37
larowlanblocked on #1885542: DatabaseControllerNG does not rollback failed ::save() operations
Comment #38
larowlanProgress update: 4 failing tests to go, will continue on Monday morning.
Needed some of the field_attach_* changes from #1778178: Convert comments to the new Entity Field API to get field output with EntityNG, sunk a heap of time into debugging that :(.
Comment #39
larowlanOk, this adds tests for all new functionality as follows
sandbox commits
still to do
Comment #41
xjmComment #42
larowlanNew commits
Updating remaining tasks on issue summary.
Comment #42.0
larowlanUpdated remaining tasks
Comment #43
larowlanComment #45
larowlanFixes mis-named update function.
Now blocked on #1887904: update_retrieve_dependencies() only considers enabled modules
Comment #47
larowlan#45: custom-blocks-content-entities-1871772.45.patch queued for re-testing.
Comment #49
larowlanre-roll against head after #1778178: Convert comments to the new Entity Field API went in.
Comment #50
hass CreditAttribution: hass commentedThere is at least one debug() call
Comment #51
larowlanat least it was clean, could have been much worse given my usual standards
Comment #52
larowlanOk, so this is green now.
We need to decide if the custom bundle ui should be taken out of this patch or added as is and cleaned up in #1875058: Provide separate interface for editing custom blocks.
Not fussy either way.
Comment #53
larowlanOk, so here's latest patch after #1871696: Convert block instances to configuration entities to resolve architectural issues went in.
It won't pass, I can't get CustomBlockTranslationUITest.php to pass locally, so something else in core has changed that I'm not across.
Basically, the translated field values are not being stored properly. Will try and debug it again tomorrow morning but hoping someone might notice this and go oh, that was '#123456 Random translation ui change you didn't know about'.
Also found changes in #611294: Refine permissions for Field UI were breaking some of the new tests too, updated that to request a change notice.
Had a discussion with @sun on irc about this, he advocates removing the machine name from the plugin ids, and revert back to the serial bids, leaving the 'content staging' question for another issue. I'm kind of torn, because we are running out of time and I feel that being able to deploy layouts that include custom blocks will be pivotal to my ability to do my job effectively for the Drupal 8 life-cycle. If we don't get it done in another issue after this point, we might have let something slip here. That said, I see his point about there needing to be a larger discussion about content staging. I guess it all depends where layouts /SCOTCH ends up. This patch still uses machine name. I will rip it out if thats the consensus we come to.
Patch still includes the bundle ui, same deal again.
Patch also includes some coding standards cleanup (run through coder_sniffer) for the new tests.
A shiny pony to anyone who can fix the translation test (latest code is in the sandbox).
Let's see what else is failing (please bot).
Comment #55
larowlanheh, can't grant 'administer custom_block fields' permission without field_ui enabled, dropping that perm for all but the block type test.
Comment #57
larowlanSo, turns out EntityNG you unset all the values except langcode during ::init method.
Also cleaned up the language config element on bundle, didn't need to use the long-way round comment bundle method.
If this comes back green I consider it ready for real reviews and for the discussion around machine names and the bundle ui.
Comment #59
larowlan#57: custom-blocks-content-entities-1871772.57.patch queued for re-testing.
Comment #61
larowlanLooks like random test failures, postponing for now until #1893800: Something is very, very wrong with update.php / upgrade tests... demons suspected is resolved
Comment #62
larowlan#57: custom-blocks-content-entities-1871772.57.patch queued for re-testing.
Comment #63
larowlanYay, green again
Ok, this is ready for real reviews and the discussion around bundle ui and machine name.
thanks in advance
Comment #64
andypostPatch looks awesome! There's a few code-related issues
custom_block <=> {block_custom*} tables?
Suppose naming should be unified.
Also changing the tables means no way for contrib to upgrade data. Suppose module should use own tables by migrating {block_custom} data to them. See #1860986: Drop left-over tables from 8.x
Place title to hook_menu()
same
A kind of magic???
Please use [module]_arg_load() pathern here
[module]_entity_uri()
entity_label for hook_menu() is enough
same
Please use parent::buildRow() as default and make a title as link, we need consistency for admin listings in core, more in #1855402: Add generic weighting (tabledrag) support for config entities (DraggableListController)
Different access methods - is there any reason for?
Comment #65
larowlanThanks @andypost, keep the reviews coming!
While I think of it, in regards to using bid/machine_name in block config - quoting @timplunkett from #916388-143: Convert menu links into entities
The same applies here, at the moment I'm using machine name over bid (HEAD uses bid), but uuid works for me too.
Comment #66
larowlanWe can't because these will be seen as implementations of hook_ENTITY_TYPE_load() in both instances.
During init, the various properties are raw values, we use the unset() method so that magic getters/setters apply. However the ->set()'s can go.
Made all the other suggested changes and merged in 8.x.
Added to list of deprecated tables in #1860986: Drop left-over tables from 8.x
Updated for hook_entity_bundle_info() and hook_entity_view_mode_info().
Go bot go!
Comment #67
larowlanGreen again!
Comment #68
BerdirHere's a review of the entity related stuff...
I think the goal is towards standardizing the primary key of an entity as 'id'. (and having something like user_id, node_id, custom_block_id for references).
As this is completeley new code, it makes sense to start with that.
Not sure what is the plan for the revision id but I'd go for revision_id.
Nitpick: I think the primary key usually comes first, I was actually confused for a second and wondered why there is none :)
I think at least the label (info?) should be part of the revision table as well, so that it can be revisioned..
Hm. I think we do want to move away from dwr(). My suggestion would be to use normal db_insert() queries here. You do have a fixed list of properties that you know you want to store...
Is there a reason for not doing this at the same time?
The previous update function already assumes that there aren't so many custom blocks or it would need a batch function too.
Wondering if this should depend on the filter update function that converts filter formats to CMI and get the information from there.
\Drupal\...
Woah, drupal_goto() as a page callback is ... interesting ;)
We want to remove that function (it is a major task, actually), so I think we need to find another solution here.
Should we already introduce an access controller and use that?
- Not sure if we really want to use the machine name here (not exactly sure what the idea/goal of that is)
- What exactly is this page for? A block isn't really meant to be displayed like that or not? Is it for admins to see how it looks? Is the view custom blocks permissions specific to this page or is it always checked when blocks are displayed?
This could use entity_load_by_properties() I think.
I know where you copied that from :)
We depend on block.module, so the check isn't necessary in this case.
The label method is a bit tricky, because this overrides the configurability. You might want to leave it away and use the default implementation unless there is a problem with that.
Search and replace Definition of with Contains :) And \Drupal\...
Duplicate code, probably a merge left-over...
Comment #69
larowlanThanks berdir, great feedback.
Will address all these points shortly.
The block page view callback is required for translation entity. It won't add it's tabs and menu callbacks if there is no page view of the entity. If we can get around that in a generic fashion, happy to remove it. I didn't think adding a chunk of conditional code in that module, specific to custom block was the way to go and also thought duplicating translation entity code in a hook menu alter in custom block module seemed no better. What are your thoughts?
Comment #70
BerdirAh, I see. IMHO, having an edit tab should be enough to be able to get the translate tab, I'd say that's something that should be improved in translation_entity.module. I'd suggest you talk with @plach about that.
Comment #71
larowlanThis is only used in the block library. The label can be configured when placing the instance and all uses of the instance will fetch that label (from the instance config entity). i.e. they are two distinct entities, the block instance (config entity) where the label used when placing the block is set and the other the base entity (custom block entity) which provides the plugin that the instance encapsulates.
Have sent plach a message on irc. Thanks.
Comment #72
larowlanBerdir clarified that he meant, I'm overriding EntityNG::label here, which means the label entity key can't be overridden.
Makes sense now.
Comment #73
larowlanI think this covers all of @berdir's points except
I just removed that hunk, it wasn't used.
Comment #75
larowlanmissed the entity key changes after renaming table cols
Comment #77
larowlangroan, revision_id != revision_vid
Comment #79
larowlanSo there's a shipload of vid and bid in the test code and a few refs in the controllers, need to rename those too. Have some grep results and will resolve asap.
Comment #80
larowlanThis should be closer now
Comment #82
larowlanThat test passes locally. Kicking off again
Comment #83
larowlan#80: custom-blocks-content-entities-1871772.80.patch queued for re-testing.
Comment #84
larowlanGreen again
Comment #85
larowlanThis one ready for serious reviews and the conversations about machine name/uuid/bid in the plugin id and the block type ui.
Tag update.
Comment #86
hass CreditAttribution: hass commentedIs there a need to get this patch committed before 18th February deadline? It would be extremly bad if this is not going in D8...
Comment #87
EclipseGc CreditAttribution: EclipseGc commentedThis should now be:
I prefer to see this on the menu callback if possible. If not that's fine too.
Is there a reason these aren't methods on the entity classes? They support that.
I think there's a listAll() method or something that should do this for you. Maybe I'm full of crap. Entities aren't my forté.
Do we have to? maybe this is the best way to do it, I dunno.
++ for being on top of the NG train here.
?? looks like a bad character in dreditor.
I've not made it all the way through, but I am reviewing this. In general there are a lot of things that have changed in the entity system that need some tweaking here. if timplunkett could review it that'd be great. I'll try to finish my review tomorrow. I'll also try to install this tomorrow and give it a functional test.
Eclipse
Comment #88
larowlanSo this fixes the issues identified by @EclipseGC at #87 and #27.2 from @tkoleary
Also removes the machine_name in favour of uuid as discussed with @sun in IRC and @timplunkett quote at #65
Comment #89
larowlanGreen again - I think we're getting close!
Comment #90
tim.plunkettNo architectural problems left, IMO. Just some nitpicks and then RTBC.
This class should probably just be renamed. Our standard is that the class names should be differentiable from each other.
Why the @? We should try to avoid that now.
That '1' is unfortunate. Maybe the upstream code should use string keys?
Missing blank line.
Can we use Request here?
This should be
if ($types && count($types) == 1) {
, because count() returns 1 if its a non-array. Yeah. Seriously.Here and elsewhere, missing a slash (\Drupal\...)
Extra blank line.
Are we allowed to do that? Isn't that the same as just calling the function?
Can't this just rely on the default NULL value?
The Overrides goes at the top, with a blank line after it
What's the use case for this? Shouldn't this be covered by hook_ENTITYTYPE_presave/insert/update?
blank line
Unneeded double quotes
Missing blank line
These should be in increments of at least 5
I guess this is technically correct, but I'm so used to actually having helpful info like string/array/int...
Why not theme?
This should be "Overrides \Drupal\...."
You need to include the entity and entity_type here
What? Why not just entity_load_multiple('custom_block'), no $ids passed in?
Put that line back! :)
There is a method for this in DerivativeDiscoveryDecorator.
Don't `use` top level classes, just
catch (\Exception $e) {
inlineprotected function setUp(), and put a blank after the parent:: call please
Missing blank line and docblocks
Missing blank line before end of class.
Remove this :)
Missing trailing comma
This should state that its overriding a theme function.
Comment #91
larowlanAlso can _wdm_ get a commit mention here as he helped with this during the code sprint in Sydney.
This fixes all issues identified by @timplunkett except:
I don't have that in scope, $this->discovery is an annotation based discovery, ie I can't get the derivative decorator in that scope.
I added a todo for that (for now) for when #1874498: Provide and use API methods for retrieving base plugin and derivative names from block plugin IDs lands.
Comment #92
larowlanAlso re
That seems to be the way other entities are doing it.
I guess its simpler than
Comment #93
larowlanBack to green!
Comment #94
plach@larowlan:
This is ok for now, altough it's ugly: we shouldn't require entity-defining modules to repeat this code every time. I think #1834542: Implement hook_entity_create() to setup the default entity language should be the proper solution. Moreover when every entity will be converted to NG we will be able to automatically define the
language_select
form item in theEntityFormController::form()
method based on field definitions, as we do now for configurable fields infield_attach_form()
. See #1728780: Build and prepare a basic entity form through the Entity Property API.Comment #95
tkoleary CreditAttribution: tkoleary commented@larowlan
Ok I just tested this. I believe it functions as designed but it is definitely a usability fail. Problem is I can't tell whether the fail is in this feature or in blocks as plugins.
Let me just outline it and maybe you can enlighten me.
So lets start with "As a user I want to create a custom block and place it on a page in my site." + "As a user who has created a custom block, when I return later I want to be able to edit the content in the custom block"
So As usual I go to structure, blocks, add block, then I see "add custom block" all pretty good so far. I load "add custom block",
Problem #1: why does it not load in the overlay? This is strange to the user.
Continuing I create a title (bananaphone) and some content and save the block.
Problem #2: because I am not in the overlay when I save the block I am not returned to the blocks UI, I have to navigate back there.
Continuing to blocks UI I browse through the blocks.
Problem #3: where is my custom block?
So I guess that maybe I have to go back to the block library to find it. So I do that. Once there I see the block and it has a drop button.
Problem #4: I mus click "add block" to go to the block library when what I want to do is edit/configure not add.
Problem #5: The thing I want to do, add my block to the blocks UI, is not an option in the drop button.
I look down the list to find my block
Problem # 6: Even though the field I used when I created the block was labeled "title" the culumn that the titles are in is now labebed "Subject" (?!)
So I find my block and I guess that maybe if I configure it I can get it to appear in the blocks UI. I do that and I see "region". I select sidebar first and click save. Now I am back in blocks UI and I look for my block.
Problem #7: The column where I see (what I thought was) the title of my block is labeled "block" (?!!)
I find my block in sidebar first, good.
Now I return at a later point and I decide I want to change the body of my block. I go to structure/blocks and look for my custom block with the title bananaphone. I find the block.
Problem #8: The dropbutton does not have an option to "edit" (?!!!)
I figure maybe if I go to a site page and check the contextual links I might find the edit option there. I do that and it is there. I click edit and I an able to change the body of my block but I decide that I want to also edit the title of my block, but I don't see title, only "description" which is "bananaphone"
Problem #9: Why did the title automatically become the description?
Problem #10: edit does not show me the title field even though the users expectation is that the title is part of the content and I do not know that if I change the description it will also change the title.
Comment #96
Gábor Hojtsy@tkoleary: indeed, most of the issue you found are either issues existing in the Drupal 8 blocks UI or with integrations with this new feature and the blocks UI. I think what's critical to understand here is this is a huge patch that would be good to have to land sooner than later and the usability issues might be easier to rework afterwards, since people don't need to juggle with a 120k+ patch in a 100+ comment issue :)
Comment #97
tkoleary CreditAttribution: tkoleary commented@Gabor
Noted. I'll work on designing fixes.
Comment #98
larowlanHi @tkoleary, thanks for taking the time to review in this length responses as follows
Problem #1: why does it not load in the overlay? This is strange to the user.
hmm, I have hook_admin_paths defining block/add, block/add/* but because we get their via a RedirectResponse, this must not respect overlay. So that is beyond this patch, will try and find if we have an existing issue for that. If I trick the url to /#overlay=block/add%3Ftheme%3Dbartik it works.
Problem #2: because I am not in the overlay when I save the block I am not returned to the blocks UI, I have to navigate back there.
Yeah if I get to /#overlay=block/add%3Ftheme%3Dbartik it works from there, so same issue here, RedirectResponse doesn't play nice with overlay.
Problem #3: where is my custom block?
Problem #4: I mus click "add block" to go to the block library when what I want to do is edit/configure not add.
So these are solved by 2.
Problem #5: The thing I want to do, add my block to the blocks UI, is not an option in the drop button.
This is an existing issue since blocks as plugins
Problem # 6: Even though the field I used when I created the block was labeled "title" the culumn that the titles are in is now labebed "Subject" (?!)
Problem #7: The column where I see (what I thought was) the title of my block is labeled "block" (?!!)
These are existing too, we need new issues to resolve these, they are great novice tasks.
Problem #8: The dropbutton does not have an option to "edit" (?!!!)
I played around this morning with trying to add it. It is available to edit in the block library, but the plugin info is not available in this form so I can't add the button. I think this is a good follow-up candidate, it requires changes to that form. Correct me if I'm wrong though, I think the block page will go eventually anyway? In favour of layouts and master layouts?
Problem #9: Why did the title automatically become the description?
Problem #10: edit does not show me the title field even though the users expectation is that the title is part of the content and I do not know that if I change the description it will also change the title.
Yes, this is the two entity forms issue. One entity (the block instance - the configure link) contains the title. The title is per instance and can be overridden. The other entity (the edit link) is the actual block entity - where the other fields are found. Again I think moving to layouts/master layouts will remove some of this confusion, as the title will be part of the layout.
So I think we have the following 'follow-ups'. Some of which may already exist:
*Make RedirectResponse respect overlay.
*Unify use of subject/label/title in block ui
*If we don't end up with layouts/master layouts - make plugin info available in form data at admin/structure/block so other modules can extend operations.
Thanks again!
Comment #99
hass CreditAttribution: hass commentedMaybe the overlay issue is #1788888: Overlay at admin/modules closes on submitting the form.?
Comment #100
larowlanRe-roll against head.
Also, putting my hand up to maintain custom_block.module
Comment #101
EclipseGc CreditAttribution: EclipseGc commentedThis is because of the use of all the redirects. This is unnecessary. I did 2 simple things to change this:
Normally I hate this hook, but with the router changing completely in core, it has its uses and this is one of those instances. The intro logic is fun since I array_slice paths and stuff but it totally works. This removes the first of two redirects you have. The second is of course on block/add itself where you redirect when there is only one valid custom block type, and switching that to this fixes that as well:
Note that my first snippet of code allows you to remove the menu items that were doing the initial redirecting (that whole drupal_container() foreach loop can go).
I'm testing the patch now, this was the first thing I noticed and it was annoying enough I just fixed it. Will keep posting stuff all night I'm sure.
Eclipse
Comment #102
EclipseGc CreditAttribution: EclipseGc commentedYeah I think removing the redirects fixed this. I definitely ended up back in the blocks UI after adding a custom block.
Eclipse
Comment #103
hass CreditAttribution: hass commented#101 I'm not really sure, but it may not work if you customize your system menu as the path is changing. Than the
(substr($root_path, 0, 26) == 'admin/structure/block/list')
may not match.Comment #104
EclipseGc CreditAttribution: EclipseGc commentedMy version of this patch. I've not done a detailed review of the code, but enough other have that I'm pretty happy with the actual UI at this point. It's not perfect, but then it's building on an imperfect system and it delivers on all the things I wanted functionality wise. I'm very happy with this at this point.
Eclipse
Comment #105
EclipseGc CreditAttribution: EclipseGc commentedOh, I also cleaned up some whitespace issues. Maybe git will stop complaining now.
Comment #106
EclipseGc CreditAttribution: EclipseGc commented@hass
That's not any less true with real menu items.
Eclipse
Comment #107
tkoleary CreditAttribution: tkoleary commented@larowlan
Thanks for the detailed reply. Looking much better already.
True, but there should be an edit link there as well.
Comment #108
sunThe most important aspect here is this:
This introduces the SECOND entity in core that has REVISIONS! :)
That's so very utterly badly needed. Really. We've to standardize, clean up, and also abstract so many things in that space that this very argument alone should require us to commit this as soon as possible.
Reality is, the smaller details about the user interface and interaction, and coding patterns and whatnot, can still be happily adjusted in the next 12-24 months. In the grand scheme of things, those absolutely are secondary.
Here's my review: Pretty much all of the issues I'm raising can be addressed in follow-up issues; ideally we want to create them ahead of time.
Normally we handle this case via
hook_schema_0()
implementations +update_module_enable()
, but we can clean that up later.Normally we add drupal_set_title('Edit %label block type', ...) here, but we can improve this later.
(page title != menu link title)
Is this copied from
node_schema()
? Is it actually true?Limited to 4 chars - are you sure? :)
I just left some remarks in IRC that this implementation of
hook_menu_local_tasks_alter()
is one of the most complex I've ever seen and that I'm absolutely confident that it can be vastly simplified.But that's straight refactoring/simplification, so can happen later. :)
That said, in case #1864066: Simplify hook_menu_local_tasks() and MENU_DEFAULT_LOCAL_TASK usage will land first, this hook needs to be adjusted to be not an alter hook, since it adds a local task (instead of altering an existing).
Minor: Should default to just "Body", I think.
I'm concerned about module namespace issues here; technically, this module-specific field should at least start with "custom_block_" as it is owned by custom_block.module.
Core can make exceptions, of course (just like the "body" field on nodes and comments), but with D8+, we're nearing a phase of maturity in which such "hacks" are less easy to explain.
For adding stuff to block views, we should have a _view hook that is not a _view_alter hook. I don't know whether that exists or not, but in case it does not, we need one (and this one should be converted). Potential follow-up issue.
This definitely needs a follow-up issue to improve the DX/API situation.
My prediction is that there will be many more custom_block-alike modules in contrib in D8, and we really do not want them to have to repeat this request-futzing weirdness. :)
I... hrm. We have (minor) wrong "node" class name in there, but I don't really get why the /add callback optionally renders a list of (existing?) custom blocks that may be added - shouldn't that be a different controller/callback?
In any case, do we really need a theme function for that? Can't we make it a simple table or something? :)
Hm. That user permission belongs to Node module.
We definitely need a (major) follow-up issue for that.
The BUNDLE_TYPE_form pattern is a massive anti-pattern from Node module in the first place.
Let's create a (major) follow-up issue to change that form ID pattern to prevent such namespace conflicts (for all entity types).
Hm. Not sure whether this is the correct Language API we're supposed to use?
Off-hand, I can't tell how this works for other entity types though, so perhaps this is actually right...
In any case, I think a follow-up issue to clean up the "API" would be helpful for D8.
Our new standard is to call the vertical tabs container 'advanced' instead of additional_settings. Let's adjust that in a follow-up.
Comment #109
larowlanAdded follow-ups:
#1919910: Refactor custom_block upgrade path to create tables using hook_schema_0() instead of in block_update_N()
#1919914: Set page title when editing/managing custom block type or content type
#1919916: Change label of custom block default body field from "Block body" to just "Body"
#1919920: Investigate adding a _view hook for blocks and use that to set contextual links rather than in a _view_alter hook
#1919922: Improve DX around setting the theme when adding a (custom) block
#1919924: Combine theme_node_add_list, theme_custom_block_add_list, and theme_admin_block_content for a generic theme_admin_list
#1919926: Simplify custom_block_menu_local_tasks_alter().
#1919928: Resolve usage of 'view content' permission in modules other than Node
#1919930: Bundle entity form IDs violate module namespaces (both on server-side + front-end CSS)
#1919934: Change key of custom block vertical tabs to use 'advanced' instead of 'additional_settings'
Comment #110
Dries CreditAttribution: Dries commentedI just committed #1864066: Simplify hook_menu_local_tasks() and MENU_DEFAULT_LOCAL_TASK usage. This may require this patch to be re-rolled per sun's review in #108.
I like the follow-up issues in #109 but I don't see any for the UX issues that have been pointed out in the comments above. I saw a couple of issues related to the UX in the issue summary but maybe there are more.
Can someone update the issue summary with all the follow-ups from #109?
Comment #111
Dries CreditAttribution: Dries commentedI tried this patch. Here is what I found.
So I created a new block type called 'Quote', using the newly added datetime module (yay!). Here is what it looks likes (doesn't really matter for the rest of this comment):
But when I want to create a new instance of my quote block, I couldn't figure out where to go. Eventually I figured it out, but it wasn't intuitive at all.
(I was going to review more of this but simplytest.me expired my environment.)
Comment #112
Dries CreditAttribution: Dries commentedI haven't looked at the code yet, but I like where this is going from a feature perspective. As pointed out by many (including myself in #111), this patch has significant UX issues. We've held up, and are actively holding up, other patches for much smaller UX issues. I've also seen very little feedback on architecture, upgrade path or even performance. That said, I plan to review this patch's code tonight with the aim to commit it. It would be great if we could fix some of the UX issues today but I understand that may have to happen after feature freeze.
Comment #113
hass CreditAttribution: hass commentedWe also need to followup with the buttons implemented in #1751606: Move published status checkbox next to "Save"
Comment #114
xjmWe have #1875252: [META] Make the block plugin UI shippable for the UX regressions in the Block UI, so let's make sure anything from Dries' feedback that doesn't get fixed here has a followup under that meta.
Comment #115
Dries CreditAttribution: Dries commentedRe @xjm in #114: I just cross-posted my review to #1875252: [META] Make the block plugin UI shippable. Given that #1875252 is marked as "critical", I'm comfortable proceeding with this patch. I'll look at the code now.
Comment #116
Dries CreditAttribution: Dries commentedCommitted to 8.x. Moving to 'needs work', as it needs work based on recent reviews.
Comment #116.0
Dries CreditAttribution: Dries commentedupdated remaining tasks
Comment #117
EclipseGc CreditAttribution: EclipseGc commentedI think we can probably do the necessary work in the related issues in the issue summary as follow up to this one. Most of the problems specced here are issues with the interface as a whole and not specific to custom blocks.
Eclipse
Comment #118
sunBefore we're going to add a issue component to the d.o Drupal project:
#1920862: Rename custom_block.module to block_content.module
Comment #120
Gábor HojtsyIt is surprising a change notice was not demanded for this issue. I've added one at http://drupal.org/node/1933768.
Comment #120.0
Gábor Hojtsyadding reference to #1875252
Comment #120.1
larowlanUpdated issue summary.