Add reusable option to inline block creation

Updated attributions

Hello

tl;dr
Almost all points are fixed, I'm not a poet and the main issue here is to provide better descriptions of what is reusable block, all caveats of its usage, etc.

Determine if this is something we should tackle in core

Could someone clarify if we need this core? if not I can implement contrib solution.

UX Feedback from #2999491-88: Add reusable option to inline block creation: We felt that a checkbox on the block's edit form is the wrong place to change it from non-reusable to reusable. This is a change that cannot be undone. The usual way to handle that is not a warning in the description text for the checkbox: it is to use a confirmation form. There was strong consensus for this recommendation.
UX Feedback from #2999491-88: Add reusable option to inline block creation: Having the confirmation form should make it easier to convey the implications for access control: see #38. Describe the other implications in terms that make sense to the content editor: the block will appear in the custom block library; it can be used in more than one place.
UX Feedback from #2999491-88: Add reusable option to inline block creation: It will also be convenient to choose reusable or not when adding a new block. There are several options, and we do not have a strong recommendation. For example: two links, something like "Add reusable block" and "Add block for this page only". Or, after choosing "Create custom block", select reusable or not along with the block type.

Put all these points into one since they are all about the same. So I have reworked "Create custom block" form. It is now "two-step" form where in the first step the user can choose what type of block he/she wants to create. There is a "new" description that we will show for users of what is inline and reusable block, could someone review/improve it?

UX Feedback from #2999491-88: Add reusable option to inline block creation: We already have an irreversible action for a block: deleting it. We should use the same pattern for both. That is, add an item to the contextual links. Selecting the link triggers a confirmation form in the off-canvas sidebar.

Implemented contextual link as suggested in #88.

UX Feedback from #2999491-88: Add reusable option to inline block creation: At the same time, review the text for removing a block (from the layout). Perhaps we should have different text depending on whether the block is reusable.

As per this point, I think we should not change anything, since deletion of block does not delete block content entity itself. To delete the block-content user should go directly to the custom blocks library.

UX Feedback from #2999491-87: Add reusable option to inline block creation: Fix UX on status message.

Could not reproduce this.

UX question from #2999491-38: Add reusable option to inline block creation: I just wonder if people will understand if they add a block and check this box the content they add via the block will be available for others to see regardless of the state of the entity. So if you add on page and that page is draft meaning most users can't see the page, the reusable blocks will be immediately available to other users who have access to use custom blocks.

I added a warning but I think someone can add a better one.

UX question from #2999491-40: Add reusable option to inline block creation: is it possible for a user to find themselves in a situation where the site permissions are setup in such a way where they can edit a Layout Override of an Entity and can add non-reusable custom blocks through LB, but can't create site-wide reusable blocks through the Custom Block UI? If that is possible, how would the additions in this patch handle that, would the user still be able to create reusable custom blocks via LB?

I have removed "Create custom block" permission that was in patch before. As for me it's enough for user to have "Create and edit content blocks" permission. And if we are talking only about this permission it's not possible to reproduce behavior from the quote above.
If someone has suggestions why we should have second permission for reusable blocks I would like to hear them.

UX question from #2999491-80: Add reusable option to inline block creation: For this patch I simply suggest to have discard changes not act on re-usable blocks and clearly state so.

I also think that this is a bad idea to have a discard option for reusable blocks. I added a warning but I'd appreciate it if someone can come up with a better one.

Write tests

Since it's not clear if we want to have this in core, I didn't implement tests for it.

Here is "interdiff"

P.s. I messed a bit with branches in PR because I didn't know about new release strategy in core, just ignore the second branch.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The patch works as expected. We should probably allow the form display to be overridden, like in ✨ Use a different form mode when editing block content in an inline block Needs work . Attached patch implements something like that.

Tried to fix CCF in #112.

Another patch to try and fix some of the tests.

Whoops, pressButton should be called on $page.

The last submitted patch, 115: 2999491-115.patch, failed testing. View results →

Changing status needs review because it is still not clear if we want to have this functionality in the core. Could someone clarify this?

My two cents here are this feature is important for folks using Layout Builder to build composable content. I have seen a number of use cases where content editors want to reuse a block from page to page. I would vote for including this in the core functionality.

From reading 📌 Improve the page building experience Active definitely think this functionality is wanted in core.

I've fixed the failed tests in #115 ✨ Add reusable option to inline block creation Needs review .
Agree with @johnpicozzi. I would love to see this in core.

Made a small boo boo with phpcs error. Fixed that and adding an inter-diff with patch from 115 as well.

Hi there, we're running 10.1.6 and I tried the patches from #103 and #82, and neither worked for me. I was getting the same error as some others above:
Class "Drupal\layout_builder\LayoutReusableBlockDiscardChanges" not found

I see that the later patches / MR are against D11. Will this work with D10 at all or do we have to wait until D11 for this one? Thanks!

I have fixed Cannot redeclare method and removed duplicated code in core/modules/layout_builder/src/Element/LayoutBuilder.php

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

An update for my previous comment - I applied a patch based on this commit: https://git.drupalcode.org/issue/drupal-2999491/-/commit/1330eff91b12349... and that seemed to work great as it provides the required class LayoutReusableBlockDiscardChanges.php. I'm still testing it out and will report any bugs if I find any.

@codechefmarc Do you have a patch that applies cleanly against 10.2.1? I tried applying https://git.drupalcode.org/issue/drupal-2999491/-/commit/1330eff91b12349... and it failed.

#82 is the newest patch that is working for me after a re-roll against 10.2.x. I'm not sure if it's the mish-mash of patches and direct commits and jump straight to 11.x since then that are causing this confusion, but no combination I've tried will apply or work, even after attempted re-rolls. So if anyone has a working patch for 10.2.x, please upload it.

In the meantime, the attached patch is the only thing that works for me. Since #66 worked on 10.1.x, this is good enough for me, at least for now. If anyone can get the newer commits working on 10.2.x, please share it.

Just some friendly reminders
* This issue needs to target 11.x now
* Work should be done in the Merge request (which was created from comment #103, against D11).
* If needed, individuals can create their own patches from the MR (see the plain diff link under Issue fork above). But patch files should no longer be posted in the issue queue because this one is already very complex and hard to follow.
* The issue should be reviewed, but can't be marked RTBC due to pending items, see IS and Issue tags.

I'm hiding the patch files to help prevent some confusion going forward.

capysara → changed the visibility of the branch 2999491-add-reusable-option2 to hidden.

capysara → changed the visibility of the branch 11.x to hidden.

@capysara How do you recommend this should be handled by those of us who are already using older patches for versions that are no longer being targeted even though they are still current and supported? My team has been using a now three-year-old patch until recently for this functionality, which no longer applies after the 10.2.x updates. Throughout the many years I've worked with Drupal, this is this first time I've been told that this method of submitting patches for community use is not acceptable. A large number issues tracked by developers are many, many years old and this has long been the method used by the community, in my experience.

Do we need to re-roll patches like this and then self-host them even though doing so means others who may need it can't see it or are you suggesting that we commit changes to drupalcode even if we know they will never be merged just so we can share them? It makes sense to target 11, but that version won't be out for a while now, so if the rules have changed, I'd just like to make sure I am following them, while still providing solutions for those that need them in the meantime.

The current 11.x branch is Drupal 10.3, see https://www.drupal.org/about/core/blog/new-drupal-core-branching-scheme-... → ( And even the real 11.x is actually not that far out). In most cases, merge requests against 11.x also apply against the current stable version. That's also the case here, I just verified that https://git.drupalcode.org/project/drupal/-/merge_requests/4178/diffs.diff applies on 10.2.x for me with both git apply -3 and patch -p1.

@Berdir Okay, I see that now. That sort of makes sense, although it seems very confusing for the versions to not match.

In any case, I already did attempt to patch that 4178 diff you linked, but for me, it fails to apply to 10.2.0 (I had to do this upgrade before 10.2.1 for change control reasons). What would be the proper protocol in that situation?

The MR needs to be updated after 📌 Convert FieldTypeTest into a Kernel test Fixed .

carolpettirossi → closed merge request !6278

I solved the MR conflict for 11.x

However, we need an MR for 10.2.x as the diff from 4178 MR doesn't work with Drupal 10.2.x

I have added the re-rolled patch for Drupal version 10.2.x and tested it with Drupal 10.2.2. The patch applies cleanly.

@abhinesh:

As pointed out in Comment #129, it is confusing to have patches posted on an issue with a MR. But it is also helpful to have patches that apply to specific, tagged versions, as pointed out in Comment #133.

As a compromise, I think that any patches should indicate the version where they apply. Would you mind uploading a renamed version of your patch and hiding the existing one? Personally, I would use something like 2999491-141-10.2.2.patch

I guess you removed the conflicting change to core/modules/block_content/tests/src/Functional/Views/FieldTypeTest.php so that the patch applies to both 10.2.2 and 10.2.x. (See Comment #136.) That is worth mentioning.

@carolpettirossi:

Since you closed the new MR, I guess you already noticed that the current MR does apply to both 10.2.x and 11.x.

On my current project, we are updating from the patch attached to Comment #82. That patch adds the permission "create reusable blocks" (in the layout_builder module) and the more recent ones. We got errors until we removed that permission from user roles: see Permissions must exist → .

📌 Adjust Layout Builder permission checking for inline blocks once more granular block permissions exist Needs work is finally getting some attention. I am adding it as a related issue.

MR 4178 had conflicts from 📌 Abstract the shared parts of DefaultsEntityForm and OverridesEntityForm Fixed .

I rebased the feature branch on 11.x and resolved merge conflicts. This was challenging, since the MR included some duplicate commits and some that reverted changes from earlier commits. I may have made some mistakes when resolving merge conflicts. I decided to squash some commits so it will be easier to rebase the next time.

The commit "Fix phpcs reported issues" included changes to several files that had no other changes in the overall MR. I reverted those changes. That makes the MR simpler: not a big change to the number of lines, but it touches fewer files.

I did a quick test of basic functionality, adding one inline block and one reusable block.

Thank you for your work on this patch, however it no longer applies for me on Drupal 10.2.4. Using:

https://git.drupalcode.org/project/drupal/-/merge_requests/4178.diff

@JeffMattson:

Have you tried using the patch attached to Comment #141? It applied to Drupal 10.2.2, so it will probably also apply to 10.2.4.

Yes! The patch on Comment #141 works for 10.2.4. Thank you!

Patch #141 is not working on 10.1.8 so I have rebase the patch agained 10.1.8.

Attached will work for 10.1.x.
#141 will work for 10.2.x

Hello,

For information, there is on conflict with patch #4 from issue 3305126 ✨ Add bundle information to LayoutBuilder Postponed: needs info .

See composer install -vvv

Hunk #3 FAILED at 261.

1 out of 3 hunks FAILED -- saving rejects to file modules/layout_builder/src/Element/LayoutBuilder.php.rej

To solve it I had to apply first the #148 patch and then the patch from the other issue (by ordering patches in composer), thus the second one can apply successfully with an offset as it is declared as one continuous block.

I think it will be the same for #141 patch for 10.2.x

Grimreaper → changed the visibility of the branch 2999491-82-10-2 to hidden.

Grimreaper → changed the visibility of the branch 2999491-82-10-2 to hidden.

Grimreaper → changed the visibility of the branch 2999491-82-10-2 to active.

Grimreaper → closed merge request !7551

Hi,

On a project we had applied patch from comment 82 on Core 10.1

Patch from comment 141 has too much differences in terms of structure and the intermediary step to choose inline or reusable block provokes too many side effects on the project.

I am not sure if it is because of the hook_event_dispatcher modules that is triggered too early, but other modules (contrib and custom) altering the add and update blocks forms, may want to use the "getCurrentSection()" method and I had fatal errors because $this->delta was null at this moment. Commenting that out, I had then plenty of JS/ajax errors and no more time to that dig out.

So I took patch from comment 82 and then rebased it on 10.2.x, so the MR that I closed immediatly, just to generate the patch file.

Attaching the patch file for composer usage.

Can we figure out a way to not hardcode the form mode to 'edit'? I'm running into the same issue mentioned in ✨ Use a different form mode when editing block content in an inline block Needs work .

Howdy all,

Hopefully this is the place for this.
We're using 10.2.5 currently with the patch from #141 (Also tested the following with #157 as well).

There is a issue we're running into with making inline blocks into reusable blocks.
If an inline block is made reusable and placed on multiple pages, but the original page is deleted then the block becomes broken.
This is happening after the layout_builder_cron() is run. It seems to disregard whether the block has the 'reusable' column set to '1' and removes the block from the database.

Steps to recreate:
1. Create a page
2. Create an inline block (any type)
3. Save the layout
4. Set the inline block to reusable
5. Create a new page
6. Add the reusable block to the new page
7. Delete the original page
8. Run Cron
9. The reusable block should be broken with text similar to "This block is broken or missing. You may be missing content or you might need to enable the original module."
10. In recent log messages you should see the block is broken after the layout_builder_cron() has completed its execution.

This only happens with inline-to-reusable, so if a block is created as reusable from the start there is no issue.
It also only occurs once the original page that the block was created on is deleted. If you delete the block from the original page, but don't delete the page the error will not happen. But as soon as you delete that page, whether it has the block or not, the block will break.

We haven't been able to solve this yet and we're hoping others might test and see if it's happening in their environments as well.