🇳🇿New Zealand @DanielVeza

Rewrote the test to be FunctionalJS that follows the unhappy path

No further updates have been made on this issue and it was unable to replicated in the last comment over 3 months ago, so I'm now marking this one as closed.

Thanks!

No further updates have been made on this issue and it was unable to replicated in the last comment over 3 months ago, so I'm now marking this one as closed.

Thanks!

As per #10, it appears that another issue has already provided the required functionality. There has been no further updates to this issue since the last comment over three months ago, so I'm now marking this as closed.

Thanks!

No further updates have been made on this issue and it was unable to replicated in the last comment over 3 months ago, so I'm now marking this one as closed. Thanks!

No further updates have been made on this issue since asking for steps in replicate in the last comment over 3 months ago, so I'm now marking this one as closed. Thanks!

If this one can be replicaated with just core or other people see this, feel free to reopen or open a follow up issue. Thanks!

No further updates have been made on this issue and it was unable to replicated in the last comment over 3 months ago, so I'm now marking this one as closed. Thanks!

No further updates have been made on this issue and it was unable to replicated in the last comment over 3 months ago, so I'm now marking this one as closed. Thanks!

This has been open for 5 years with no comments outside of the original post, and no further updates in 3 months since I put thoughts in, so I'm now marking this issue as closed.

Thanks!

No further updates have been provided in over 3 months since this was marked as Postponed (maintainer needs more info). This appears to have been improved since this issue was first opened as per the screenshot in #22, so I'm now marking this as closed.

Thanks!

Solutions for how to achieve this have been posted in this video and it has been over three months since the last update where this was marked as Postponed (maintainer needs more info).

Therefore I'm marking this one as closed. Please feel free to reopen or open a follow up ticket if you think there is still work to be done here.

Thanks!

This issue is now over 4 months old, and no new comments have been added since the contributed module suggestion in #8, so I'm now closing this one.

Thanks!

This issue has been open for over 3 months without a reply so I'm now closing this one.

Thanks!

Ah ok - I tracked this down.

This issue stems from the fact that /taxonomy/term/X is already a view page, that also embeds terms in the view header. The workaround is to disable the taxonomy term view, since it's not needed if you're using LB with a term page.

This feels like a pretty painful spider web to untangle, I wonder if we should add a warning when you enable LB on a vocab display to make sure that either the view is disabled or a filter has been added to the view so exclude this particular vocab

Confirming that I can replicate this on D11, so updating the version.

It's a strange one. 2x pagers render specfically when embedding a views block in LB, but only on taxonomy term pages. An additional mini pager is rendered along with the one that comes from the view.

If you embed a block through the block management on a term without Layout Builder, it works as expected. I changed theme and the issue still happened, so it doesn't appear to be a theme issue.

Spent a bit of time trying to track this down but couldn't manage it. Leaving these notes here to pick up later.

I agree with #25

Thanks for raising an issue! This would need test coverage before it is can be moved to RTBC.

I'm not entirely convinced it is a better UX than the existing setup, but I'd be interested in hearing others opinions on it too

I think asserting the class .block-system-breadcrumb-block is more robus

Don't personally feel strongly about this, can add it in along with the preview test check.

I also personally prefer the approach of adding a few assertions to an existing test

I disagree with this one for a couple of reasons

This issue is in the breadcrumbs block, not Layout Builder, so the test should live in the system module, not Layout Builder
Layout Builder tests are already doing too much in single files and need to be split, so I don't want to add more bloat.

Moving the block is vital to triggering the error, so I think the test needs to move the block.

I'm 50/50 on this one. FunctionalJS tests are slow and I don't think it's needed in this case. I fully understand writing a test that runs the exact steps to trigger the issue, but in this case I think we can be confident that the changes to the build function of the breadcrumbs block also resolve the dragging issue, and they're being tested in this issue.

That being said, I don't feel strongly about this one and I can see the other side of the argument for sure. If someone would want to rewrite it as a FunctionalJS test I wouldn't block it

Tests on MR is green now.

Adding an entirely new BreadcrumbBuilder for this block feels excessive. I propose we simply check if the block is in preview and display the fallback text if it is. This was suggested in #19 and that issue has now landed so this is possible.

Added a MR with this approach with test coverage.

DanielVeza → made their first commit to this issue’s fork.

Thanks for raising an issue!

Can this be replicated without the contributed module? If not could we please get a backtrace? It may help identify if this is something that should be fixed in core or in lb_plus.

Ran the tests only feature since I couldn't replicate this in #9. Test fail as the steps to replicate have described, so I expect that was an issue in my local testing.

Code changes look good, I was debating if we should add a $this->getRequest()->query->has('destination') check around it, but since no other processing is being done to the destination I think it's safe to just run the remove.

$this->getRequest()->query->remove('destination'); does an unset of the param passed, if it doesn't exist there should be no issue.

This may be resolved by 🐛 Add missing category to Drupal\layout_builder\Plugin\Layout\BlankLayout and let modules and themes alter the list of layouts RTBC .

Since this needs a contrib module to be able to replicate this, I think it would be good to understand why this is happening with LB plus so we can define if the issue exists in that module or something that should be fixed specifically in core. Just casting things to strings feels fishy to me.

layout_builder.settings was removed in 📌 Replace "Expose all fields as blocks to Layout Builder" configuration with feature flag Active . As far as I understand this is no longer required, so I'm closing this. Please feel free to reopen if I've made a mistake with that.

Thanks!

larowlan → credited DanielVeza → .

DanielVeza → created an issue.

griffynh → credited DanielVeza → .

DanielVeza → created an issue.

Hello! Thank you for raising a very detailed issue.

Could you please confirm that the steps to reproduce in the IS are with a fresh install of Drupal 11? If not can they be updated with the steps to reproduce starting with a fresh install of D11?

I'm asking because I've tried to this and I've been unable to replicate this. The steps I've followed are below:

• Fresh install of a standard D11 site
• Enabled LB
• Added Layout builder to articles
• Added a text field to articles with a default value of "Default text"
• Created a new article and removed the default value from the field
• Went to LB for the article and added the fieldblock for the new field.
• I'm not able to see the label as described in the IS

Thanks!

string|\Stringable makes sense to me and I agree with the comments in the remaining tasks section of the IS.

Tests are still green, no linting issues introduced. Looks good!

griffynh → credited DanielVeza → .

DanielVeza → created an issue. See original summary → .

smustgrave → credited DanielVeza → .

DanielVeza → created an issue.

We are reviewing this again as part of the Bug Smash Initiative. This has had no updates in over a year and earlier reviews suggest that this has been fixed by other issues, so I'm now marking this issue as closed.

Unless I'm looking at the wrong place, the patches in #5 & #10 both take different approaches. I think we need some clarity here about which approach is the correct one and sync them up.

I totally understand the thoughts behind this, personally I'm not 100% sure about this idea. This seems like it would be highly disruptive to both contrib and to anyone that has applied custom styling to LB.

I think this could live in contrib so it can be an opt in for people who want it.

Thank you all! I'm happy to join the team :)

This has been inactive for 3 months since the last comment that asked for more replication steps. I'm marking this as closed. Thanks!

griffynh → credited DanielVeza → .

I was concerned the new approach could stop the message from showing when we do want it to display, but I've checked all child classes of EntityDisplayFormBase and the only one that sets redirects in the form state is LayoutBuilderEntityViewDisplayForm.

I prefer the approach taken here to my original MR & it's flexible enough to be used in the future if something like this pops up again. So +1 from me

Confirming that I'm still interested in being a maintainer and that I've read the required documentation.

Thank you for the kind words and support!

tim.plunkett → credited DanielVeza → .

No further updates in 3 months, examples have been provided on how to do this. I'm now marking this issue as closed.

Thanks!

Correct screenshot 🤦

I've tested this on a fresh install of Drupal 11 and this issue doesn't not exist.

Since this issue is over 3 years old and the issue appears to be resolved, I'm marking this as closed.

The steps to replicate this in the IS & in #3 both involve the use of Layout Builder Restrictions.

Can we please get some steps to replicate this with a fresh install of D11 and no contributed modules?

Thanks for raising an issue! In my opinion this is a duplicate issue of 🐛 System Menu blocks do not have attributes Needs work . That issue has an open MR and some further progress, so I'm closing this one as a duplicate.

Thanks!

This issue is now over 3 years old with no updates.

Can we please get some steps to replicate this starting with a fresh install of Drupal 11. If no further updates are made to this ticket in 3 months this will be a candidate to be closed.

Thanks for raising an issue!

This is postponed on ✨ [PP-1] Reorder Layout Builder sections Postponed

Can we get some more information on what the desired outcome is here please?

I'm marking this as postponed and it will be a candidate to be closed after 3 months.

Thanks so much!

I've rerolled the patch and pushed that straight to the MR

DanielVeza → made their first commit to this issue’s fork.

Needs to be an MR

Some tests would be good here too.

Based on @longwave's comment in #27 I've built a POC of an alternate approach that prevents the message from being added to the messenger().

This is a more generic approach that other forms could use in the future if this comes up again in other places in core.

This is failing tests at the moment, they need to be passing before this can be RTBC

This can check if the current route is using layout builder with code like this stripped back example

$parameters = $this->routeMatch->getParameters();
if ($parameters->has('section_storage')) {
  if ($parameters->get('section_storage') instanceof SectionStorageInterface) {
    return TRUE;
  }
return FALSE;
}

Then in a preprocess you can add a class if that returns TRUE.

I'm -1 of building something like this into core, I think it's something that can be quite easily added on a site by site basis, or it could be a contrib module.

Postponing for others to give thoughts

With #3027653: Allow block and layout plugins to determine if they are being previewed → being committed, is this issue still required?

Hello!

This issue is a duplicate of #2976152: Don't allow placing a Custom Block in the layout override for itself or a default layout → which also has a lot of discussion around the history of this and some working patches. I recommend we keep working moving forward there, so I'm marking this one as closed.

Thanks for raising an issue!

Updated the approach based on the feedback from @mstrelan

Work was done on making this test more stable in #3268680: [random test failure] Restore and fix LayoutBuilderDisableInteractionsTest::testFormsLinksDisabled() →

In my opinion we can close this issue now. Postponing so others can give their thoughts or report that they're still seeing random fails here.

I have not been able to replicate this with a fresh install of D11.

Can we please get steps to reproduce this issue starting with a fresh install of D11?

If steps have not been provided this issue will be a candidate to be closed after 3 months.

Thanks for raising an issue!

This was is quite tough to debug and feels like it may be an issue with the particlar site.

Out of the box Core has no way of attaching Layout Builder to Menu items so I expect some custom work was done to support that. @maxstarkenburg suggests one way that this may have been done.

I think to progress this any further we would need steps to replicate from a fresh install of D11. I suspect that 🐛 Layout Builder cannot be uninstalled while overrides exist; no easy way to revert all overrides Needs work will mostly fix the issues that have surfaced on this particular site.

Thanks for raising an issue!

This is a duplicate of 🐛 Empty layout sections get rendered Needs work and that issue has made more progress than this one, so I'm marking this as closed.

I'm unable to replicate this issue on a fresh install of D11.

Steps I attempted:

1. Install a fresh D11 standard site
2. Enable LB
3. Add LB to the default view mode of articles
4. Added a field called field_test
5. Added this field to the teaser layout
6. Created a node with content in the field_test field
7. Ran the code below in a debugger
8. Verified that the correct values were in the render array.

Code ran for testing:

$node = \Drupal\node\Entity\Node::load(1);
      $field = $node->field_test;
      $viewBuilder = \Drupal::entityTypeManager()->getViewBuilder('node');
      $renderArray = $viewBuilder->viewField($field, 'teaser');

Marking this as postponed as we need steps to reproduce. If this issue isn't updated with steps to reproduce after 3 months it will be a candidate to be closed.

Thanks for raising an issue!

Can we please get this converted to a MR?

We also need tests. @danflanagan8 added some in #27 but it appears they aren't in the latest patch. That may be a place to start?

On a fresh D11 install I'm also unable to replicate this.

Something else to flag is that on /admin/content on a fresh D11 install there is no layout action out of the box. So there may be a custom or contributed module that is interferring here.

Since steps to replicate were asked for over 3 months ago and nothing has been provided I'm marking this issue as closed. If you're able to provide steps to replicate please feel free to reopen or add a new issue.

Thanks!

@mstrelan raised a clever alternate approach on the MR. Setting back to NW for that

This makes sense to me.

Rather than using a hook, is it possible to set the new admin label in Drupal\layout_builder\Form\AddBlockForm::buildForm instead?

This will also need test coverage of the new label coming through. It seems like overkill to add an entire new test case for this, so maybe an assert for the new admin label could be added to LayoutBuilderTest::testFieldBlockLabel?

Opened a MR to strop the message from showing on the Disable Layout Builder Form.

There was no specific test coverage for the disable form itself, rather than add a whole new test for this I added coverage to an existing test that was already asserting LB messages.

I'd pretty interested in expanding this out to Contrib as well, so I'm not sure if we should be removing the Core package.

This might be expanding the scope of this a little too much, but what if we added another key for feature flags to the info.yml? Then in the modules page in a follow up we could filter them out based on the new key and have them in their own dedicated section?

Not something I feel super strongly about, especially if it will delay the issue moving forward. Just trying to think of options for both Core and Contrib.

This has had no further updates for 2+ years, so I'm closing this issue.

If this is something you're interested in following up on, I recommend writien up a detailed question about what you trying to achieve and using the excellent support page → on Drupal.org to find people who may be able to help

Thanks for raising an issue!

Thanks for raising an issue! Work around this is being done in multiple issues to try make this better, so I'm closing this one as a duplicate.

🌱 [META] Layout builder editorial improvements Active
🐛 Reduce the number of field blocks created for entities (possibly to zero) Needs review

In my opinion, this could be moved into LayoutEntityHelperTrait and made protected.

Thanks for raising an issue!

I can see that Page manager has some Layout Support but it appears that panels does not.

In terms of getting help with migration and general support around this, I'd recommend going to Drupal.orgs excellent support page → .

Thanks for raising an issue!

I am not able to replicate this on a fresh install of D11.

Replication steps attempted:

1. Install a fresh D11 standard site
2. Enable LB
3. Enable LB for the user profile
4. Update the picture field for the user field so that alt text is required
5. Add an image to a user profile
6. Visit the user profile and verify that the alt does exist.

I'm marking this as postponed to give others the chance to try to provide steps to replicate on a fresh install of D11, otherwise this ticket will be closed after 3 months.

Thanks!

Tests green again

This was postponed on 📌 Improve StringItem::generateSampleValue() Fixed . That has now been commited and this is no longer an issue.

I've tested this on a fresh D11 site and the sample text no longer overflows. See screenshot below.

So I'm now marking this as closed. Thanks everyone!

Thanks for raising an issue!

As Drupal 9 went EOL on November 1st, 2023 I'm marking this one as closed.

If you can replicate this on supported versions of Drupal please feel free to open a new issue.

Thanks!

This has been open for close to 18 months with no additional thoughts given.

I believe this could be built with views integration based on entities layout_builder__layout field (This may exist already). I'm -1 on having this as part of core, I think this would be a great candidate for a contrib project that can be required by the sites that want it.

Keeping it open but marking as postponed for others to give their thoughts

I did have the thought while I was doing this that it was a little odd to be writing a specific override for one entity type.

I don't have the historical context on why OverridesSectionStorage::buildRoutes doesn't set _admin_route to FALSE, so I'm unsure if thats an oversight or a choice that was made.

Either way, I've reworked the solution to disable the _admin_route in OverridesSectionStorage::buildRoutes and renamed the test to be more generic.

I agree with @mikelutz in #3. That remains accurate today so I am also -1 on this idea. This would be a lot of work, BC/updates to existing sites would be tricky and I don't believe the system would be better because of it.

@tim.plunkett pointed out a contrib module that may help resolve some of the pain points outlined in this issue, so this may belong something that remains in the contrib space rather than core.

Postponing so that others can provide their thoughts

There hasn't been any further requests for this since 2019, and from what I can see other modules in core don't provide a common class for their confirm forms.

Considering this custom generic class can be added in on a site basis with a form_alter I think I'm -1 for this.

Leaving open so others can put their thoughts in

Can we get some more information about the use-case behind this? :)

Is this something we still want to do moving forward? Nighwatch doesn't appear to be used very heavily in core tests at the moment and the existing FunctionalJS tests seem to be performing well. I'm not aware of any recent random fails, but I could be missing something

This has been marked as postponed for over 12 months with no further updates, so I'm now closing this issue.

If somebody is able to replicate this with a fresh install of Drupal please feel free to open a new issue.

Thanks!

makes it quite confusing as it gives the impression that other changes are saved automatically. It could lead people to wonder why they don't get that message when they change a select list item or type in CKEditor, is that saved automatically?

Adding another screenshot that I feel illustrates this point better

Coming at this from a Layout Builder point of view.

In my opinion we should hide the "You have unsaved changes" from components using the tabledrag library when we are in the Layout Builder context. It doesn't really add any value and makes it quite confusing as it gives the impression that other changes are saved automatically. It could lead people to wonder why they don't get that message when they change a select list item or type in CKEditor, is that saved automatically?

In terms of hiding this, adding .layout-builder-configure-block .tabledrag-changed-warning { display: none } in Layout Builder should be enough to hide instances of this, if people agree with my points above.

🤦‍♂️ Renamed the test file

No major updates to this issue since 2019. 7 months ago the issue could not be reproduced and there has been no further updates or replication steps added.

Because of that I'm marking the issue as closed. Feel free to reopen or open a new issue if this can be replicated in the future. Thanks!

MR feedback addressed, IS updated.

Ready for review again

Got a basic MR up. Still need to think about how tests work when you need a require a module outside of the ecosystem.

DanielVeza → created an issue.

Ah yes you're correct. We check if that route exists before we do anything. That esentially acts as a moduleExists('block_content') anyway :)

Verified from manual testing that this is still an issue. Updating the IS and added steps to replicate.