Problem/Motivation
When you create a Custom Block entity, a Block Plugin definition is derived and is available anywhere Block Plugin definitions are listed. This makes a lot of sense when Custom Blocks are meant to be re-used, for example you may want a "Logo" Custom Block on many pages in different regions.
However when using Custom Blocks within the Layout Builder the current functionality has many problems:
UX
Currently to add a custom block to a Layout the user would have to
- Navigate to the custom block page
- Create the block
- Navigate back to the layout builder
- Click Add block
- Find the block they just created.
Access Control
If we allow inline block creation a user who creates a custom block on the Layout Builder for an access controlled entity probably expects only those that can view the entity that has the Layout to be able to view the custom block.
Currently blocks have no connection on an access level to where they are placed so this block would be able to placed via the block admin page, if you listed the block in a View or it was referenced in an Entity Reference value anyone would be able to see it.
Performance
When using something like Panels or Layout Builder, Custom Blocks are often only meant to be used once. If you're building out hundreds of pages you could end up with thousands of Custom Blocks and lead to performance issues. These blocks would be shown every place you are able to pick a block including the layout builder and the current block place listing.
Revision
Currently if you placed a block via the Layout Builder there is no connection between the revision of the block and revision of the entity with the Layout. If you updated a custom block all previous revisions of the entity with the layout would display the update version.
Deletion
If a custom block is deleted then it would be removed from all placements via the Layout Builder across all revisions.
Proposed resolution
Add the ability for users to create custom non-reusable blocks from within the Layout Builder. These blocks will be the same entity type as Custom Block provide by the block_content
module therefore all the site's current block types will be available from within the Layout Builder.
The non-reusable blocks will only be viewable within the context of the layout that they were placed in. For layout entities where the bundle is set to create a new revision for each update the changes to custom inline blocks will be tracked with the parent entities revision.
The new custom inline blocks should be "owned" by the layout that they are placed in in that they should *not* be editable or deleted from outside of the layout builder.
Implementation
In #2976334: Allow Custom blocks to be set as non-reusable adding access restriction based on where it was used. but included here in combined patch, review the do-not-test.patch for this issue.
See that issue Custom Block related implementation.
For integration with Layout Builder, we should create a new Block Plugin that stores Custom Block revision IDs and implement an inline form for creating and editing these Custom Blocks. All inline blocks created with this plugin will be non-reusable.
A simple database backed usage tracking service will be created to track usage of the inline custom blocks. These inline custom blocks will only ever 1 parent so it was determined that it does not need generic usage tracking
@see #2976366: Create generic entity usage tracking service
Remaining tasks
Write a patch to explore the feasibility of this approach.
Write tests
Review
User interface changes
Users would be able to create Custom Blocks inline from the Layout Builder. These blocks would not be available in the Custom Block library.
API changes
API changes needed for this change are being made in #2976334: Allow Custom blocks to be set as non-reusable adding access restriction based on where it was used.
Data model changes
Data model changes needed for this change are being made in #2976334: Allow Custom blocks to be set as non-reusable adding access restriction based on where it was used.
Comment | File | Size | Author |
---|---|---|---|
#250 | 2957425-250.patch | 92.42 KB | tedbow |
#250 | interdiff-239-250.txt | 572 bytes | tedbow |
#249 | interdiff_339_248.txt | 72.09 KB | mpotter |
#248 | 2957425-248.patch | 17.8 KB | mpotter |
#242 | 2957425-241-test-only-FAIL.patch | 93.34 KB | tedbow |
Comments
Comment #2
larowlanPlus one
Comment #3
johnwebdev CreditAttribution: johnwebdev commentedSince it was unassigned, I just started hacking something together, here's a starter patch with the reusability. Will create a block plugin and more tests later in the week.
Comment #4
johnwebdev CreditAttribution: johnwebdev commentedNeat. I could pretty much use the whole code provided from https://www.drupal.org/project/drupal/issues/2948064 and change the way its stored.
Heres an updated patch that allows you to add inline blocks. Still need to work with revisioning, update hook, etc.
Comment #5
johnwebdev CreditAttribution: johnwebdev commentedComment #6
johnwebdev CreditAttribution: johnwebdev commentedComment #7
johnwebdev CreditAttribution: johnwebdev commentedComment #8
johnwebdev CreditAttribution: johnwebdev commentedForgot the updated test.
Comment #9
johnwebdev CreditAttribution: johnwebdev commentedComment #12
johnwebdev CreditAttribution: johnwebdev commentedNew patch.
Comment #13
johnwebdev CreditAttribution: johnwebdev commentedComment #14
BerdirThis is a possible solution for #2940755: block_content block derivatives do not scale to thousands of block_content entities.
I think we should just not have used derivates for this initially, same for menus and other things and just gone with an autocomplete widget + some kind of pre-fill feature through a place action on the block overview, but too late for that :)
In general I agree with the idea but I don't think we should make it layout builder specific but allow all content blocks to be placed like this instead. That means also thinking about the exact wording for this and how the UI would work for normal blocks. And maybe eventually even deprecate the old way?
I'm also not convinced to use the block revision ID for this. That would IMHO be extremely confusing for users as it would make updating very hard? But I also didn't look closely at the patch.
Comment #15
samuel.mortenson@Berdir Using the revision ID is basically required for having Content Moderation and Layout Builder work nicely together - users expect Custom Blocks placed in Layout Builder to be tracked with the parent entity's revision history, much like how Paragraphs users need entity reference revisions when using Content Moderation.
Edit: The above doesn't mean the user experience makes sense in the current patch, just that we need this functionality for Layout Builder.
Comment #16
BerdirGood point. So we're basically building poormanscron paragraphs functionality here ;)
Comment #17
hawkeye.twolfAdding related issue #2948064: Layout Builder should support inline creation of Custom Blocks using entity serialization, as Custom Block (Non-)Reusability should be considered a replacement for the "inline blocks" approach mentioned there.
Comment #18
kevincrafts CreditAttribution: kevincrafts commentedTrying to understand what would happen with revisions in this scenario:
Is the output of the custom block on Page A and Page B identical? Or because of where the revision happened, the pages are displaying two different versions of the block?
Comment #19
johnwebdev CreditAttribution: johnwebdev commented@kevincrafts
Here's what I think:
Page A and Page B would have separately blocks in the first place because inline blocks are not intended to be "reusable" but only where it was created.
Instead, I think it should be something like:
1. User create a Page A (revision 1), and adds a Inline custom block "Hello foo!"
2. User updates Page A (revision 2) and also change the inline custom block to "Hello bar!"
3. User reverts Page A to revision 1 and inline custom block should now be back at "Hello foo!"
Comment #20
hawkeye.twolfThere might actually be an argument for having _both_ approaches (this issue's "re-usable" checkbox vs. inline-blocks of #2948064: Layout Builder should support inline creation of Custom Blocks using entity serialization). Most notably, this approach supports translation of individual blocks. If a site needed to maintain parity of layout between translations, editors could place the content as a "standard" block instead of inline-block and then accept the inherent limitations (e.g., no content moderation support until ERR makes it into core).
Comment #21
hawkeye.twolf@kevincrafts, I believe the behavior outlined by @johndevman is the ideal approach but falls outside the scope of this ticket. To support this kind of functionality, we would need ERR (or something akin to it) in core. Solving this very hard problem is one of the main motivators for going with a "serialized" approach (a la #2948064: Layout Builder should support inline creation of Custom Blocks using entity serialization) rather than saving blocks to the database as normal entities.
Currently, all we have is an ER field, which targets the entity ID and not the revision ID, so this content would not follow the expected revision pattern. This sub-optimal behavior is how things currently work when placing blocks via Layout Builder but is more acceptable / makes more sense when a block can be used in more than one place (its content doesn't "belong" to that particular node).
Comment #22
hawkeye.twolfAdding related core issue that's needed to help make ERR a viable solution.
Comment #23
kevincrafts CreditAttribution: kevincrafts commentedSo how would one go about placing a reusable block content on multiple pages with Layout Builder that would not cause the block content to diverge on the editing of a page?
Comment #24
tim.plunkettAll blocks are currently reusable. So it would work the same as it does today.
I guess this issue is more about "non-re-usability"?
Comment #25
hawkeye.twolf@kevincrafts, yes, it works like @tim.plunkett described. There would be two types of blocks:
Make sense?
Comment #26
BerdirThe thing with re-using (as in, the entity type, not a single entity) block_content with a new field however is that at least unless we adapt the view, /admin/structure/block/block-content will still list all those blocks and users will try to edit them through that, which will then not actually do anything as the embedded blocks reference a specific revision ID.
Also translations sounds like an interesting problem as each translation will have different revision id), I guess it would work with a translatable layout_selection field, but as config for an entity view display? And publishing draft translation revisions results in new revisions.
I might be biased as a paragraphs.module co-maintainer, but I would recommend to be very careful about trying to cover this with some kind of quick fix like this in core. As mentioned here, ERR/paragraphs is very complex.
As an different but possibly interesting idea, imagine what you could do if you couldn't just put fields into layouts but control each field item separately? E.g. you could have a multi-value body field and then split two deltas up into a two-column layout, or have a large image with smaller images below. Or have N paragraphs in a single paragraph field and place them on a page in whatever way you want ;)
Comment #27
tim.plunkettThat's sorta how https://www.drupal.org/project/paragraph_blocks works, and douggreen suggested a per-delta feature back in Baltimore. Not sure that it has a core issue yet.
Comment #28
hawkeye.twolf@tim.plunkett/@Berdir, if the content can be edited elsewhere (e.g., the node edit form), then referencing field deltas with Layout Builder presents some challenges. Targeting the actual entity revision ID (as we do here) gets around those challenges, but then we get back into the realm of needing ERR-like functionality in core (#2812595: Support revision tracking of entity references in core), and I echo @Berdir's concerns about attempting to re-solve the issues ERR has already surfaced and tackled w.r.t. moderation, translation, etc.
@Berdir, I'd love to get your thoughts on #2948064: Layout Builder should support inline creation of Custom Blocks using entity serialization as an alternate approach. Will you chime in there? Many thanks!
Comment #29
tedbowI have been working on #2948064-49: Layout Builder should support inline creation of Custom Blocks using entity serialization but I ran into a bunch of problems that make me(and others) think that issue won't work. You can see the reasons in the comment I linked to here.
So now looking at this issue to solve the problem. It looks like this patch has made great progress so far!
If we save blocks in form submit in this way then any block that is added in the Layout Builder will exist whether the user actually saves the Layout or not.
So if you add 5 new inline blocks but never actually save the layout then you have will have 5 new content blocks that aren't actually used anywhere.
It would probably make sense to keep this in tempstore and only save the blocks when the layout is saved.
This is also the case for editing existing blocks. If the blocks would be saved and new revision created, if applicable, and then if the user cancels the layout changes they likely do not want the changes to the blocks either.
Can we ship an updated View? Not sure how that works.
If not we documented that View should updated or reverted back to new core View. We may want to disable access to the block edit form for non-reusable blocks outside of the layout builder or at least at the existing edit route. Because presumably editing of reusable blocks should only ever happen in block plugin form. So we could prevent that.
Comment #30
tedbow\Drupal\layout_builder\Plugin\Derivative\InlineBlockContentDeriver::getDerivativeDefinitions()
to\Drupal\Core\Entity\EntityInterface::getConfigDependencyKey()
and\Drupal\Core\Entity\EntityInterface::getConfigDependencyName()
instead of using hardcoded stringBlockContentDeriver
toInlineBlockContentDeriver
to match the plugin nameInlineBlockContentBlockTest
be most comprehensive based on changes from #2948064: Layout Builder should support inline creation of Custom Blocks using entity serialization. To make this test work I had to add$block->setNewRevision(TRUE);
otherwise it wouldn't make a new revision. Without the new revision when you change the block in an override it actually changed the default also because it wasn't a new revision.Comment #32
tedbowHere is a failing test that proves #29.1
If you add an inline block to a layout but never end up saving the Layout because you either Cancel or Revert the content block still gets created and remains
@todo Create a similar test for editing existing inline blocks. When you edit a block but don't save the layout the content block changes remain.
Also in writing this test I found this #2970801: If you add block then try to Revert the layout it doesn't revert
Comment #34
johnwebdev CreditAttribution: johnwebdev commentedFirst attempt on #29.1.
Comment #35
johnwebdev CreditAttribution: johnwebdev commentedComment #37
tedbow@johndevman thanks for the patch.
I think this method will work though I am not sure what would happen with field fields. Will the files be removed?
I think the test failure is because of #2970801: If you add block then try to Revert the layout it doesn't revert
So created a dataProvider function for
\Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockContentBlockTest::testNoLayoutSave
to demonstrate that it works for "cancel" but not "revert"So this will still fail for the "revert" case but should pass for the "cancel" case.
Comment #39
johnwebdev CreditAttribution: johnwebdev commented@tedbow Good question. What would the expected state of the block be if, it's deleted on an entity? It should not be deleted, because of revisioning. So I suppose the image will remain, right?
It should be deleted if the host entity is deleted (like Paragraphs) though(?) We should write tests for this.
Comment #41
johnwebdev CreditAttribution: johnwebdev commentedAdded a start on testing revisioning. Needs more scenarios covered... but this is a start :)
Comment #42
tedbow@johndevman thanks for the continued progress! Setting to need review to run tests.
Comment #43
tedbowCan't use @inheritdoc on test methods that don't override a method.
I don't think all of these permissions are needed. We should only have the permission needed to do the actions in the test.
I think we should be able to just do
$page->pressButton('Revert')
like the other button presses in this method.Comment #44
tedbow\Drupal\layout_builder\Form\RevertOverridesForm::submitForm
\Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockContentBlockTest::testInlineBlocks()
to cover canceling/reverting a layout with blocks that existing in the layout but with edits since last layout save. The block edits should not be saved. This passes.This case is much more complicated and must handle these cases:
In this 2nd case the inline content block may still be referenced in a layout override for different entity for the same bundle if the override was created before the inline content block was removed from default. So in this case the content block should
not deleted
.But also in this 2nd case the inline content block may not be referenced if has removed from all overrides or if all the overrides were created after the inline block was removed from the default.
There is actually no way to know this except by checking all entities of this bundle that have overrides.
For this reason it may make more sense when first creating a layout override to duplicate any non-reusable blocks reference. Then when it is removed from an override it will then can safely be deleted from the content block storage.
Comment #46
johnwebdev CreditAttribution: johnwebdev commentedThis is a started patch on duplicating inline blocks when overriding.
Still needs to update test.
Comment #47
johnwebdev CreditAttribution: johnwebdev commentedComment #48
johnwebdev CreditAttribution: johnwebdev commented.. Meh.. whitespace.
Comment #49
johnwebdev CreditAttribution: johnwebdev commentedComment #51
johnwebdev CreditAttribution: johnwebdev commented#44 Regarding deletion, there many scenarios we need to take under consideration:
a) Override
1) When the entity is deleted
2) When an block is removed and the layout is saved
3) When the layout is reverted to default
b) Default
1) When the content type is deleted
2) When the block is removed and the layout is saved
c) Layout Builder uninstall.
Initially, I planned to create a class, and do something like:
on each of these scenarios through out the code, but I think it would be cleaner if we could implement some event handling for Section storages for these events post save instead? What do you think?
Edit: Posted this idea in Slack:
Comment #52
tedbowre @Berdir comment on #14
The changes we having been making to the patch to support not saving blocks before that layouts are saved have actually been connecting to Layout Builder module.
So with these changes the content block won't actually be saved/created when the block plugin configuration form is save.
This means if Block UI module is used to place the block then the content block won't actually be saved.
Here we actually do the saving in
\Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage::save()
. This connects the functionality to the Layout Builder module.I don't see how we get around this.
One idea would be to save the content blocks in
\Drupal\layout_builder\Plugin\Block\InlineBlockContentBlock::blockSubmit
so that it would work for Block UI. Then\Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage::save
if the block is no longer referenced, in the case it was removed, we delete the content block.But then what if user never actually saves the layout but just closes the tab. Then a content would have saved but it is marked as not reusable but it is not used anywhere, orphaned.
We basically need the block form to act 2 different ways. With the Layout Builder don't save Content Block then with other uses save the Content Block in
\Drupal\layout_builder\Plugin\Block\InlineBlockContentBlock::blockSubmit
.Could use a different plugin form for layout builder using the functionality we created in #2763157: Allow plugins to provide multiple forms?
This is what we used in Settings Tray to provide a different block plugin form when used with the settings tray.
So basically in
\Drupal\layout_builder\Form\ConfigureBlockFormBase::doBuildForm()
We could do something like
Copied from
\Drupal\settings_tray\Block\BlockEntitySettingTrayForm::getPluginForm()
Comment #53
tedbowThis gives a try at
It allows the configuration form to not save the content block when used with layout builder but to save the content block when used with all other uses, i.e. block ui.
Comment #55
tedbowThis requires the Layout Builder
\Drupal\layout_builder\Plugin\SectionStorage\SectionStorageBase
to know about 'inline_block_content' plugins.Really what need here is concept that plugin form could be submitted but we don't yet want to make the changes permanent. In this case it because don't want to create Content Block entity when the plugin configuration form is submitted. For all we know the user may never say the layout and the content block entity would be orphaned.
So in this patch I am created a
PermanentSavePluginInterface
to denote a plugin that has extra save step that makes permanent changes.So also renaming
saveInlineBlocks()
topermanentlySaveComponents()
.I renamed this in the patch but also does actually need to be on the interface? Couldn't this be a protect method on
SectionStorageBase
Comment #56
tedbowI feel like we should also abstract this away from
inline_block_content
plugins.Should we have some concepts of plugins that are not safe to just create duplicate configuration of. Meaning we should tell the plugin that we want to create duplicate and would do any necessary operations if any and update configuration if needed.
Comment #57
tedbowMostly nits.
Don't think we need both here we can just a have a $reusable parameter.
"Reusable" is one word. Changing all.
"upto" is 2 words. Won't comment on whether "spiffiness" is a word 😉
Should not have period.
Whoops I have fixed my editor now.
This can be move up to the parent class.
Can't change this back to protected but if we change
\Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage::duplicateDefaultsInlineCustomBlocks
to something more generic we can.Comment #59
johnwebdev CreditAttribution: johnwebdev commentedIt's taken from a similar test in core!
Nit: We can remove one of these.
Could be one-liner?
Comment #61
tedbowNow I am thinking what we actually need here is an event for preparing the layout for the UI.
Since Inline Content Blocks ultimately probably should live in the Layout Builder module then we don't want specific logic for them here.
I will give this a try.
Comment #62
tedbowHere is rough start for #61
The event doesn't work how I would like yet though I think it works for what inline blocks needs. But it would be nice if it fired whenever a layout was prepared for the UI not just creating an override.
I have to switch to something else now so just wanted to get this start out there but should able to get back this in a few hours.
Comment #64
tedbowSo more thoughts
For override we couldn't delete a content block when it was removed from the layout unless the parent entity is not using revisions
We could only delete when delete content blocks when the parent entity is deleted. and then we would have to make sure every content block referenced in ever revision is delete.
Since we could have 100's or 1000's of revisions this might be difficult to do on saving the parent entity.
We would only have to store the ids. This way we wouldn't have deserialize every layout revision.
We could add `
OverridesSectionStorage::addNonResubleEntityUsage()
` and `::getAllNonReusbleEnityUses()
` then on parent entity delete would could all `::getAllNonReusbleEnityUses()
` to get all entities to deleteThe only problem I can think of right now would be what if `::getAllNonReusbleEnityUses()` returned 100's of entities. could you delete them all when deleting the parent entity?
EntityUsage
service like have forFileUsage
service but I think should explore other options first likeOverridesSectionStorage::addNonResubleEntityUsage()
if we had a `
EntityUsageService
` then in `hook_entity_delete
` we could call `EntityUsage::removeUsagesWhereEntityIsUser($entity_deleted)
`Assuming that you only use this service for entities that should be deleted when not used elsewhere.
Comment #65
johnwebdev CreditAttribution: johnwebdev commentedThis doesn't do anything right now.
Can we make the Block description optional? It is not really required for many blocks when used inline. (it is required for the block UI, and if to be changed through Block UI, since required) but not in LB context, Perhaps we could use some sensible fallback if used in LB and you don't want the field shown.
Comment #66
johnwebdev CreditAttribution: johnwebdev commentedThis patch begins to explore the idea of tracking dependent entities on the storage. It implements a new interface PluginWithDependentEntitiesInterface, an idea that tedbow had.
Some hacky code from my experiments remains, but it works.
Comment #67
johnwebdev CreditAttribution: johnwebdev commentedComment #68
johnwebdev CreditAttribution: johnwebdev commentedI think we can store the ID here instead on the configuration?
Can the unserialize be done on the field item instead?
Needs docblock updated with return type.
Not sure which data type to use here?
This is not needed. It failed when I used the wrong data type.
Should we support any entity type rather than just block content?
Comment #70
tedbowNeed to check if $entity instance of FieldableEntityInterface
Not sure this method is needed on the interface. InlineBlockContentBlock doesn't need it and really neither does OverridesSectionStorage because it is only called internally
If each plugin keeps track of it's own dependent entities this doesn't need to be public.
For this to work outside of layout builder would need to return an array where the keys are entity type ids and values are array of entity ids.
layout_builder_entity_predelete
currently is hardcode forblock_content
entity type.This maybe the part I like best about the change in this interdiff, keeping the dependent_entities at a field property.
But it does kind of field weird to keep information about pass revisions in the current revision field value.
Not sure how this is related.
DefaultsSectionStorage
storage would also need to implementPluginWithDependentEntitiesInterface
because default also will have InlineContentBlock plugins. So maybeSectionStorageBase
would implement it instead.Now I wondering if
PluginWithDependentEntitiesInterface
was a good idea 😜Comment #71
johnwebdev CreditAttribution: johnwebdev commentedThe proposed solution for dependent entities in #66 won't work because it assumes that the latest/current revision keeps track of ALL dependent entities. If the entity is reverted to an older revision that could mean that some dependent entities are NOT in the new revision and latest revision.
So we'll continue from patch #62 instead, moving forward with perhaps the entity usage table.
Comment #72
tedbowThis adds tests for making sure content blocks are deleted when they should be. Currently it will fail.
Agree with #71, @johndevman thanks for exploring the PluginWithDependentEntitiesInterface idea.
Thoughts
Interested EntityUsage service/table idea which would be similar to file usage. 1 big difference to file usage is that the way it works currently is that a inline block content would only be used by 1 entity or default.
If we didn't duplicate blocks then that won't be true. I suggested duplicating blocks in override to make the usage tracking easier. So maybe if we had better tracking duplicating would not be better.
One strange situation if we didn't duplicate blocks in overrides would be you could get revision histories that would be:
Of course maybe this doesn't matter if you don't look at revisions for an individual inline block but if someone wanted to build a view to look at them there won't be a way to separate out what the revisions are connect to.
\Drupal\file\FileUsage\FileUsageBase::delete
actuallyworks deleting a file usage just marks the file has temporary and then
file_cron
actually deletes the files. In our case we would need to delete blocks on cron because there could be hundreds in the revisions for a single node. But if we are duplicating the content blocks from the default to the override we maybe able to skip the tracking.Here is an idea:
reusable
field from a boolean toreusable_state
an integer. Have 3 constants for the statesREUSABLE
,NON_REUSABLE_ACTIVE
, andNON_REUSABLE_INACTIVE
,reusable_state
toNON_REUSABLE_INACTIVE
.block_content_cron
deleteNON_REUSABLE_INACTIVE
content blocks.Now that I write it out maybe that doesn't help 😟. Doing hundreds of ::save() vs hundreds of ::delete() calls is probably not that much better.
For that reason when deleting a bundle we would only have to delete the inline content blocks that were currently on the layout
Comment #73
johnwebdev CreditAttribution: johnwebdev commentedComment #75
tedbowTalked with @johndevman. Going to give EntityUsage Service a try.
Comment #76
tedbowLooking back I think was wrong in #55 when I introduced the
PermanentSavePluginInterface
.The problem I was trying to solve was that I thought there wasn't a point where we could save the block after the configuration form was saved and we didn't want to save it when the configuration form was save because we would end up with orphaned content blocks.
Now I realize that what I added in
\Drupal\layout_builder\Plugin\Block\InlineBlockContentBlock::savePermanently()
can actually be done inhook_presave
if it is fieldable entity then just need to check forlayout_builder__layout
and if the entity type isentity_view_display
it will be in the third party settings.I am also wondering if we could get rid of this event altogether. Do we need to duplicate the blocks here or would be able to do this also in
hook_entity_presave
orhook_entiy_save
.When creating a new override we could check
$entity->original
to see if it is new or is there another way to know?Either when creating a new override we could duplicate the blocks we are actually saving the entity.
One drawback would this fire on every entity save vs this custom event.
Unassigning myself since not working on it currently
Comment #77
tedbowThis does #76.1 gets rid of the need for PermanentSavePluginInterface.
I think it makes sense not to introduce this if we don't need it.
Comment #79
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThere's no need to call
setInitialValue()
on a separate line :)I remember that we had to do some kind of optimization for loading config entities only by their ID, because it was quite slow otherwise. So we need to ensure that we're not losing it with this change..
There are some extra empty lines here that can be removed.
I think this means that the patch is doing two things:
1) allows a custom block to be re-usable or not
2) adds a new concept of "inline block content"
If that's the case, why aren't we doing it in two separate issues?
Comment #80
tedbow@amateescu thanks for the review!
I updated the summary to say why I think we should keep this as 1 big patch for now
Comment #81
tedbowThis patch removes the need for this event that was added in #62. The event was used to duplicate the content blocks from the default when creating an override layout.
This can actually be done in
layout_builder_entity_presave
without the need for a new event.The only problem is that to determine if the layout is new override I am using
$original_sections_field->isEmpty()
.This seems to work now because if you remove all the sections from an override and save the layout you actually revert to default. So if that is intentional then this method will work.
Would we ever want to allow the ability to save an override with no sections?
So that reduced the size of this patch a good bit 🎉
Comment #83
tedbowReview #79
1. fixed
2. BlockContent entities are not "config entities" does you comment still apply.
3. Fixed this in #81
Other changes.
Just note here these are outside layout builder. After we have a patch validates this whole reuse concept we will probably want split change to
block_content
module to it's own issue.Or if we really wanted to be experimental in this module we could actually extend
\Drupal\block_content\Entity\BlockContent
Use our own class for this entity. We could also swap out the deriver for our own to handle reuse.
Probably want a separate issue but just saying it could be done.
Unused "use"
Re #57.7 we don't need to be public anymore 👍
Comment #85
johnwebdev CreditAttribution: johnwebdev commentedBlock revision ID?
This will always be TRUE.
Nice rename. I am unsure about doing duplicate logic within the save method though. This behaviour is kinda coupled to LB only as well. Right?
Comment #86
johnwebdev CreditAttribution: johnwebdev commentedI did some manual testing:
General (things adressed in current patch)
I started playing some with Content Moderation, and I noticed this issue as well:
Also consider to #2942675: Layout builder should use the active variant of an entity to avoid orphaned revisions add as a blocker? We cannot properly Content Moderation integration without that issue solved.
I am mainly referring to the fact that blocks can be moderated as well.
Comment #87
tedbowOk this patch does a few things(also sorry have not read #85 & #86 yet, thanks @johndevman for looking into these issues)
$count == 0
to denote entities that were used but have had all there usages removed.InlineBlockContentUsage
service to keep track of Inline Block entities and make sure they are deleted when necessary. This logic is bit complicated I was doing this all in entity_crude hooks so I changed these hooks to just call the service.#2937199: Track Layout override revisions on entities which support revisioning keeps this from working correctly with revisions.
\Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockContentBlockTest::testDeletion
is still not passing. My manual testing seems to indicate that it working so I have figure if the test is wrong or if there is some other bug.Except for Inline Blocks that are placed via the Block UI the Inline Blocks are actually deleted on cron not entity deleted. It is possible that for
LayoutBuilderEntityViewDisplay
we should just delete the Inline Blocks when the block is deleted.It also attempts to delete Inline Blocks if the entity is not a new revision on update.
Comment #89
johnwebdev CreditAttribution: johnwebdev commentedGreat work @tedbow, here's just a quick review when looking at first part of the patch (it's quite big). Will look in to it more in depth later.
An entity ID can also be a string, so I don't think we can store it as int due to generic usage.
We can use EntityTypeInterface::ID_MAX_LENGTH instead.
I think we should keep it open, if possible.
file usage => entity usage
Defaults to 'v' => Defaults to 'entity_usage'
Nit: id => IDs?
$count is not used here.
Nice with $limit, was just thinking about it, when looking earlier in code.
Can we make the method name more explicit, is this used or user?
Comment #90
johnwebdev CreditAttribution: johnwebdev commentedComment #91
johnwebdev CreditAttribution: johnwebdev commentedOk, let's see what test bot says.
So I updated patch to delete node through UI, and it works. I also tested this manually. And I also tried deleting through Node::load(1)->delete() via CLI, which also worked. So the failing tests seems to be only happening in the test case...
I also updated testNoLayoutSave::revert to not assert empty blocks because with current logic in patch this is not the intended behaviour anymore.
Comment #92
johnwebdev CreditAttribution: johnwebdev commentedComment #93
johnwebdev CreditAttribution: johnwebdev commentedComment #95
johnwebdev CreditAttribution: johnwebdev commentedThis is just a rough start on working on test coverage for Entity Usage service. I think @tedbow planned doing some changes, so I just upload this now, that I can continue later when the API becomes more in its finished state.
Edit: Looks like random fails, since the diffs in this patch should not affect these tests.
Comment #96
johnwebdev CreditAttribution: johnwebdev commentedComment #98
tedbowre #95 thanks for starting
\Drupal\Tests\layout_builder\Kernel\EntityUsageTest
this patch expands on it which uncovered some bugs.In this patch
\Drupal\layout_builder\EntityUsageBase
because some the methods didn't actually do any database logic. These were entity related helper methods just to improve DX.::remove()
to return the total remaining uses of the child entity not just by parent being removed.Comment #100
tedbowI have confirmed the patch here #2937199-13: Track Layout override revisions on entities which support revisioning allows these test lines to be uncommented.
So that will need to be blocker for this issue to work correctly
Comment #101
tedbowThis does a few things
entity_usage
table in an update hook.Comment #103
johnwebdev CreditAttribution: johnwebdev commented#101
Can we do this? Given the user might have changed that view?
Nvm: Looks like this is being done in #2909435: Update block library page to adapt publishable block content implementation, so I suppose it's not an issue.
Comment #104
BerdirAm I the only one who thinks this issue is getting way too big ? :) Adding poormans-paragraphs *and* entity_usage in a single issue?
Did not actually review the patch yet, but it's 80+ kb changes, a lot of it on an existing stable module/entity type and I guess there is still a lot to do? If we do really want to go down that path, would it maybe make more sense to do this with a completely new, experimental module/entity type? I'd be much less worried about introducing such a huge change then and we would have a clean separation and not have to worry about existing views and all that.
Comment #105
johnwebdev CreditAttribution: johnwebdev commented#104
I think this part in the IS summarises it well:
...
I think most of the required changes are present in this patch now.
Yeah, it's not a bad idea. Introducing a new entity type would most likely make some issues easier to solve. For instance, we could have a parent id stored on that entity type and no longer need the entity_usage?
Though, if we want that new entity type to be re-usable as well, we are just implementing Block content again, but with the use case flipped. If not, a User might need to maintain both a Inline block and a Content block entity type doing the same thing, would that be reasonable?
Comment #106
BerdirFine if this is just a place to experiment, did not read the updated issue summary, sorry :)
Well, if they want to use a reusable block they can still use a standard block_content entity, because with reusable, the expectations on revisions are very different and while the UX might not be great when editing things inline (same with media and other similar use cases), it can't be by-reference-id because editing the block over the overview must work. That's what workspaces are for then.
Comment #107
tedbow@Berdir thanks for taking look again. Yes it is huge issue but I think it is still an experiment. Using a different entity might eliminate the need for entity tracking for instance.
For using a new entity type instead of block content:
Pros:
Having theThis would only work if we had ERRparent_id
on the newinline_content
entity type as base entity reference field would add possibilities for making relation ships via ViewsCons:
For the biggest win from a site builder perspective for using the existing "Block Content" entity type would be the same block types could be used in both situations. I assume this would be much easier for theming but that is out of experience area.
Maybe though if this is really a "poor man's paragraph" then having different block types will not be a drawback as this what people are use to with paragraphs.
Comment #108
hawkeye.twolfre:
and:
Is there some clever way of making a new entity type that actually just inherits the bundle configuration and theme hooks of another entity type? (and disable Field UI for the new entity type). That all might be confusing as hell for developers and a bastardization that we don't want in core, but the idea just occurred to me.
Comment #109
samuel.mortenson@derek.hawkeye.deraps That's basically how #2844054: Inline Revisionable Content Blocks works, sounds like we're getting back to the CTools approach after all this time.
Comment #110
DamienMcKennaI'd vote for allowing the same block entity bundles to be useable here, it feels like an arbitrary separation otherwise.
Comment #111
tedbowUpdating the title for now. Need to update the summary soon.
Comment #112
tedbowCreated a parent issue #2974864: [Meta] Determine best method of allowing inline content creation Layout Builder to discuss the differences between using Custom blocks, i.e, this issue and using a new entity type, a new issue #2974860: Allow inline content creation in layout builder through a single purpose entity type
Comment #113
tedbowThis patch creates
\Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockTestBase
which will be shared with #2974860: Allow inline content creation in layout builder through a single purpose entity type.This ensure we are testing the same functionality where appropriate. @see #2974864-7: [Meta] Determine best method of allowing inline content creation Layout Builder for reasoning
Comment #114
tedbowCopy & paste recursion error 🙀
Comment #117
tedbowin the comment 5 on #2974864-5: [Meta] Determine best method of allowing inline content creation Layout Builder I was worried about
I go on for another 5 paragraphs after... 😜
I am hoping we can actually handle this access problem.
This patch does
AccessDependentInterface
(andAccessDependentTrait
to implement it). This simply has getter and setter for an access dependee.InlineBlockContentBlock
andBlockContent
both implementAccessDependentInterface
\Drupal\layout_builder\EventSubscriber\BlockComponentRenderArray::onBuildRender
set call$block->setAccessDependee($entity)
passing on layout entity as a dependency.InlineBlockContentBlock::getEntity()
pass on dependency toBlockContent
::\Drupal\layout_builder\Plugin\Block\InlineBlockContentBlock::blockAccess
get called the block entity will have the dependency set\Drupal\layout_builder\Plugin\Block\InlineBlockContentBlock::build
this allows field blocks on Block entities one level down to still be rendered in the layout builder default. (i.e. Node entity -> Content Block -> field block body(checks entity access) 💥🧠)BlockContentAccessControlHandler
to check non-reusable block and checks the access on the dependee.Comment #119
tedbowComment #120
tedbowSo #117 stop you from being able to view non-reusable blocks but they will still should up as referenceable entities in entity reference field.
This is because:
EntityReferenceSelection
plugin\Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection::buildEntityQuery
does add the tag 'block_content_access' but the block_content module does not do anything with this because up to tell now this was not a problem. Blocks really didn't have access controlWe could add a
EntityReferenceSelection
plugin to the block_content module to overridebuildEntityQuery
but this would not effect any customEntityReferenceSelection
plugins that currently exist and are used with entity reference fields targeting block_content entities.Instead I am adding
block_content_query_block_content_access_alter
which:Checks for tags 'entity_query_block_content', 'entity_reference', 'block_content_access'(because of function name)
checks for table
block_content_field_data
checks for any conditions against
block_content_field_data.reusable
(nested also)If there are no conditions set against this field and all other conditions are true the it
$query->condition('block_content_field_data.reusable', TRUE);
This should stop the DefaultSelection plugin from returning any non-reusable block entities. It should also work against other
EntityReferenceSelection
plugins unless they are adding the proper tags in which we should not alter them.Comment #122
tedbow🙀
Fixed.
Doing interdiff against #117
Comment #124
tedbowWorking on big self-review patch
Comment #125
tedbowThis service to check route access is not no longer needed because the logic has been moved into the access controller.
checking all operations. This means you can't update non-reusable custom online blocks outside of layout builder.
This test was passing locally for me but not on DrupalCI. Seeing if this fixes it.
Implement the $limit logic which needed to actually be converted to use
::notExists
This is more efficient than having many separate update calls.
$account was never used after being set.
This wasn't actually showing. Fixed
I was getting random failures that I think was because there was no wait for the message.
Comment #127
tedbowWhoops logic error in
\Drupal\block_content\BlockContentAccessControlHandler::checkAccess
Fixed
Comment #129
tedbowThis patch does
InlineBlockTestBase
to use existing methodaddInlineBlockToLayout()
for all block placements.InlineBlockTestBase
to sue new methodconfigureInlineBlock()
for configuring existing blocksInlineBlockTestBase
EntityUsageTest
to not create actual entities since most method accept just IDs. Mock an entity when needed.\Drupal\block_content\BlockContentAccessControlHandler::checkAccess()
, no change in functionality.BlockContentEntityReferenceSelectionTest
to make sure the block is referenceable when change to reusable.Updated Issue summary.
Comment #130
tedbowComment #131
tedbowI created 2 child issues
#2976334: Allow Custom blocks to be set as non-reusable adding access restriction based on where it was used.
#2976366: Create generic entity usage tracking service
So here is a do-not-test patch with the other code stripped out.
Comment #134
tedbowIn [#2976366-] @phenaproxima rightly stressed how problematic entity usage tracking could be based on our experience with File usage.
See #2821423: Dealing with unexpected file deletion due to incorrect file usage.
but we may have need for Media usage tracking #2835840: Track media usage and present it to the site builder (in the media library, media view, on media deletion confirmation, etc.)
But that got me thinking:
So basically are use of tracking is not to determine if the entity is use still anywhere in the system. Each non-reusable block as used by the Layout Builder will only have 1 entity that is using it.
So this patch adds non-generic usage tracking only for this use case to the Layout Module itself.
Comment #136
tedbowWhoops not all the files got in the combined patch. the interdiff and do-not-test patches should be the same.
Comment #138
tedbowThis seems to fail on DrupalCI and randomly fail on my local.
I am not sure why. When I output the page when the message is not there after the wait then it is still on the layout builder page. But usually it passes.
I am changing this for now to manually drupalGet() the href of the link. This seems to fix the random failure for me.
It will at least let us see if the rest of the test passes. Anyone else has any ideas why this might happening?
Comment #140
tedbowThis patch removes all logic from
\Drupal\layout_builder\InlineBlockContentUsage
that does deal directly with the usage tracking.I added
\Drupal\layout_builder\EntityOperations
that is called from the entity crud hooks and cron. It calls the usage service.This seems cleaner.
Comment #142
tedbowTo get around this for now I am making a new revision of the inline block whenever block is edit in a layout and the parent entity supports revisions. If the layout is updated but a particular existing inline block is not edit then this will not cause a new revision. This makes
\Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockTestBase::testInlineBlocksRevisioning()
pass for me locally.Comment #144
tedbowThis was actually causing the last test failure.
The id length was too short and getting a truncate error.
Comment #145
tedbowSelf review, big interdiff but hopefully b/c simplifying.
re @Berdir in #14
If we want to add this functionality in the Block UI then we should create a follow up. The wording will be very important to not confuse users.
For now I have implemented
hook_plugin_filter_TYPE_alter
to disallow this new block plugin except when the $consumer is layout_builder.I have also removed the logic from
\Drupal\layout_builder\EntityOperations
that handle block config entities that used this plugin.Replace with EntityTypeInterface::ID_MAX_LENGTH,
Table name wrong
s/dependee/dependency
s/The lock/The block
I think it is safe to remove this base class because this issue seems more promising than #2974860: Allow inline content creation in layout builder through a single purpose entity type
This patch remove the need for the base class and simplifies
InlineBlockContentBlockTest
.Comment #146
johnwebdev CreditAttribution: johnwebdev commentedReview:
Block revisions => Block revision ID
Why do we use the phrasing object?
These can be chained in to a one line.
i.e.
Could we just add a simple comment to explain why we do this?
Is this class even overridable?
Can we make a comment to explain better what we're not targeting here, or break them up to more return conditionals to make it more clear?
Why do we need to check $this->storage?
I don't think we need @internal here?
Can this be a one-line?
I think we can opt-out early if there are no sections?
We should probably use same param type hint here.
Instead of loading all blocks, we could make a entity query instead
Comment #147
jibranThis should be post update hook.
Comment #148
tedbowre #146
@johndevman thanks for review!
1. fixed.
2. This was left from when it was generic usage. Changed also change for type. Using "parent entity" in both cases now instead of "entity using".
Maybe we just change this and column names from "parent entity" to "layout entity" since now it is very specific.....
Yes changing to 'layout_entity_id', 'layout_entity_type', and 'block_content_id'. This code started out as generic entity tracking usage table now it is specifically for the Layout Builder use case. Right now you can't use it out of layout builder.
If we decide to do #2979142: Determine if the Inline Block plugin should allow via the Block UI and other module other than Layout Builder then we won't need usage for that case because it will always be a 1-to-1 and we can delete the block_content entity when the block config entity is deleted.
3. I had this as online 'content_moderation' module but then I realized it is much hard to debug because in IDE(phpstorm in my case) cannot this as a "usage" of the methods.
i.e. I am at
\Drupal\layout_builder\EntityOperations::handlePreSave
and I try "Find usages" nothing will come up.Do we make code changes for ease of use in IDEs? I don't think this is PHPStorm specific case. I would vote for leaving it.
4. Create follow up #2979142: Determine if the Inline Block plugin should allow via the Block UI and other module other than Layout Builder and @todo.
5. Note sure what the question is? Should we make it "final". Think we almost never do that in core.
6. Add split up into 2 if statements with comments which allows having the 1st if before calling
$this->getEntitySections($entity)
This is hopefully more understandable plus more performant!
7. Because the constructor checks
$entityTypeManager->hasDefinition('block_content')
setting storage.We could just check before we call public methods here if 'block_content' module is enabled but I would rather have all the logic in the class if possible.
Maybe we should check if the module is enabled instead.
Added
isStorageAvailable()
helper function which as doc comment.8. We don't need it but it is more clear that according to API policy this should internal. Nobody should be extending this.
9. Fixed
10. This ops out of the rest of method.
$this->removeUnusedForEntityOnSave($entity);
actually should be called even if there are no sections because for a 'entity_view_display' entity you could have no sections but$entity->original
might have had them.11. fixed.
12. Nice catch!. Changing
getPluginBlockRevision()
togetPluginBlockId()
because it is only ever called where we need the ID.re #147
@jibran. You're right! Fixed in #2976334: Allow Custom blocks to be set as non-reusable adding access restriction based on where it was used. since the current issue is only for layout_builder changes. The combined patch I upload here, which is for testing, though will have the fix.
Will name the patches better.
Comment #149
tedbowThinking more about @johndevman's suggestion to not load is this method which had me change
getPluginBlockRevision()
togetPluginBlockId()
.Even though we aren't loading the block entity anymore when we don't need to, it is still calling
getPluginBlockId()
individually for each component. This means if there are 30 components there will be 30 entity queries each with their own alter calls.Instead it would be better to first get all the
revison_id
s from all components and then do 1 query with anIN
clause.So this made me change the method to:
But then....
I looked at where
getInlineBlockIdsInSections()
was being called inremoveUnusedForEntityOnSave()
if ($removed_ids = array_diff($this->getInlineBlockIdsInSections($original_sections), $this->getInlineBlockIdsInSections($sections))) {
This code is finding which block IDs are no longer used and so can be removed(if needed for revisions this code will not be called).
The problem is that 2 calls to
getInlineBlockIdsInSections()
will be made even if the revision ids of the original and current are exactly the same. So that would mean 2 entity queries withIN
clauses when there might not need to be any.So I changed this to:
The outer if will only pass if there are revisions in the original that are no longer in the current. Because it is just looking at the components it does not have to do any entity queries for this.
So in the case where the user only adds or edits inline blocks or adds/edits/deletes other block plugins then we saved ourselves 2 entity queries here.
The inner if statement with it's 2 calls to
getBlockIdsForRevisionIds()
still would only makes 2 entity queries.I think it is very important to think about performance with the layer builder because this is now more likely to be done with the frequency of content editing vs site building since no configuration is actually being edited here.
Although I changed the code around this we are still calling
deleteBlocksAndUsage()
when it is safe to delete block entities on save.But in
handleEntityDelete()
we don't delete the blocks entities but just remove the uses. On cron the actual block entities will be deleted.We do this because with hundreds of revisions we might have hundreds of inline blocks so it is not safe to do when just submitting a delete form. Or when deleting 10s of node entities via the content listing page.
My question is should we only be deleting the usage and not deleting the blocks in
removeUnusedForEntityOnSave()
. Will there ever be a situation that you are removing so many inline blocks on a save that it is not safe to delete them all.Probably it is ok the way it is. But interested if others have use cases where it won't be.
If it is safe the way it is should we then we also update
handleEntityDelete()
to delete all block entities not just usage iif the parent entity does not support revisions.I removed the cron call here because the block entities will actually be removed in
removeUnusedForEntityOnSave()
which does wait on cron.This is because it is non-revisionable entity.
Comment #150
twfahey CreditAttribution: twfahey as a volunteer and at University of Texas at Austin commentedHave been testing this out, and I am excited to see this new functionality. It is a really nice upgrade for LB!
Minor thing I noticed - Should we add an @todo in the commented out portion of the test:
Comment #151
tedbowI uploaded a big change to #2976334-41: Allow Custom blocks to be set as non-reusable adding access restriction based on where it was used. that change from adding a reusable flag to block content entities to adding the ability for blocks to have a parent set.
This allows this issue to get rid of the usage table for content blocks.
Since the blocks know their own parents it is easier find blocks who parents have been deleted.
It is little more complicated because we have to take into account that some entity types are not stored a database table. This is standard for
entity_view_display
entity type but also for when a site is setup to not store a certain entity type like node in database table.Comment #154
tedbowComment #155
tedbowBring up-to-date with #2976334-47: Allow Custom blocks to be set as non-reusable adding access restriction based on where it was used.
Move the block_content deletion logic into block_content module.
Comment #157
tim.plunkettThis is really not a problem at all, but I thought I'd point it out since it's a recent change that is nice to have.
These do the same thing now!
Comment #158
tedbow#157 @tim.plunkett thanks. Fixed
This patch brings up-to-date with #2976334-53: Allow Custom blocks to be set as non-reusable adding access restriction based on where it was used.
Comment #160
tedbowAccessDependentInterface
and add::setParent
actually are not going to work with layout builder access wise. At least I can't see how they would.I added failing test coverage to prove this
I added the comment in the code explaining which will copy here:
tl;dr; If an inline block is removed from node in previous revision the user can still pass 'view' access on the block if they access to the published revision.
I am leaning towards adding back
AccessDependentInterface
or at least adding this methods toBlockContentInterface
. Then maybe instead of havingreusable
as a base field adddependent
because I think this more descriptive.This would also mean this patch would have to be changed. Basically layout_builder would have call
setDependent()
during rendering process again. This seems to be much surer way to lock down access.The downside, and maybe it actually isn't a downside because access is much stricter, is that would couldn't render these blocks easily outside of layout builder because would have to call
setDependent()
.Comment #162
tedbowAssigning to myself. Need to bring back up-to-date with #2976334-57: Allow Custom blocks to be set as non-reusable adding access restriction based on where it was used. which re #160 switches from using blocks with parents back to the idea of reusable blocks.
#149 was the last patch here that was built update reusable blocks.
I don't think we want to revert back to that 149 because there has been other problems solved since then and at the very least we want to keep the better test coverage.
For quick test though going to test the layout_builder changes from #149 to make sure they still pass now that we have rolled block_content to use reusable blocks. In theory this should be green..
After this runs I will start back making sure we keep all the improvements since added between #149 and #160 if they still apply.
So it should go back to "needs work" after this test run regardless of whether the patch is green.
Comment #163
tedbowOk here the further improves after #149 that still apply after switching back to
isReusable()
.Comment #164
tedbow@Berdir mentioned in #2976334-54: Allow Custom blocks to be set as non-reusable adding access restriction based on where it was used. that we should be testing for private files on blocks
I have added test for access to private files on inline blocks in this patch.
Initially private files on inline blocks would always return access denied because
\Drupal\file\FileAccessControlHandler::checkAccess
checks if the user has access at least 1 entity where the file is used.Because non-reusable blocks need to have
::setDependency()
called for them to return access allowed then private file check on the inline block would always return access denied.So basically I needed to figure out a way that if
$block->access()
is called before::setDependency()
is called we automatically set the dependency to a revision of the layout node where the block entity is used.I am using an implementation of
hook_ENTITY_TYPE_acccess
to do this. Basically this hook is called before access handler is called so we can check if the access dependency has been set and set if it is not.Although this solution works there 2 concerns I would have with it:
\Drupal\Core\Entity\EntityAccessControlHandler::access
in that the hook must be invoked before handler itself checks access. But maybe that is logical assumption. Also that way it would break is to deny access when it maybe should have been allow not to give access when it maybe should have been denied.hook_ENTITY_TYPE_acccess
is way it may not have been attended for. Basically it is using as a chance to alter the entity before access is checked.The other options I see:
hook_ENTITY_TYPE_load()
but then we would be setting dependency on load when most of the time for inline blocks we will set it explicitly with the revision we want to check. File usage does not use revisions 😢\Drupal\Core\Access\AccessDependentTrait::getAccessDependency()
if the access dependency fire an event that would allow layout builder(or other modules using non-reusable blocks) to set the dependency.Comment #165
tedbowWhoops forgot the patch.
Comment #167
tedbowOk, this patch removes the use of
hook_ENTITY_TYPE_acccess
to ensure that there is an access dependency set.I updated
\Drupal\block_content\BlockContentAccessControlHandler::checkAccess
to fire a newBlockContentGetDependencyEvent
event if no dependency has been set on non-reusable block when$block->access()
has been called. I did this instead of inside\Drupal\Core\Access\AccessDependentTrait::getAccessDependency()
because if$block->getAccessDependency()
was called inside one of the subscribers reacting to the event then this would cause recursion. A subscriber might call$block->getAccessDependency()
to determine if a previously called subscriber had already set the dependency.This addresses the 2 concerns I mentioned in #164 about using
hook_ENTITY_TYPE_acccess
.I am just about upload patch to #2976334: Allow Custom blocks to be set as non-reusable adding access restriction based on where it was used. with the block_content module changes only.
Comment #169
tedbowThe fail in #167 was random test failure that I have been trying to figure out. I think I have got it now.
I have added wait to this patch that waits for the file link to show up on the file widget after uploading the file. This will ensure that the test doesn't try to close the dialog before the file properly uploaded.
Comment #170
tedbowUnassigning myself, will continue to work on this but also looking for reviews
Comment #171
sjerdoSome review comments:
typo: allowe => allowed
There seems to be some inconsistency in naming the Database Connection variable. In class SetInlineBlockDependency its named $database, in class InlineBlockContentUsage its named $connection. Current usage in core: 27x $database, 63x $connection. I suggest we use $connection as variable name.
Unnecessary empty line
Unnecessary empty line
This method is too complex. You could add early returns to remove the deep nesting. Also the code wrapped in the else-block is unnecessary wrapped in a block (since the if block contains return;)
I guess this 'quick fix' should be fixed in the correct way before committing this patch.
Unnecessary parenthesis around $form_state instanceof SubformStateInterface
Comment #172
tedbow@sjerdo Thanks for the review! Hope Dev Days is going well!
1. fixed
2 Good catch. I actually changed to
$database
because I checked other uses and it seems more recent modules are using this in core. Also it is more descriptive. But all the same now.3. fixed
4. fixed
5. fixed
6. Will see what I can do to get #2970801: If you add block then try to Revert the layout it doesn't revert fixed. leaving comment for now.
UPDATE: I cleaned up the patch in #2970801: If you add block then try to Revert the layout it doesn't revert and I now think it is correct fix. Be great to get reviews over there.
7. fixed.
I also changed
InlineBlockTestBase
to extendWebDriverTestBase
now that #2942900: Convert JavascriptTestBase Tests to use DrupalSelenium2Driver has been fixed! 🎉. I tested locally and seems to still work.I had to remove calls to
$assert_session->statusCodeEquals(403);
inInlineBlockPrivateFilesTest
because this isn't supported by the driver to$assert_session->pageTextContains('You are not authorized to access this page');
I think this fine because we are also testing that the contents of the file is not output.
Comment #174
tedbowThis patch does a couple things:
So this patch adds new test module that removes css animation for testing like we did in #2782915: Standardize the behavior of links when Outside In editing mode is enabled. I added a @todo to #2901792: Disable all animations in Javascript testing so we can just have 1 test module for all other modules or just remove animations for all JS testing.
Comment #176
tedbowThis patch
\Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockTestBase::assertSaveLayout()
change to usedrupalGet()
save the layout. UsingclickLink()
causes random test failures. I explored many options to fix this(I mostly did this in scratch issue #2977515: [ignore] Test Package manager core merge). I think this solution is fine but create a follow up #2984161: Convert Layout Builder Javascript tests to NightWatch because hopefully NightWatch won't have these random failures.EntityOperations
toInlineBlockEntityOperations
and updated the comment. I think this is clearer and avoids any unrelated operations need for Layout Builder going into this class in the future.\Drupal\layout_builder\InlineBlockEntityOperations::removeUnusedForEntityOnSave()
actually removes the usage rows in the case it deletes content block entities on save(for non-revisionable entities). Also added test coverage for this change.Update summary to not detail the API changes that are actually happening in #2976334: Allow Custom blocks to be set as non-reusable adding access restriction based on where it was used. but rather to just reference that issue.
Comment #177
tedbowWith the access changes introduced in #167 it means you can now call
$block_content->access()
and if the block doesn't have access dependency set then the dependency will be set automatically to a revision of the entity that using the block in a layout.\Drupal\block_content\BlockContentAccessControlHandler::checkAccess()
now calls$block_content->getAccessDependency()->access($operation, $account, TRUE);
. This means the access is controlled by access dependency which as the name suggests is good thing.But for Layout Builder this doesn't actually fit our use case. Only "view" access should actually be dependent on the "view" access to the dependency. For
update
anddelete
those should controlled by layout builder but effectively they should only be granted if you have access toconfigure any layout
permission.Basically even if you can edit a entity you don't have access to the configure the layout for that entity.
I have added an implementation of
hook_ENTITY_TYPE_access
to ensure this. I also added test coverage.Comment #179
tedbowChanging to need review and retesting. Not sure how the failure in
FormErrorHandlerQuickEditTest
could be relatedComment #180
tedbowJust updating patch with changes need to keep in sync with #2976334-68: Allow Custom blocks to be set as non-reusable adding access restriction based on where it was used.
Comment #181
EclipseGc CreditAttribution: EclipseGc at Acquia commentedEvent subscribers are always documented as a service anyway, so you should be able to inject their dependencies w/o using container injection interface.
is this suppose to be public?
getInBlock??? These methods and variables all have the word "revision" in them, but you're bailing on revisionable entities earlier, so per our discussion, these need renaming because this is about entities that aren't revisionable.
That's a feature. :-D
Seems like we can just use this variable w/o recalling the method.
I know this is all internal and you probably have other error checking at some point, but you're not actually looking at only PluginInspectionInterface objects here because they don't do what you need. You're looking at something more specific, and I'd be way more comfortable if there was a check for that.
Tim should confirm if this is future proof wrt his opt in/out patch.
Shouldn't this be inside the previous "if" statement? Otherwise we do it on every getEntity() call.
This is confusing to read because both situations are dealing with duplicate_block == TRUE && !empty($this->configuration['block_revision_id'], but they state it in really different ways when the only difference is whether or not empty($this->configuration['block_serialized']. If we could just nest an if statement instead of the if/elseif, I think it'd be way more readable.
I really like the direction of this patch and feel like it's pretty close to being where it needs to be. Honestly, most of my issues here are pretty nit-picky. I'd like to see them all solved, but there's nothing in here that's obviously broken in any way. Let's fix the bits and get this in.
Eclipse
Comment #182
Tim Bozeman CreditAttribution: Tim Bozeman at CyberSolution for Achieve Internet commentedComment #183
tedbow@EclipseGc thanks for the review!
I have changed the
DependentAccessInterface
back to having only 1 dependency see changes in #2976334-70: Allow Custom blocks to be set as non-reusable adding access restriction based on where it was used.1. fixed
2. Nope, fixed
3. In our discussion I forgot that we are bailing on revisionable layout entities. This revision_ids are actually the revision_ids of the block_content entities used in the layout. Leaving.
4. Thanks for the clarification removing the todo and updating the comment.
5. fixed.
6. Changed to use
InlineBlockContentBlock
as the parameter type.7. ok
8. fixed
9. yep that is confusing. thanks for pointing this out.
I have simplified it this. I think it more readable now.
Comment #184
AaronMcHaleHi all
I've been loosely following this issue for a while, just read the summary to familiarise myself with it again, and wanted to raise a couple of things:
Firstly, since these non-reusable blocks will only be accessible in the context of the Entity they were created in, does a side effect of this issue mean that once the patch is applied any block (including regular reusable custom blocks) will be editable from directly within the Layout Builder?
In using LB for a project this is something that could be useful if a Block is shared between a few Entities, where navigating to the Custom Block Library and having to find the block among lots of Blocks can be rather time consuming and annoying.
Secondly, as mentioned I've been using Layout Builder on a project and we have built up quite a collection of Custom Blocks, has anyone considered a migration path for existing Custom Blocks to allow site builders to easily migrate them to these new non-reusable Custom Blocks?
Apologies if these have already been discussed or covered in the past, as you can appreciate there are so many comments on this issue it would be hard to find what I'm looking for, and I also don't have enough time to test the patch right now so thought I'd just bring up these points in the event it hasn't been covered in the patch.
Thanks
-Aaron
Comment #185
tedbow@AaronMcHale thanks for taking a look.
Nope this will not effect reusable blocks which is all current blocks. The inline blocks and the existing custom blocks are 2 different plugins. It maybe useful to be able to be able to edit reusable blocks in there block plugin configuration form but that problem exists outside of layout builder as this would also be useful in the regular block UI. I believe 1 reason it isn't done is because it is hard to get across to users that they edit something that could effect on other parts of the site.
I believe that should be separate issue for Custom Block module. If the Custom Block module changed the configuration form to be able to edit the blocks this would take effect in both the layout builder and block UI regardless of this patch.
This would be a separate issue and I think handled better by contrib. Of course if you did this all the blocks that were the same entity would after migration all be separate entities.
Comment #186
tedbowBring up-to-date with #2976334-87: Allow Custom blocks to be set as non-reusable adding access restriction based on where it was used.
The big change for layout_builder is in
\Drupal\layout_builder\EventSubscriber\BlockComponentRenderArray::onBuildRender()
if we know we are know we are preview in the defaults layout builder instead passing the entity itself(which will be a sample entity) on as the dependency we pass a accessible object that will pass on view but fail on everything else.This eliminates the need to put special logic in
\Drupal\block_content\BlockContentAccessControlHandler::checkAccess
to allow access on preview.Comment #187
tedbowSelf review, while waiting for other reviews
This assumes that
$return_as_object
will always be called as TRUE in block access handler. Although this is true with update block access handler in #2976334: Allow Custom blocks to be set as non-reusable adding access restriction based on where it was used. we shouldn't assume that.Fixed
Which should check here that actually block revision id is used in a component. Some other module may have made a new revision of this block entity.
Fixed
Comment #188
tim.plunkettThis is huge for site builders.
Comment #189
tedbowwhoops didn't upload patch for #187 so this includes those changes also
Comment #190
johnwebdev CreditAttribution: johnwebdev at Kodamera AB commentedThis patch starts to look really good! Most nitpicks here.
The descriptions does not match.
This should be AccessResult::neutral()?
Do we have test coverage for this?
Nit: entity will BE returned
Nit missing a .
This could be shorten down to one line.
Nice with a todo for this..
we should really try to get this committed
Hmm, we are using three different terms for describing this concept. Can we try to decide for one instead? This kinda applies to the whole patch.
We should use fetchObject or fetchAssoc() instead, as per recommendation in the Drupal docs (https://www.drupal.org/docs/8/api/database-api/result-sets) also, I'm not sure if Drupal changes this, but I think you can change the default fetch mode which would break stuff.
Really like how this method has changed! It's much more clear what's going on.
Can we add a comment to explain what this really does?
Comment #191
tedbow@johndevman thanks for the review again!
1. fixed
2. fixed
3. The JS test cover this because otherwise the inline blocks would not render in defaults.
but I have also updated
BlockComponentRenderArrayTest
to cover blocks that implementRefinableDependentAccessInterface
4. fixed
5. fixed
6. fixed
7. 👍
8. yep
9. Good point.
Changing to use "inline blocks" whenever possible.
I still think it in some places it makes to use "block content" like parameters for
\Drupal\layout_builder\InlineBlockContentUsage::deleteUsage
and other parameters where we want to clear the ID or the entity itself's type.10. Changed to fetchObject()
11. Great!
12. updated the comment to reflect the service must be provided dynamically because it is dependent on another module.
This call is not needed
Comment #193
tim.plunkettComment #194
johnzzonSuper-grammar-nazi-nitpicks, sorry 🙈:
an instance
an inline block
an inline block
an inline block
an entity block.
Also, should this be "an inline block"?
Comment #195
tedbow@johnzzon thanks for the review! No worries, grammar is important.
1. This is actually in the
block_content
so this would be in #2976334: Allow Custom blocks to be set as non-reusable adding access restriction based on where it was used.2. Fixed
3. Fixed
4. fixed
5. yep change to "an inline block"
Also made a @todo for #2976334: Allow Custom blocks to be set as non-reusable adding access restriction based on where it was used.
Comment #196
tedbowJust a re-roll
Comment #197
tedbowself review after a few days. Saw a couple things to simplify
Renaming to
providerBlockTypes()
s/have been is/have been set is/
Changing to "Adds"
Changing to accept an Entity. This method is only called 1 place and we have the entity.
Changing to
$blockContentStorage
to be clear.Move this check into the first if statement of the this methods because we don't need to call
getEntitySections()
if we already know this is revisionable entity.Renaming to
$unused_original_block_content_ids
to be clear.Changing to
To have 1 less point where we have to know this implementation details.
in a previous refactoring I added
saveInlineBlockComponent
but somehow didn't replace this block with a call to the new method. Fixed.This can be replaced with
return array_pop($this->getBlockIdsForRevisionIds([$configuration['block_revision_id']]));
Since we already would have
getBlockIdsForRevisionIds()
.Changing to
$inline_block_components
to be more clearSimplifying to
foreach ($section->getComponents() as $component) {
Comment #199
tedbowSorry I should run tests again right before creating patch.
Comment #200
tedbowRe-roll after #2936358: Layout Builder should be opt-in per display (entity type/bundle/view mode)
Update: forgot that #2936358 actually means we have to update tests. Updating
Comment #202
tedbowUpdated the tests.
This was being set 6 places across 2 file so changed this a constant in the parent class.
Comment #203
tim.plunkettThe test coverage looks great! This patch is really close.
I don't think we need the other $entity->in_preview, the $event->inPreview() should cover it, yeah?
Is there no direct API for this? It seems weird to use the DB directly.
This (and probably a lot of others) should be @internal.
Doesn't seem to be used in this file
Idk that we usually have all these @throws for a non-API method, especially when they're not doing the throwing themselves. And when they are thrown directly, the @throws should explain when they are thrown. In this case I'd probably just drop these
Nit, the parts of the comment that are part of the @todo should be indented a further two spaces
The entity type ID check should be redundant with the instanceof check.
Furthermore, the instanceof should check for LayoutEntityDisplayInterface not the class itself
This should use the block plugin ID, not the class
More with these @throws
Comment #204
tedbow@tim.plunkett thanks for the review!!!!
1. Yeah that sounds right. fixed
2. There is not as far as I can tell. In the method description I pointed to this issue I created: #2986027: Move NodeStorageInterface::revisionIds on up to RevisionableStorageInterface.
Only
NodeStorageInterface
has this method but it should be able to added toRevisionableStorageInterface
3. Marking all new classes as internal in this. I don't see of any that should not be. Also not adding the message "Layout Builder is currently experimental and should only be leveraged....." because these classes should still be internal when the module is stable.
4. Sure it is. It is set in the constructor 😜. Just kidding fixed.
5. Fixed.
6. fixed
7. only checking for interface now and not the entity type id.
8. Ok removing the assignment and just checking
$component->getPlugin()->getPluginId() === 'inline_block_content'
9. fixed. Also removed all
@throws
from the tests(I knew I could make this patch smaller 😜)Comment #205
tim.plunkettThanks!
Two more tiny bits and this is good to go
This can just be
if ($component->getPluginId() === 'inline_block_content') {
No need to instantiate the plugin
No need now for the extra parentheses or the newline
Comment #206
tedbowOk fixed!
Comment #207
tim.plunkettGreat! Thanks @tedbow for the incredible effort on this issue.
Comment #211
tedbowWhoops! we need to be checking the base id here.
Fixing
also fixing the 2 codesniffer_fixes.patchissues from #208
Comment #212
tim.plunkettAhh, totally makes sense. Thanks!
Comment #213
tedbowSince Layout Builder is experimental this is still possible for 8.6 beta
Comment #214
alexpottI wonder if we need #2952390: Off-canvas styles override CKEditor's reset and theme first. Without that using this with the default config in Standard is a pretty messy experience. Plus there are problems with tooltip for text formats. See the screenshot in the issue summary of #2952390: Off-canvas styles override CKEditor's reset and theme. Testing via the UI reveals the patch works as expected. You're able to add blocks to a layout and they don't appear anyway else in the UI. Nice.
Through testing the UI I noticed a PHP notice:
Notice: Only variables should be passed by reference in Drupal\layout_builder\InlineBlockEntityOperations->getPluginBlockId() (line 193 of core/modules/layout_builder/src/InlineBlockEntityOperations.php).
Now going to do some code review...
Comment #215
alexpottIf we always envisage this being part of the layout_builder module I think we should prefix the service with
layout_builder.
.Also I think we should to make a decision about whether these are
inline block
orinline block content
things. For example, we have InlineBlockEntityOperations and InlineBBlockContentBlockTest and InlineBlockTestBase. There's alsoinline custom block
in the code and UI text. I think we should call them inline blocks everywhere and drop the content and custom as I think these have been used because we're extend the block content entity which using both these words.There's a double space here.
settings_tray has some pretty similar test code - we could open an issue to add a generic module to system/tests/modules so we don't repeat ourselves and others can benefit easily.
Comment #216
alexpottI don't think this provides that service - it registers an event listener that uses this service.
Do we need to check the plugin type here - just incase another block plugin type uses the same configuration key? Nope getInlineBlockComponents() does that for us - so this looks good.
Comment #217
tedbow@alexpott thanks for looking at the issue!
re: #214
Only variables should be passed by reference
errorI could see this being part of the block module after layout builder is stable if we determine these blocks should be able to added via the Block UI.
Leaving now for @tim.plunkett's feedback
We have a @todo here about this.
Since REST access can't call
\Drupal\block_content\Access\RefinableDependentAccessTrait::setAccessDependency
it would follow the same logic as the Private file access or any other time access is determined on non-reusable blocks. You would only have access if you had access the entity if is used in. This controlled by\Drupal\layout_builder\EventSubscriber\SetInlineBlockDependency
The block access handler
\Drupal\block_content\BlockContentAccessControlHandler::checkAccess()
is written you don't get access unlesssetAccessDependency()
has already been called or an AccessibleInterface is set in theBlockContentGetDependencyEvent
. so therefore you would never get accidental access to an non-reusable block.For access other than view
layout_builder_block_content_access()
ensures that you will not able to CREATE UPDATE or DELETE if the none reusable block is used by layout builder and the user doesn't haveconfigure any layout
permissionIt doesn't register the event listener if actually set the service definition in the container. Because we can't always register this service if block_content module is not installed.
Changed to
Sets the definition for inline block usage service.
Comment #218
alexpottI still think these things are not aligned. The comment makes me think you are setting the definition of the inline_block.usage service.
Comment #219
tedbow@alexpott yeah sorry I missed your point before. fixed.
Comment #220
tim.plunkettI disagree that #2952390: Off-canvas styles override CKEditor's reset and theme is a hard blocker for this. It predates this, is replicable in other ways aside from this, and this doesn't make things any worse.
#219 addresses all of #214/215/216, thanks @tedbow! And thanks @alexpott for the great review.
Comment #222
tedbowCrediting everyone who reviewed and help here
Also adding @mglaman because help on #2948064: Layout Builder should support inline creation of Custom Blocks using entity serialization where this code started
Will add other users from related issues per comment
Comment #224
tedbow@phenaproxima for help on #2948064 and #2976366: Create generic entity usage tracking service which ended up being specific tracking here
Comment #226
tedbow@mtodor also for #2948064: Layout Builder should support inline creation of Custom Blocks using entity serialization
Comment #228
tedbow@japerry, xjm, and @EclipseGc(already credited) for extensive help on #2948064: Layout Builder should support inline creation of Custom Blocks using entity serialization discussion to determine problems with that approach
Comment #230
tedbowComment #231
tedbowI think that is everyone from other issues. Thanks for everyone on this. Big thanks to @johndevman for the numerous reviews over last couple months!
Comment #232
alexpottDiscussed a couple of concerns in the layouts slack channel.
I had a concern that using this on per node layouts might cause problems eg. too much unstructured power, not included in search (turns out they are). @tim.plunkett, @tedbow pointed out that you can already use custom blocks here but this makes things much better because they don't pollute the main block UI and this solves a performance issue (see #2978102: Profile 1000s of custom non-reusable blocks for performance).
My other concern is the entity model - like getting a node via REST api, these blocks are very unconnected. @tim.plunkett pointed out again that this is already the case and @samuel.mortenson pointed me to #2942975: [PP-1] Expose Layout Builder data to REST and JSON:API.
So for me both of these concerns have been addressed.
Comment #233
alexpottCommitted and pushed 500403b458 to 8.7.x and 43187a5476 to 8.6.x. Thanks!
Backporting to 8.6.x as layout builder is experimental and this has been described as the killer feature so it's great to get it in front of people to experiment with in 8.6.x.
Comment #236
bkosborneThis is great, thanks for all the hard work everyone!
Comment #237
yanniboi CreditAttribution: yanniboi at FreelyGive commentedHi, this is amazing!! :) Great work!
I've just been playing with this and it seems that cron errors when there are no inline blocks that need removing.
I've posted a follow up issue #2992817: Layout builder cron errors when no cleanup required.
Please let me know if I am missing something...
Comment #238
xjmUnfortunately we had to revert this due to a critical regression. @tedbow has a fix and test coverage for it. We can reroll this and recommit it with the fix once ready.
This will need to wait until after the RC commit freeze ends, but I think it is okay to backport this between RC1 and 8.6.0's commit freeze, since the module is only beta.
Comment #239
tedbowThis commit had an access by-pass vulnerability for access to private files that are used inline blocks but not in the layout for current revision of the entity.
You can see this vulnerability by:
The explanation
\Drupal\file\FileAccessControlHandler::checkAccess()
will callfile_get_file_references()
this will return the block_content entity that file was attached to.file_get_file_references()
has a noted limitation in that will only get references for the current revision. But the file is attached to the latest revision of the block. Though the inline block is not in the layout for the latest revision of the node\Drupal\file\FileAccessControlHandler::checkAccess()
will attempt to determine if the user has access to the block_content entity by calling$referencing_entity->access('view', $account, TRUE)
\Drupal\block_content\BlockContentAccessControlHandler::checkAccess()
.setAccessDependency()
has not been called on this block theBlockContentGetDependencyEvent
will be called to allow a dependency to be set dynamically.\Drupal\layout_builder\EventSubscriber\SetInlineBlockDependency()
will respond to this event.\Drupal\layout_builder\EventSubscriber\SetInlineBlockDependency::getInlineBlockDependency()
will find return the first revision of the node that uses the inline block in the layout. This revision is loaded via$revision = $layout_entity_storage->loadRevision($revision_id);
BlockContentAccessControlHandler::checkAccess()
viaaccess()
access would be checked on that revision. That assumption is false. For all revisionable entities in core access checking does not take into account the revision of the objectaccess()
is called on.Access to revisions is only taken into account on the routing level for instance by nodes in
\Drupal\node\Access\NodeRevisionAccessCheck
Suggested solution
Change
\Drupal\layout_builder\EventSubscriber\SetInlineBlockDependency::getInlineBlockDependency()
to only check to see if the block is used in the latest revision of the entity after it is loaded via$layout_entity = $layout_entity_storage->load($layout_entity_info->layout_entity_id);
This would mean you would not have access to a private file unless it is on the layout for the latest revision of entity. This though is the current situation with file fields: see Private file download returns access denied, when file attached to node revision other than current. So this seems like reasonable behavior.
Comment #240
tedbowUn-assigning myself to get reviews
BTW we found this issue because #2937199: Track Layout override revisions on entities which support revisioning caused
InlineBlockPrivateFilesTest
to fail.Comment #241
tedbowHere is patch with the same as #239 except without the fix to
core/modules/layout_builder/src/EventSubscriber/SetInlineBlockDependency.php
The new assertion at the end of
\Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockPrivateFilesTest::testPrivateFiles()
should fail.Comment #242
tedbowAnd now I will actually upload the file I mentioned in #241
Comment #243
DamienMcKennaWhile it's frustrating that this was reverted, I appreciate that you all jumped on this so quickly. Thank you.
Comment #245
tedbow#242 had the expected failure
re #243 yep it is frustrating, but to good to get it fixed now.
Comment #246
tim.plunkettThat interdiff in #239 is hugely instructive. As is the comment! Kudos @tedbow
(d.o might not like an RTBC issue where the last patch is failing, might want to reup the committable version)
Comment #247
Gábor HojtsyComment #248
mpotter CreditAttribution: mpotter at Phase2 commentedHere is a patch that changes update_8001 to 8602 so that it runs after 8.6-rc1. Sorry I wasn't able to post an interdiff.
(Also, this patch is against the 8.6-dev version)
Comment #249
mpotter CreditAttribution: mpotter at Phase2 commentedHere is the interdiff. So this was re-rolled against 8.6.x dev so there are some more changes here that I'm not sure about.
Edited: Sorry for the typo in the filename.
Comment #250
tedbow@mpotter thanks for catching this. I have re-rolled the patch against 8.7.x
Comment #252
tim.plunkettThanks @mpotter!
Comment #253
tim.plunkettTo better clarify my comment in #246:
I have not directly worked on this patch. @tedbow and I worked through #239 together, and I can confirm that I:
The follow-up change in #250 was because another issue was committed in the interim that added another numbered hook_update_N().
Comment #255
webchickRock, thanks for the clarification, @tim.plunkett. That all looks sane to me. Also thanks to @tedbow for the detailed STR in #239; very helpful!
Since @xjm indicated in #238 that we could still commit this to 8.6 given it's kinda "grandfathered" in, and given this has been given a security-minded review, going to try recommittting this, since we're currently between code freezes.
Committed and pushed to 8.7; cherry-picked to 8.6. Hopefully second time's a charm! :D
Comment #257
webchickAhem.
Comment #258
webchickComment #259
R_H-L CreditAttribution: R_H-L commentedSmall issue I noted in testing this:
If you have something in your custom block with a file upload (such as an image field),
and the filename is long with no spaces, it pushes the right hand part of the file information box off the canvas, which also pushes the remove button off the screen.Edit: It looks like this remove button is pushed off the screen regardless of filename size
Looks like this is already being looked at in issue 2951547
Comment #260
alesr CreditAttribution: alesr commentedThis issue could be related to the fact that the only reference of "block_content_field_data.reusable" I found was in this issue and there were some changes committed to 8.6.x.
https://www.drupal.org/project/drupal/issues/2998096