πŸ‡³πŸ‡ΏNew Zealand @DanielVeza

Brisbane, AU
Account created on 1 December 2015, about 8 years ago
#

Merge Requests

More

Recent comments

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

I've reverted the test coverage, you're correct that is was passing regardless. I didn't realise that waitForElementVisible doesn't throw an exception and I haven't found a good way to do a mouse down + hold

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

I have a couple of issues that I think are worth raising in regards to this:

✨ Prevent auto-adding new fields to LB layouts Needs review is in the exact same situation that @acbramley raised in #100. The new functionality is tested, the only part that is missing a test is the update hook, which is simply setting a new boolean config value to TRUE.

πŸ› Contextual links can be used to drag and drop Layout Blocks. Needs review is a one line fix on a 5+ year old issue. Testing the dragging Layout Builder blocks appears to be an existing issue because the existing tests around this simulate the element moving in the DOM rather than performing an actual drag.

Testing this particular issue requires us to be able to click and hold on an element. We don't have this in core and I can't think of a way to do this in a test without writing code that I feel like may be prone to random failures.

Considering this is a very old issue with a one liner fix that can be easily tested manually I think it may be a good candidate for this.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

DanielVeza β†’ created an issue.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Added some test coverage

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Hello! Thanks for raising an issue.

This doesn't feel like a bug to me, it feels more like a support request. For your example in the issue summary, you could override the template for that particluar piece of block content to add the class you need, or you may need to rework your CSS.

Drupal.org has a great support page β†’ where you can find forums that may be able to help you more than we can here.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Hey all,

I've been testing this with a fresh install of D11 and the block filter is working correctly in all of my tests.

Steps I've taken:

  1. Install fresh D11 standard site
  2. Enable LB & overrides on the article content type
  3. Create an article and go to the layout page for it
  4. Click add block
  5. Try filter by "ID", and verify the ID, Revision ID and User ID blocks are shown and all other blocks are filtered out.

Do people have other LB modules that may be causing issues here? For example I see that Layout Builder browser has had various issues β†’ related to this over the years.

Could we please get some steps to replicate with just Drupal core?

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Unless I'm mistaken, SortableTestTrait doesn't actually do the drag & drop, it simulates it so that wouldn't work.

However when you mousedown on a sortable element you a class gets added if it's draggable. I may be able to test the class gets added when you mousedown over a draggable element, and does not get added when you mousedown over the .contextual element.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

This is actually a really quick fix. The Sortable library has a filter param which can be used to exclude selectors from triggering the drag behaviour.

The issue here is around testing, I'm not sure with the current setup if we can add testing around this

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

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

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

This has been inactive for 3+ years with no updates or further requests. I don't mind this idea though. Most layouts I use are distinct enough to not need this but I can see how it could be useful. I'm not sure if it should live in Core or be a contrib module though.

This is pretty easy to set up in ConfigureSectionForm if we go for the core route, screenshot attached of a few options

Bumping this for visiblity so others can give opinions.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Hello! Thanks for rasing an issue.

I've tried to replicate this in D11 following the exact steps in the issue summary. Comments showed correctly on both new and existing nodes. Since this cannot be replicated I'm making this as closed.

If you're able to replicate this on a fresh install of D11 feel free to reopen and update the relication steps in the issue summary :)

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Hello! Thanks for raising an issue. I believe this should be working already with the permissions that are already used within Drupal

Can we please get some more detailed steps to replicate in the issue summary starting with a fresh install of D11?

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Hello! Thanks for raising an issue.

I've attempted to replicate this on a fresh install of Drupal core. I'm not able to replicate this so I'm closing this issue.

The replication steps I followed based on the IS:

  1. Install fresh D11 standard profile
  2. Enable LB
  3. Add LB to Articles & make it overridable
  4. Create a block called "Second block"
  5. Create a new Article and go to LB on it
  6. Verify both blocks are listed under "Create content block".
  7. Close the off canvas.
  8. In a seperate tab, update the label of "Second block" to "Second block (updated)"
  9. In the first tab, click "Create custom block" again
  10. Verify that the second blocks label is "Second block (Updated)"

If you're able to replicate this again on Drupal 11 feel free to reopen and list the new steps to replicate :)

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Hello! Thanks for raising an issue.

I've attempted multiple times to replicate this on a fresh install of Drupal core. I'm not able to replicate this so I'm closing this issue.

If you're able to replicate this again on Drupal 11 feel free to reopen and list the new steps to replicate :)

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Reverted the last commit. Since updating the default value and tests can hopefully be done in a follow up I'm moving this back into needs review

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Pushed up a new commit that fixes the deprecation error, this is ready for review

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Not sure if this would fly though. If a bunch of tests do break, maybe in the setup we just set this new configuration to true with a todo to a follow up to cleanup.

I'm a bit lost now. Having it FALSE by default then overriding every LB test to set it to TRUE feels significantly less clean to me then just maintaing the existing behaviour in this issue then changing the default in a follow up. Maybe I'm not understanding something.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

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

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Pushed a commit that sets the new configuration to FALSE for new sites. Lets see how the tests go

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

IS updated, CR created.

Setting this to needs review, there is a couple of open threads that could use some more discussion moving forward.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Flagging that we should add a change record for this too

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

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

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

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

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Tests are green, I've responded to the feedback. Happy to hear other options if people disagree. Ready for review :)

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Agreed with the the last feedback item. Pushed up a commit, back to NR

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

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

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Setting back to needs review. Resolved all threads except one, and left a comment about why that one may not work. Unless I'm misunderstanding something

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Pushed up a commit that fixes most of the MR feedback. This is the leftover item, need to look into it more.

I think you can use EntityContextDefinition::fromEntityTypeId('entity_view_display') here to simplify

PHPStorm doesn't like it. I wonder if static functions aren't allowed here

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Could we please get a stacktrace as well?

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

I've set up the SectionStorage Attribute and converted all existing plugins over to the Attribute. Tests green, Ready for review

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Would this be a duplicate of ✨ Export block UUID with custom Layout Builder blocks Needs work ?

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

@bendqh1 I'm not quite following why Layout should be there? If your example you're already in the Layout Builder screen right?

Or in your example does the custom block also have Layout builder enabled on it? So you're expecting the contextual links on a block to be something like:

  • Configure block
  • Configure block layout
  • Move block
  • Remove block
πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

MR feedback addressed, CR created [#3420954]

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Thanks for raising an issue!

Can we get some more details & steps to reproduce from a vanilla install of Drupal core? I'm not quite following what the issue is here.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Just did a review of a MR, had a couple of small nits and some questions around the deprecations

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

This came up as a daily target for the Bug smash initiative. I've checked out the code and the entity isn't validated before it's saved.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

There is also an existing module in contrib β†’ that you can use for this. No updates in 4+ years, should we close?

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

A reason for this being marked as internal was added to the codebase in #3041053: Mark Layout Builder as a stable module β†’ when LB was committed to core. That statement matches the thoughts in #3. No other updates on this one in 5+ years, shall we close?

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

I almost think this could be closed now that the other issue has been fixed. We no longer get a WSOD and the "Placeholder for the "Image" field" fallback text feels perfectly acceptable to me considering this is somewhat rare and had no updates for 5+ years?

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Opened a MR that adds a kernal test for each function in InlineBlockUsage. Ready for review :)

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

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

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

There hasn't been any updates or further requests for this for the last 4 years.

@alexpott was close to closing this in #6 and has provided an alternate approach to getting the config.

I'm going to mark this as closed, feel free to reopen if anyone disagrees.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

I feel like this one should be tagged as needing Usability review as it's a pretty big UI change for LB.

My 2c is that I'm not a big fan of the idea, I feel like having these links as contextual links hides them a bit too much. There is already quite a few contextual links on a page built with LB. Adding more to configure sections as well as blocks makes this messy in my eyes

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

This seems like a great idea! This issue has been quiet for over 4 years so just giving this a bump to see if others have thoughts on this.

I'm presonally leaning towards this being a great idea for a contrib project, I'm unsure if it should be in core.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Thanks for raising an issue!

As this has not been updated in over 4 years I'm going to close this ticket.

This could be achieved by adding a custom layout. The exact implementation would be site specific so I couldn't run through the exact way to do it. I would start at this documentation page that outlines how to create a custom layout :) https://www.drupal.org/docs/8/core/modules/layout-builder/theming-and-ex... β†’

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Pushed a new commit that addresses the MR feedback. Ready for review again :)

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

@gmoraleb it looks like your problem may be due to either the Panels or Panelizer modules. I would recommend going through the issue queue of those modules that seeing if there is an issue similar to what you're seeing.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Agreed with all the feedback. Pushed up a new commit to address them.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Only thing I'd say is we can just call it OverridableInterface instead of LayoutBuilderOverridableInterface as per the proposed resolution, but this is consistent with LayoutBuilderEnabledInterface

Yup - I had the same thoughts while developing this. I figured keeping it consistent with LayoutBuilderEnabledInterface was the right call, but I don't feel strongly either way

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

The code itself looks good, do we need a IS update before we can move this to RTBC since this is doing more than 2 strings now?

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Looks good to me, feedback has been addressed. I'm happy to mark this RTBC

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Just one small comment left unrsolved in the MR https://git.drupalcode.org/project/drupal/-/merge_requests/6440#note_261850.

Not changing the status in case that hasn't been done on purpose but I think it would be good for consistency :)

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Added a review, just had two tiny nit comments around formatting.

It might be out of scope for this issue, but I wonder if it's worth rewording some of the tests at some point so we don't need to add so many cspell ignores.

For example, "amphibius" isn't actually used for any form of testing. It's put in a comment body. We could just remove that from the comment then we wouldn't need the ignore.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Opened a MR that:

  1. Adds a new trait call LayoutBuilderEntityFormTrait
  2. Moves all the shared functionality from DefaultsEntityForm & OverridesEntityForm into the trait
πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

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

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Opened a MR for this change

  • Adds a new Interface LayoutBuilderOverridableInterface
  • Adds this new interface to DefaultsSectionStorageInterface & LayoutEntityDisplayInterface
  • Removes the overridable functions from DefaultsSectionStorageInterface & LayoutEntityDisplayInterface as they're now part of the interface
πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

MR feedback addressed, this looks good to me!

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

No further updates or requests for this in 8 months. This would be very difficult to implement safely for the reasons outlined in #3. This could be built as part of contrib.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

This has been PNMI for 6 months now with no further issue reports, and we have no steps to reproduce with just core. Closing this issue now, if someone can replicate this with just core feel free to reopen.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Reviewing the comments in this thread and having a look at some contrib, it looks like there is already a way to do this.

Comments from larowlan in #6 are correct in that you can just use hook_element_info_alter()

larowlan
But if you just want to attach them to layout builder you should use the hook_element_info_alter hook and add an extra process callback for the layout builder render element and attach it in that

larowlan
IE there already a hook

Example of this in Contrib

function epa_layouts_element_info_alter(array &$types) {
  if (isset($types['layout_builder'])) {
    $types['layout_builder']['#attached']['library'][] = 'epa_layouts/epa_layouts';
  }
}

I think this can be closed?

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

I feel like this is something that could be done as a contributed module rather than part of core itself. Unless I'm misunderstanding the IS I feel like it 99% of cases you wouldn't want any markup to render for empty fields

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Left a review, just a couple of small suggestions.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Hello! Thanks for raising an issue.

I've recommend looking for a theme that has options like this or creating your own layout to provide this option.

Documentation for building custom layouts can be found here β†’ . There is also a great support page β†’ that outlines places where you may be able to find more help.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

I'm going through and triaging LB issues. It would be good to get an update/some thoughts on this one. I'm leaning towards marking it as outdated based on #11

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

This appears to be a duplicate of πŸ› Drupal\Component\Plugin\Exception\MissingValueContextException: Required contexts without a value: entity in Drupal\Core\Plugin\Context\ContextHandler->applyContextMapping() (line 150 of web/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php Postponed: needs info - So I'm closing this one so that discussion can continue there.

However I also don't think this is an issue in Drupal core. This has popped up multiple times and it's always related to commerce products. πŸ› Product Variation is without context in view based listing of Products when using Layout builder for managing product display Needs review may be of more help to you :)

If you believe this isn't a duplicate or solved by the commerce issue please feel free to reopen

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

This appears to be a duplicate of πŸ› Drupal\Component\Plugin\Exception\MissingValueContextException: Required contexts without a value: entity in Drupal\Core\Plugin\Context\ContextHandler->applyContextMapping() (line 150 of web/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php Postponed: needs info - So I'm closing this one so that discussion can continue there.

However I also don't think this is an issue in Drupal core. This has popped up multiple times and it's always related to commerce products. πŸ› Product Variation is without context in view based listing of Products when using Layout builder for managing product display Needs review may be of more help to you :)

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Hello! Thanks for opening an issue.

This is a question that looks like it's for building a specific site and not a problem with Drupal itself so I'm going to mark this as closed.

There is a great support page on drupal.org where you may be able to ask someone for help. https://www.drupal.org/support β†’

Thanks!

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

I've been working on this one, tests are now green. I've done the following:

  • Introduced a new test trait for enabling LB in tests
  • Updated tests to enable LB via the API where possible.

I tried to keep the scope for this one small, we should open a follow up for converting the rest of the LB tests to use the new trait.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

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

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU
  /**
   * Retrieves the section storage object.
   *
   * @return \Drupal\layout_builder\SectionStorageInterface
   *   The section storage for the current form.
   */
  public function getSectionStorage() {
    return $this->sectionStorage;
  }

This code is used in 5x places with just a slightly different comment each time. It would be a small trait but it could be worth it to add a new trait that adds the property & the getSectionStorage function. What do people think?

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

There has been no updates or further requests for this from other people for 5 years, so I'm marking this is closed.

This could be implemented on a site potentially by preprocessing the block template and checking if the node type from context has Layout builder enabled. Then you could add an extra class.

Feel free to reopen if others think this should not be closed :)

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

I don't think this is something that should be in core and this issue has been quite for many years now so I'm marking as closed.

In terms of implementing this if someone comes across it in the future, something like this should work:

  • Use the Layout section classes module β†’
  • Configure the module to include a breakpoint field on the section with options like Mobile, Tablet, Desktop
  • Place a mobile and a desktop section on the layout, and use CSS to target the classes you've chosen with the Layout section classes module to hide the sections when the breakpoint does not apply

Thats a pretty oversimplified way of doing it and it could be done better once you go in and start building, but that should be a starting point :)

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

This is indeed caused by FileItem::generateSampleValue.

This can be confirmed by:

  1. Installing a standard site
  2. Enable Layout builder on the page CT
  3. Add a file field to the page CT
  4. Edit the default layout and confirm that the file has been created on the server.

Out of the box a file should only be created by viewing the default layout of a content type that has a file field on it. That shouldn't generate 50,000 files in a few hours. There may be something else at play here with the paragraph setup. Do the paragraphs have file fields?

This seems like it's working by design, the only thought I have after trying to replicate and having a look at the code, I wonder if the file thats created in ::generateSampleValue could be marked as a temporary file. That way the file will be garbage collected in the future.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

This is sounding like it could potentially be related to an issue in Commerce, not Drupal core.

πŸ› Product Variation is without context in view based listing of Products when using Layout builder for managing product display Needs review looks to be very similar to the IS, which also mentions editing a product layout.

@AlfTheCat could you please have a look at the issue above and see it it resolves your issue?

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Tested this on a large site with over 3000+ block content entities where I was running into OOM issues.

After saving a block and therefore having the Block definition caches cleared:

No patch: ~152MB memory usage
With patch: 76MB memory usage

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Is #47 still valid? If there is still no usages in core and this never worked it feels like we should deprecate and remove it

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Opened a MR to resolve this. We collate the libraries from each section rendered via the new LB and attach them to the Decoupled LB element.

Includes tests.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

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

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Fixing another D10 deprecation in TwigExtension

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Updating the patch to add 9.3 || 10 to the version requirements.

The updates use \Drupal::service('file_url_generator') which was introduced in 9.3, so I've set the minimum requirement to that.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

The code around this hasn't changed so I would assume this is still an issue.

Needs a failing test to verify the bug & need to check if the patch still applies to D11.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

Could the problem here be #3103015: Add Support for Layout Builder β†’ ? Menu items aren't fieldable in Core, so this could be an issue with the contrib module.

πŸ‡³πŸ‡ΏNew Zealand DanielVeza Brisbane, AU

I'm marking this as closed based on the comment in #8. It sounds like this was a site specific issue and not an issue in Core.

Feel free to reopen if someone disagrees :).

Production build https://api.contrib.social 0.61.6-2-g546bc20