🇳🇿New Zealand @danielveza

smustgrave → credited danielveza → .

griffynh → credited danielveza → .

Thanks for the kind words! I posted a very basic screencast in the XB channel in Drupal slack yesterday of the configuration form working.

It's a webm file which isn't supported by drupal.org apparently, so sharing the link: https://drupal.slack.com/archives/C072JMEPUS1/p1752030986447949

Note for the review that the code is still in a POC state. Functionally it's working well, but there is certainly some cleanup that needs to be done to make the code cleaner.

Taken a stab at implemented the suggestions from #22. Both forms are now being updated. Changes pushed & tests are green. Adding some screenshots of the new text and how it looks on the form. As per the suggestion in #22, I've also included the label of the entity (where possible), falling back to a more generic message if no entity context is available (Like when editing a default layout).

Updating the IS to clarfiy new scope for this issue

Ah thanks! I had the feeling there was an issue but either my search was bad or I skipped over it when I was looking at the related issues.

Still some discussion going on in that issue, so I'll postpone this for now and circle back once a descision is made on this and either un-postpone it or close it based on what is merged there.

For now, postponed on 📌 Bulk convert the remaining hooks to OOP Active

I really hate to be a pain, if its an easy swap can we please make the test Functional instead of FunctionalJs? Layout Builder uses FunctionalJs tests for so much, even when no JS is required. I'm trying to move all tests (where possible) to Functional tests so ideally we'd avoid adding too many more.

From a glance I don't see anything here that needs it to be a FunctionalJs test. If I'm incorrect I'm happy for this to be RTBC in it's current state.

danielveza → created an issue.

I've attached screenshots of this and ✨ Improve clarity of Layout Builder "Revert changes" confirmation form Active with some suggestions of how it currently looks and how it should potentially look, so it can be reviewed by the UX team and a choice on the question & description can be made for both the question & the description of both issues.

Similar to ✨ Improve clarity of Layout Builder "Revert changes" confirmation form Active , while prepping screenshots for this I don't think the approach currently taken in the code is correct. I was thinking that the description of the text had been updated, but it's actually the title. Attaching some screenshots of the form before/after applying the current MR, and a screenshot of an example of how it could look with overriding ::getDescription as well.

Current form in 11.x:

After applying MR:

Screenshot from my local, moving some of the text into the description (The title question still feels too long here):

Hmm. Prepping screenshots for this I don't think the approach taken in the code is correct.

This is how the form looks on 11.x

After applying the MR

This is wrong to me, when I reviewed it I thought the description text was updated. I think we should leave the question as it is in 11.x and update the description text instead by overriding ::getDescription with the text based on the decision of the UX team

Doing it on my local looks like this.

Thanks for jumping on this so quickly!

Reviewing the MR logs, the 7.3 version of the libraries are being installed, tests are passing. The warnings on phpcs & spellcheck look unrelated to this MR.

Ready for review.

danielveza → created an issue.

I think the existing text for this particular form is pretty terrible , I'd recommend we merge this so it can at least be improved without being stalled on discussions about the particular language tweaks.

The points raised in #16 are 100% valid, but my 2c would be to improve this now. The discussions can and should still continue in the other issue, but I don't want the text for this one to stay in it's current state if the other issue takes awhile to get in.

Steps to replicate was asked for over a year ago in #8, I also tried to replicate the issue as part of that and could not.

We've had no further responses & no other reports of the same issue discussed in the IS, so I'm closing this issue.

Thanks for raising the issue! If anyone else has this or a similar issue, please feel free to open a new issue.

Changes look good, match the IS and have test coverage. I think this is ready for RTBC.

+1 to the suggestion from @mark_fullmer.

Adding the novice tag since this should be a good first issue for an new contributor. If its not picked up by a novice or @libbna in a week or so I'll jump in and do it

I agree with @cilefen.

This is also an issue marked against 10.2, which is now out of active support. Can this be replicated with updating to the latest version of Drupal 11?

This makes sense to have. Done a review, code changes & test coverage look good. Happy to RTBC.

smustgrave → credited danielveza → .

+1. Been doing this in client code for a long time. I've been thinking about doing this in Layout Builder, so glad to see the concept being floated for all of core.

The DX is so much nicer rather than needing to remember or look up arbitrary strings when using/testing plugins.

Deprecations updated, tests are green

Updated the deprecations to 11.3.

In terms of testing this, there isn't a way to do it via the UI but you can create a PHP script that pretty much follows the test coverage in testNamePropertyDeprecation and run that with drush php:script

Something like

$user = new UserSession([
  'name' => 'test',
]);
// This should throw a deprecation
echo $user->name;
echo $user->getAccountName();
$user->foo = 'test';
echo $user->foo;
// This should throw an exception
echo $user->mail;

could we consider using the existing message Any unsaved changes to the layout will be lost. This action cannot be undone. instead?

In the Revert form, this message wouldn't be accurate. Changes to the layout have already been saved.

Reverting to default is more desctructive (but can be restored by reverting to a past revision) than discarding changes (Which is just clearning the tempstore), so I think a more verbose message makes sense here.

This issue has gone through a number of comprehensive reviews, the tests are green and all comments from my most recent review have been fixed or commented on.

I think this is ready to be in RTBC.

Left a review, mainly around the patterns for the meta:enum property and some small test questions

Opened 📌 Improve clarity of Layout Builder "Revert to defaults" confirmation form Active for the revert form. Agree we should handle it seperately.

This is RTBC from me. I think its much better.

danielveza → created an issue.

Good idea! Put in a suggestion.

I think we should look at RevertOverridesForm too, that might be even more confusing than this one

Changes look great. I'm happy with #49, it feels like the lowest impact solution for a situation that should be rare outside of tests.

I've popped one thought in about a hook_requirements implementation, so leaving in needs review for people to have a think about it. I don't feel strongly about this, if its decided we don't want it then this is ready to be RTBC

It would be good to get the 11.x MR up to date with these changes too, fixes should go there first :). Thanks!

I don't feel strongly about this one. I'm a soft +1 for ComponentType. In a ComponentSource (using it's current name), nothing is actually being sourced from anywhere. At this point it's handling the render and data transformation layers for use inside XB. This makes it feel more like a type to me, and I think @effulgentsia points at the bottom of the IS outline this in a really clean way that makes sense to me.

At the moment we decocate cores plugin providers (BlockManager & ComponentPluginManager) to create/update XB component entities from existing plugins. We don't currently use any particular term or interface for these, but to me this is what should be considered a Component Source. Components are being sourced from another system like block or SDC currently, and LB and paragraphs in the future.

Naming is hard!

This should only apply to SectionStorage plugins that DO NOT extend SectionStorageBase and instead implement the SectionStorageInterface.

I searched through Drupal contrib projects and can't find any examples. I agree that we should add CacheableDependencyInterface to SimpleConfigSectionStorage and remove the if. I'll start on that and see how tests look

Thanks for raising an issue!

I've tried to replicate this with 11.x and I'm unable to see this issue. Can you please try this with just the latest version of Drupal and update the steps to replicate if you're able to get this to happen again?

I've tested this on the latest version of 11.x and I'm unable to replicate this. Tested on many different screen sizes. Can you please test if this is still an issue and update the steps the replicate?

The original reporter mentioned in #9 that this can be closed and this currently still sits as a support request, so I'm going to close this issue.

If you have any thoughts about improving this, feel free to open a feature request and discussions can continue there.

Thanks!

It has been close to a year with no further updates on this one, and most of the recent comments talk about incompatibility with Gin. I'm marking this one as closed.

Please feel free to reopen or open a new issue if you can replicate this with just core.

Thanks!

Test coverage added and tests are green. I'd be interested to get a second set up eyes on this one. Mainly around this comment.

I'm a little iffy of how many places we are calling ::hasLayout. I wonder if we can put that in a central place instead.

No problem! Just thought I'd flag it so it didn't get blocked later when a committed reviewed this :)

Changes look good! And agree that since the work is already done, tests are passing and it's a test module that it's ok to do it all in one issue.

It's best not to RTBC issues that you've worked on and instead have another person review and mark it as RTBC.

In this case I've reviewed and I'm happy for this to be RTBC.

The only thoughts I had around this one is that it could be worth updating the title and/or description to make to clear that the existing hooks have also been split out into dedicated files. When I first reviewed the MR I was confused at the changes, since I only expected to see the functions in the .module file moved into hooks

It's best not to RTBC issues that you've worked on and instead have another person review and mark it as RTBC.

In this case I've reviewed and I'm happy for this to be RTBC.

I don't believe this is a Layout Builder issue, it's just that Layout builder is exposing this.

If I understand it correctly, the issue thats raised here is that too much sample text is being generated by textarea fields when placed in Layout Builders default layout. We can trace this down to either StringLongItem::generateSampleValue or TextItemBase::generateSampleValue (based on field type) that both call Random::paragraphs() without a parameter, which means it will generate 12 paragraphs of sample text.

As quietone mentioned, there isn't a lot of activity here to explore making this change at the moment, but just leaving this here as a reference point in case people are interested in exploring this change.

The patch changes from #64 need to be incorperated in the MR(s) listed as part of this issue.

I've added test coverage, and also fixed issues where this wasn't working on the default layout or on nodes that did not have overridden layouts (Yay for tests)

Lets see how the pipeline goes here. I'm a little iffy of how many places we are calling ->hasLayout. I wonder if we can put that in a central place instead.

This absolutely needs test coverage.

I haven't thoroughly looked into this issue yet, would it would to have a test module that provides a layout, add that layout to a page, then uninstall that module and verify everything still works as expected? If I get a chance I'll look into this.

greggles → credited danielveza → .

greggles → credited danielveza → .

greggles → credited danielveza → .

This has gone through multiple rounds of review, all threads have been resolved. I think it's ready to be RTBC.

MR is looking good. I've done a couple of code reviews and left a few minor comments. re: #37 I'm ok with the API change as outlined in the MR, but I'll leave the tag for now just in the case the other LB maintainers want to weigh in.

griffynh → credited danielveza → .

Made some progress on this today, assigning to me to wrap up

Just to keep this one moving along, I've started to update some of the tests, had a lot of failures before, lets see how this run goes

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

Addressed feedback, tests are green

Im pretty interested in following the ContentTemplate work, so I figured I'd make a start on this one based on the IS and the discussion from #2. I've pushed up a POC of the existing work I've done so far, which now adds config dependencies for fields to the content template entities. This has test coverage, but still additional work to be done around code cleanup and removing the legacy code from `ComponentTreeItemTest`

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

This has been partially covered above and may be part of the video from #19, but I just wanted to have my thoughts in written format from pain points experienced with Layout Builder and overrides. As flagged by this being a stable blocker so I'm happy this is on the radar, I think its something that XB needs to get right (to the best of our ability) from the start. I'm going to use Layout Builder terms in this comment but the same logic should apply to their XB equivalent.

Note that this is going to be a braindump and going to be a bit rambly.

Layout Builder is a fantastic tool, and the contrib community around has built tools to further support how it works. Where Layout Builder falls over however, is getting updates from the default template to nodes that have been overridden.

Lets say we have a content type with 5 sections in it's default template, and 10,000 nodes of that content type where most if not all have been overridden to add additional content on a per node basis. Now we need to add a new block to section 3 for legal or compliance reasons.

This new block is not going to appear on any of the overridden nodes, so we need to write a complex update hook to add it everywhere. However there are still problems here. We can't guarantee that section has not been deleted or moved on any of the overridden nodes, so we need to take that into account. Layout Builder sections don't have UUIDs ( ✨ Add UUID to sections Needs work , so we can't even look up the section during the update process with confidence. This is very flaky so we tend not to do it.

Because of this on some new projects I've started to create blocks that renders a collection of fields/blocks, so the Layout builder content now looks like this.

-- Top content (Collection of blocks in a single plugin)
-- Layout Builder content
-- Bottom content (Collection of blocks in a single plugin)

This means the editors lose some control over what parts of the page they can edit (Only the middle section), but allows us to add new content to the top or bottom blocks that will automatically apply to all nodes, regardless on if they're overriden or not.

This leads us to the second problem, how do we add new Sections to the default template and have them flow through to the overridden nodes in the correct place. Again this is a problem in Layout Builder and something that should be considered for XB.

TLDR; Updating overriden content in Layout Builder from the default template is painful. We should take the lessons we've learnt from Layout Builder and apply them to XB from the start.

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

Ah actually. While I was prepping the MR for this it wasn't as easy as I was initially thinking when I opened this because we can't override the click element, and other approaches I was thinking about wasn't as clean as I was hoping for.

Postponing this one so I can think about it a bit more.

danielveza → created an issue.

Feedback addressed and tests are green again

Changes look good, the only thing I noticed missing from the original patch were the changes to BasicAuthResourceTestTrait::getAuthenticationRequestOptions. Should they be included in this?

Other that that it looks good and is ready to be RTBC, just checking in on that one first.

Ran a test of this from the latest 11.x branch. I implemented validateConfigurationForm on the TwoColumnLayout class that set a error, and its displaying correctly.

This appears to not be an issue and it has been many years since the last update, so I'm closing this.

Thanks for raising an issue!

Layout builder in core is used specifically for managing the display of a page, there isn't support for the edit form. So I'm closing this as a won't fix.

Regarding the Drupal updates, there won't be the full module rewrites that was required like the Drupal 7 -> 8 updates. There tends to just be minor tweaks needed between major Drupal versions now to keep the modules up to date and compatible with the new versions of Drupal.

Drupal has a slack community → where questions like this can be asked if more information/help is needed :)

CR created: https://www.drupal.org/node/3513877 →

MR is up for this and green. Moving to review.

I imagine this will need a CR?

How would we want to handle the removal of this code in D12? Do we wait for this to be committed and open a follow up?

danielveza → created an issue.

Thanks for raising an issue!

I'm having a hard time understanding what would be required here. Could you please update the issue with specific steps to replicate & information about what you'd like to see and what the desired outcome would be?

If this is required on a per site basis it can be done with a form alter (See LB Claro for an example)

Since this can be done with standard drupal hooks & there hasn't been any updates since 8.6 this doesn't appear to be a highly requested feature. In my opinion this should be closed. Leaving it as postponed for the moment for any others to give thoughts

I agree with what @tim.plunkett has outlined in #18.

Since this is mainly an API module, most of the functionality that would be described in the 'uses' section is already covered in the existing 'about' section.

Other API type modules such as phppass, and the db driver modules also don't have a 'uses' section so there is precedent to excluding this.

Marking as postponed for others to give thoughts before closing.

This has had no updates since 8.6, is this one safe to be closed? Leaving as postponed for the moment for others to give thoughts

After 8 years and no updates since 8.6. I think we can recommend to close this now. Marking as postponed for the moment for others to give thoughts

jct321 → credited danielveza → .

Popped a review with some thoughts.

I think we need to decide the scope of this. Do we want to:

Allow specific attributes to be passed to the pass2 element.
OR
Allow the pass2 element to inherit the properties passed to the password_confirm element.

I'd vote for the 2nd, and maybe open a follow up for the first.

Whats others thoughts?

Done a review & there is still a couple of older unresolved threads

Could this please be updated to include the issue summary template, including steps to replicate and what should be seen

Looking again, I think this is a duplicate of 🐛 Formatters for empty fields do not render with layout builder enabled Needs work . I think this one can be closed and effort focused there

Hey @klu, could you please create a merge request from the patch that you've created against the 11.x branch?

There is some documentation here → about how to create merge requests for Drupal issues.

Thanks!

We asked for steps to replicate in #4, and this issue has now been open for 8 months without any further response, so I'm closing this issue.

Thanks for raising an issue, if you find a way to replicate this please feel free to open a new issue.

danielveza → created an issue.

I think personally I'm a -1 on this idea being part of XB core. It sounds like a good contrib module that could be added on top of XB. The IS raises 6 points that we get for free with nodes and others have been raised in comments on this issue. This is extra code & tests to maintain and increases technical debt.

I think XB should be built with node in mind from the start, otherwise we're losing functionality that we currently have with Layout Builder out of the box. Without node support, I think real world adoption will be slower and developers will have less confidence in adding XB to new sites.

For example, we were early adoptors of CKEditor5 in new sites because it was a drop in replacement for CKEditor4 and because of that we were able to catch and fix issues ( https://www.drupal.org/project/drupal/issues/3339400 🐛 Incorrect use of FormattableMarkup in logger messages Fixed , https://www.drupal.org/project/drupal/issues/3280602 🐛 Exceptions for CKEditor 5 plugin definitions containing wildcard tags when PHP is built with libxml 2.9.14 Fixed ) in CKEditor5 before it was stable.

We couldn't be early adoptors of this before it supports node, since we would need to run both Layout Builder and XB side-by-side, which doesn't feel worth it.

I understand that this is being built with the idea of rapid iteration and it's been said that supporting nodes out of the box will be done before the 1.0 release, it just feels like we should be fixing the node related issues as we go rather than kicking the can down the road and saying we'll figure it out later.

I've just hit this on a client site too. Attempting to track down if it's something we should actually fix here or if its a problem with custom code.

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

nuez → credited danielveza → .

it still allows contrib/custom code subclassing rather than copying, hence it still is a de facto API.

I can understand wanting to keep API scope small 👌.

The points you've made make sense to me and it does still achieve the goal of cleaning up the adapter code, so +1 that we merge it

I think this is also works, I just have some thoughts below around the original approach & questions about the new trait :)

Having said that, I prefered the original approach. I think it was a perfectly valid and having the final constructor would help alleviate some of the concerns around making the ImageAdapter non-final.

The new trait is also very specific, it works if the class ONLY needs EntityTypeManager otherwise you need to either mess around or remove the trait to inject other services.

Maybe if we go the trait route we narrow the scope a little and call it something like ImageAdapterTrait?

Left a couple of tiny comments on the MR.

The IS mentions updating CONTRIBUTING.md, I've checked it in the merge branch and I don't think anything needs to be updated :)

Confirming that this cannot be replicated, following the same steps I used when I originally created this issue. I think this can be closed.

I won't RTBC since I did the original work, but confirming that the changes from @omkar-pd look the same as my original changes just with the latest commits from HEAD pulled in

griffynh → credited danielveza → .