- π¬π§United Kingdom catch
Ideally I think we would take a different approach to this overall problem, although even if we do that we would probably want something similar to the existing patch too. I don't know exactly how to do that in terms of one or two issues and whether there's any dependencies between them, so just trying to write it down for now.
I think (and I think @alexpott has said this somewhere) that instead of using derivers at all, we could have a single 'entity field' block plugin, where the field is part of the block configuration. Then you create instances with the fields you want but there is no pre-defined list.
However, it might be hard to write an upgrade path for config to switch from the derivative blocks to new instances, especially if sites have block-specific markup or anything like that.
This would mean we end up with the new single block on top of the derived blocks. So in that case, we would probably want new sites to start with all the derived blocks suppressed, and for existing sites, allow them to removed derived blocks as they (manually) make the switch.
Then we could deprecate the derived blocks altogether, and issue deprecation messages, maybe add phpstan checks if they're found in config, and remove them entirely in a major version, either directly or to a contrib module.
Long process, but then we'd have one block, at least to start with, instead of hundreds.
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
I think moving to a configurable option is a good idea *for site builders*.
However, when using Layout Builder in overrides mode, this would place that feature in the hands of a content editor.
At present the existing field block is already too overwhelming for content editors (we're asking them to chose and configure a formatter) and this would make that scenario worse.
I think what we should instead be considering is something like layout builder browser module, but extending it for field blocks.
I would see this working whereby a site builder could create a pre-configured field block using a config entity. The config entity would store the entity type, field name, bundle and pre-configured formatter. We could have one block for each of those. Then the site builder might configure e.g. a 'Short published date' and a 'Full published data' field for the content editor to place in overrides mode.
So to summarize:
* I think configurable instead of derivatives are good for site builders working on the default layout, and agree with your approach above, and that the upgrade path will be hard
* I think we can't do that without also adding something that allows site builders to pre-configure field blocks for users that includes formatter configuration - π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
I added β¨ Add the notion of a 'configured layout builder block' to solve a number of content-editor and performance pain points Active for #95 and #96 but it also solves a lot of open issues in the LB queue. @DanielVeza and I have discussed it with product managers and the subsystem maintainer and have support for the idea.
I would like to propose we push ahead with the existing solution here in #85 as even with the new field block idea there and from #95 it will still help sites running in the 'legacy mode'
- π¦πΊAustralia nterbogt
Re-rolling patch for 10.1.x. Haven't checked if this is working for 10.0.
- last update
about 1 year ago Patch Failed to Apply - last update
about 1 year ago 30,409 pass, 7 fail - last update
about 1 year ago 29,666 pass, 7 fail - π¦πΊAustralia acbramley
Rolling an MR, will take a look at the failures and tests next
- Merge request !6132Issue #3043330: Reduce the number of field blocks created for entities (possibly to zero) β (Closed) created by acbramley
- π¦πΊAustralia acbramley
The last 2 failures are a bit mysterious:
testBlockFilter
fails for me locally on HEAD so it's hard to figure out what the missing block is that's causing the count to be off by 1.The failure in
testLayoutBuilderUi
can be reproduced via manual steps. Both the layout page and the node page show a bunch of "This block is broken or missing." messages immediately after enabling layout builder on a bundle. This goes away after a drush cr so we just need to figure out which cache isn't being cleared. I confirmed via debugging that the block plugin cache clear is being hit so it's something else. - π¦πΊAustralia acbramley
@mstrelan helped figure out the
testBlockFilter
failure - the test is filtering on the text "adm" which was matching a user derived field block "Preferred admin language code". Enabling the config setting fixes it.I've debugged the final fail, and can see what's going wrong
I confirmed via debugging that the block plugin cache clear is being hit so it's something else
This is correct, but the order of operations messes things up, it goes:
- Block plugin cache is cleared in presave
- layout_builder_entity_presave kicks in and calls InlineBlockEntityOperations::handlePresave
- This calls getInlineBlockComponents which gathers some block plugins
- Since the block plugin cache was cleared in step 1, the derivatives need to be regenerated
- This gets back into the FieldBlockDeriver, and since we're still in presave, the loadByProperties in getFieldMap doesn't find the display that we're saving!
- We cache a list of block plugins that doesn't include the newly enabled display's bundle's fields
- Status changed to Needs review
10 months ago 2:13am 15 January 2024 - π¦πΊAustralia acbramley
Added specific kernel test coverage for how the setting and layout builder displays interact with the plugin derivers.
- Status changed to Needs work
10 months ago 7:54pm 17 January 2024 - Status changed to Needs review
10 months ago 10:14pm 18 January 2024 - π¦πΊAustralia kim.pepper πββοΈπ¦πΊSydney, Australia
I assume we need a change record if we are changing existing behaviour.
- Status changed to Needs work
10 months ago 7:43pm 21 January 2024 - πΊπΈUnited States smustgrave
Do agree with the CR since it's adding a new form and behavior change.
- Status changed to Needs review
10 months ago 10:05pm 23 January 2024 - π¦πΊAustralia acbramley
- πΊπΈUnited States smustgrave
Setup a standard install locally with layout builder installed
Applied the MR without issue
Ran the database update, which ran fine.Went to /admin/config/content/layout-builder and verify the checkbox was checked
Went into my Article content type which is using layout builder.
All the user fields (for example) appear to be there just as they were before. I don't have layout builder enabled for usersUnchecking the setting at /admin/config/content/layout-builder
Went back into my Article content type
No user fields appeared now (Yay!)Changes look good to me +1 RTBC for me. Per new approach for #needs-review-queue-initiative going to leave in review for a few days for additional reviews.
- Status changed to RTBC
10 months ago 4:50pm 29 January 2024 - πΊπΈUnited States smustgrave
Gave it a few days and as a Major don't want to wait too long.
- Status changed to Needs work
9 months ago 4:29am 20 February 2024 - π³πΏNew Zealand quietone
I'm triaging RTBC issues β . I read the IS, the comments, the MR and the CR.
I tend to think in lists, so here is what I found.
- In #33 it is stated "by the time plugin alters run, derivers have already run". There is an issue to alter plugins early for migrate. I am adding that as a related issue.
- In #73 there is an ask for a followup. I am not 100% sure it is still needed. But I am adding the tag so that others working on this issue can decide.
- #74 says this should have been a stable blocker. I will consult with the other release managers on this.
- In #83 a reminder was given to provide interdiffs. Thank you! Without the interdiffs it requires too much time to confirm that the requested changes were made so I did not. Fortunately, there are experienced contributors here and there is now an MR,
- #95 suggests a different approach. Does this need discussion or a followup?
- This is changing the UI, adding tag.
- I read the MR (not a code review) and left comments that need to be addressed. This is changing schema and the remaining tasks asks for an update test. I do not see that test.
- I read the CR and it does not mention the UI nor performance. It should tell the reader how to use the new settings, where to find it, what is the URL, etc. Usually with a UI change including a screenshot is helpful, though not a requirement here. If it is true that this effects performance then that should be explained in the change record.
- I applied the MR and eventually figured out how to alter the setting. I found the description confusing. For me, the first two sentences are saying the same thing, that the field is always exposed. And is the 'impact' to reduce performance or increase it? I think that this needs input from Usability folks.
- Status changed to Needs review
9 months ago 5:44am 20 February 2024 - π³πΏNew Zealand quietone
Discussed with catch and changing to Critical because this can make sites unusable.
- π¦πΊAustralia acbramley
Updated field description and associated screenshot.
- π¦πΊAustralia acbramley
The MR is hitting https://www.drupal.org/project/drupal/issues/3422537 π Increment or remove GIT_DEPTH limit for GitlabCi Active and needs a clean rebase with 11.x
- Status changed to Needs work
9 months ago 4:37pm 21 February 2024 - πΊπΈUnited States smustgrave
Believe the description contains enough detail. Wouldn't want to add much more personally.
needs a clean rebase with 11.x
Dumb question but is that different from a regular rebase?
- Status changed to Needs review
9 months ago 10:04pm 21 February 2024 - π¦πΊAustralia acbramley
@smustgrave some people use the word interchangeably with "merge"
I don't know how I messed up the merge so badly yesterday, MR was showing 220+ commits. Rebased and looking good now.
- π¦πΊAustralia acbramley
Working on an upgrade path test... bit of a waste of time if you ask me but would rather just do it to get this across the line.
- Status changed to RTBC
9 months ago 4:05pm 22 February 2024 - πΊπΈUnited States smustgrave
Believe this one is ready again,
Can see the test coverage here https://git.drupalcode.org/issue/drupal-3043330/-/jobs/875234
Ran the same tests from #111
Setup a standard install locally with layout builder installed
Applied the MR without issue
Ran the database update, which ran fine.Went to /admin/config/content/layout-builder and verify the checkbox was checked
Went into my Article content type which is using layout builder.
All the user fields (for example) appear to be there just as they were before. I don't have layout builder enabled for usersUnchecking the setting at /admin/config/content/layout-builder
Went back into my Article content type
No user fields appeared now (Yay!) - Status changed to Needs work
9 months ago 5:27pm 3 March 2024 - π¬π§United Kingdom alexpott πͺπΊπ
Added some review comments. We need a config subscriber to clear the block manager cache. It is tempting to add this to \Drupal\layout_builder\Cache\ExtraFieldBlockCacheTagInvalidator and rename it to something about cache management.
- Status changed to Needs review
9 months ago 10:47pm 3 March 2024 - π¦πΊAustralia acbramley
Back to NR, just need a decision on the
br
tags or not. - Status changed to RTBC
9 months ago 9:34pm 6 March 2024 - πΊπΈUnited States smustgrave
Feedback has been addressed
Believe with the
it definitely makes it easier to read the description. - Status changed to Needs work
9 months ago 11:04pm 6 March 2024 - π¬π§United Kingdom alexpott πͺπΊπ
Just realised we've missing test coverage of the config form being added. Given there are plans to add more to this form and config object I think it'd be nice to make sure this form is working as we think it should.
- Assigned to acbramley
- π¦πΊAustralia acbramley
Another thing I thought was too trivial to warrant a test π
Will get it sorted soon.
- Issue was unassigned.
- Status changed to Needs review
9 months ago 11:36pm 6 March 2024 - Status changed to RTBC
9 months ago 6:23am 7 March 2024 - π³πΏNew Zealand danielveza Brisbane, AU
Test coverage has been added for the form. Looks good to me
- π¬π§United Kingdom alexpott πͺπΊπ
Committed and pushed 1561edc2b8 to 11.x and 9f4b1f4e1c to 10.3.x. Thanks!
-
alexpott β
committed 9f4b1f4e on 10.3.x
Issue #3043330 by acbramley, nterbogt, phenaproxima, Sam152, tim....
-
alexpott β
committed 9f4b1f4e on 10.3.x
- Status changed to Fixed
9 months ago 9:07am 7 March 2024 -
alexpott β
committed 1561edc2 on 11.x
Issue #3043330 by acbramley, nterbogt, phenaproxima, Sam152, tim....
-
alexpott β
committed 1561edc2 on 11.x
- Status changed to Needs work
9 months ago 9:13am 7 March 2024 - π¬π§United Kingdom longwave UK
I think we should consider adding this to the release notes so users who don't use this functionality are alerted that they can turn it off.
- π¦πΊAustralia acbramley
Thanks very much, this one was near and dear to me π
I can have a crack at the release note, I'll have a look round for something as a guide.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Congrats on landing this! And yay for using
#config_target
! πI created the follow-up π Mark layout_builder.settings fully validatable Needs review to ensure the brand new
layout_builder.settings
is fully validatable from the first release it ships with. (Note: if β¨ Add a "Validatable config" tests job to GitLab CI to help core evolve towards 100% validatability Fixed had landed, then that CI job would've automatically detected that we're getting a lower ratio of validatable config.)P.S.: AFAICT this means we can now close π Drupal Quickstart command runs into php memory limits, especially with demo_umami profile Postponed: needs info ? π€
- π©πͺGermany rkoller NΓΌrnberg, Germany
I was following along this issue loosely, but never realized that there were front facing changes - always thought the changes would only be under the hood. After it got committed I just noticed that the MR also added a new configuration page. Taking a look at the microcopy I think that there might be room for improvement - in regards of clarity and comprehension. Personally I consider it challenging to completely understand the consequences of that configuration based on the checkbox label and the description. Therefore I agree with @quiteone in #113 π Reduce the number of field blocks created for entities (possibly to zero) Fixed that it might be a good idea to discuss the microcopy in a usability meeting. I've already set it on the agenda #3424764-2: Drupal Usability Meeting 2024-03-08 β . In today's meeting we haven't had enough time left to discuss the issue, but we can hopefully get to it next Friday. Since this issue is already committed, in case we come up with a suggestion for a potential improvement, shall we open up a follow up issue about the proposed changes?
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
As a novice Layout Builder user I did not understand the change record: it seemed safe to always disable that checkbox, which made me wonder why we even have it.
So I grepped this issue for "BC" and found a counterexample by @tim.plunkett π Can we update the change record to clarify this? π A few examples of why you can't just uncheck that checkbox, and what it would take for an existing site to eventually uncheck it.
- π¦πΊAustralia acbramley
@Wim Leers thanks for pointing that out. I've added an example to the CR.
- π©πͺGermany rkoller NΓΌrnberg, Germany
We reviewed this issue at π Drupal Usability Meeting 2024-03-15 Active . That issue will have a link to a recording of the meeting.
For the record, the attendees at the usability meeting were @AaronMcHale, @blackbamboo, @rkoller, @simohell, @SKAUGHT, and @worldlinemine.
Per @larowlan's suggestion in #142 π Reduce the number of field blocks created for entities (possibly to zero) Fixed I've created a followup issue summarizing the outcomes of our discussion: π [meta] Improve the "Expose all fields as blocks to Layout Builder" feature Active
- πΊπΈUnited States smustgrave
Would the release note go here or on this ticket which removes most of this change https://www.drupal.org/project/drupal/issues/3432874 π Replace "Expose all fields as blocks to Layout Builder" configuration with feature flag Active
- π¬π§United Kingdom alexpott πͺπΊπ
π Replace "Expose all fields as blocks to Layout Builder" configuration with feature flag Active doesn't remove the important part of this change - it only changes how it is configured.
- Status changed to Needs review
8 months ago 10:40pm 24 March 2024 - π¦πΊAustralia acbramley
Added a release note, it's written as per the current config but we can change it in π Replace "Expose all fields as blocks to Layout Builder" configuration with feature flag Active
- Status changed to RTBC
8 months ago 1:30pm 25 March 2024 - Status changed to Fixed
8 months ago 11:04pm 25 March 2024 Automatically closed - issue fixed for 2 weeks with no activity.