If we want to use Titan embeddings v2 we need the MR from 🐛 Could not load the embeddings engine to get the dimensions. Please check the configuration.Error executing "InvokeModel" on "https://bedrock-runtime.us-east-1.amazonaws.com/model/amazon.titan-embed-text-v2%3A0/invoke"; Active
I've just run into this as well, it would be good to get this committed if we no longer need to support v1. Otherwise we might need to alter the if statements to check both v1 and v2.
I wonder if we can turn these into plugins or do something a little nicer than elseifs checking strings. But I think thats something to think about later.
Passing pipeline, now with update path & tests
Converted the patch from #65 to an MR. Lets see how the pipeline looks
danielveza → made their first commit to this issue’s fork.
The replicate module has support for cloning a node & it's Layout Builder layout to a new node.
I'm not sure how I feel about allowing the default layout to be created/updated from an overridden nodes layout. It feels risky.
Marking this as postponed to see if anyone has thoughts on if this is something that should be investigated before I close.
This has been open for over 7 years without any interest. The Layout Builder Browser contributed module has this functionality included.
Since this is possible with contrib and there has not been any interest shown in this, I'm closing this issue.
Thanks!
This has been open for 6+ years with no update, and it's indicated in #3 that it may be a site specific issue.
I'm going to mark this as closed. Feel free to reopen if you find a valid bug around this. Thanks!
This has been open for 6 years now with no real interest in implementation. I'm thinking we should close this and suggest it be built in contrib if people are interested in this. I'm not sure there is enough interest to justify adding this to core.
Marking as postponed for the moment for others to give thoughts before closing
Had one last look through the code, looks great! It would be fantastic to get this in. I constantly wish I had this when I add new fields to content types.
@oily Thanks for confirming!
I don't see any feedback comments on the PR, maybe you didn't publish the comments?
Regarding
'Implements hook_HOOK_NAME_WHATEVER_IT_MIGHT_BE' type docblocks seem to be always placed just above the hook attribute which itself is just above the function (i checked by grepping 'Implements hook_').
I assume you're talking about the hook placement with the __invoke function? If so, this is valid. See core.api.php which describes this scenario, and CronHook in the file module for an example of how this is already implemented in cores codebase :).
Moving back to RTBC for now. Feel free to move back to NW if you have additional feedback on the PR outside of that
@danwonac Are you able to provide any steps on how you could replicate this? Did you have any other contributed LB related modules on the site?
Reworked the approach from #54 to consolidate the autocomplete attribute down to a single value.
Rebased to the latest 11.x
danielveza → made their first commit to this issue’s fork.
Addressed feedback. Adjusted the test to it captured the previous failing behaviour better.
Pipeline is green. Back to needs review.
danielveza → made their first commit to this issue’s fork.
Hmm not as convinced on this after some more changes to get tests running. Got a passing PR up anyway. NavigationContentTopTest had a ~12% increase in speed with the new PR averaging out over 3 runs locally.
Had a bit of a play with this, just trying out an alternate approach. I don't think we need to run all the entity queries at all for getting the cache data. We already know the display we want to cache just by the entity & entity type ID
The only thing we might lose here by loading the entity directly is that the alter hook might not run. If thats an issue we can also run the alter hook here potentially 🤔
danielveza → made their first commit to this issue’s fork.
I don't have any issues with adding these methods if it helps to unblock contrib.
I think we should have a change record to highlight that they're now available. https://www.drupal.org/node/3243396 → could be used as a reference point. We should also update the IS to include the pluginId getter, since that is also being added
+1 on the approach from the latest MR. Looks like the newly added test coverage is failing to find the newly added ID though. I expect it will be a quick fix.
That line doesn't match in 11.x either. Can you please check if you have other core patches applied to the site? If so, one of the patches could be causing this.
Did a code review of this earlier, and the feedback has been addressed. I think if this is a contrib blocker it's worth committing this in it's current state, however I do think we should open an issue based on #39 to explore the underlying code around this and see if there is any generic improvements we can make.
Confirming this is an issue. Can be replicated with the latest version of the AI module. The help text states the field can be left empty, but the Completion plugin is marking the field as required if it's enabled. The text format can't be saved if the field is empty.
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.
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.