#Blocks-Layouts

A year has passed and there has been no interest in providing the information asked for 3 years ago in #15. Therefor, closing this issue per #15.

If there is work to do here, then either re-open the issue or open a new issue and reference this one. If the choice is to use this issue then add a comment change make sure to change the issue status to 'Active'.

I just did a workaround for this problem over at 🐛 ContentEntityDenormalizer uses the field map, and so is unaware of bundle fields Active .

Just ran into 🐛 Layout builder doesn't support bundle computed field Closed: duplicate

I think we should take this out of key/value.

We could have field module implement similar logic in it's bundle field info hook instead maybe.

Crazy idea...but does it really matter if the tempstore entity is updated if the entity was saved with LB? Seems to me at face value that's a premature optimization. Can the route check just be removed...?

@catch, in #80 @tim.plunkett has self-assigned this issue and no progress for over a year ... is anyone has a better idea on how to overcome this issue without dependency on the route system?

Going to disagree that we should have test coverage added but will leave in review.

Reverted the added test. Mock tests are complex to integrate into the layout build scenario for scenario-based execution. Manual testing is required for this fix.

Thank you @smustgrave, I think I see how it could be done now. But I have run out of time in the short term, I'll try to get back to this to add the test of the upgrade path sooner rather than later.

I've tried passing the display's cache tags into the set() call so, in theory, it should be invalidated automatically when the display is saved but that doesn't seem to be the case. Must be missing something?

Much nicer solution, thanks for the links. I didn't see any decisions on 🌱 [policy] Standardize how we implement in-memory caches Needs work with how these memory cache services should be setup wrt. service id names or how specific/generic they should be but I've loosely tried to follow what other things are doing in core already with cache.asset_memory and system.module_admin_links_memory_cache

RemovePathKeyTest may be a good example

Essentially the test would be

Load a fieldBlock and verify the key DOES NOT exist

Run updates

Load the same block and verify the key DOES exist.

I added an update hook that updates FieldBlocks to set show_when_empty to FALSE, but I'm not sure if there's more I need to do there. See my previous note about whether more is needed.

But I am not sure how to go about writing a test for the update hook.

Note that the update hook should probably only target the default layouts (the entity view displays).

There could be thousands+ of nodes/entities with overridden layout data that have field blocks. Writing an update hook to load, check the nodes overridden layout for field blocks, then resave the nodes does not sound realistic. As long as the behavior for a field block that does not have any value for the new property is the same as the new property having empty value, I think this should be fine.

Thank you, I understand now.

If this key could appear on any FieldBlock then it would need an upgrade path. Good rule of thumb if you go into a config page, make no changes, click save, config export and there is a change then it needed an upgrade path.

and then a test to make sure update hook works properly

That's correct, there are a few example in core but don't have them super handy right now.

@smustgrave -

Sorry, but I want to make sure I'm understanding you correctly.

Currently the FieldBlock's default configuration has a value for the new config field that matches the FieldBlock's behavior pre-patch.

Are you saying that we need an update hook to update all FieldBlocks to set a value for the new config option and then a test to make sure update hook works properly? Or something else?

New configuration key will need an upgrade path + upgrade path test

MR !12052 refactors original approach to use a "show block when field is empty" checkbox rather than a "hide for empty" checkbox. This MR also includes updates to tests to test the new option.

As a result, I'm going to mark as Needs review and remove the needs tests tag.

My instinct is to also hide MR !5111, but I'll leave that to a maintainer.

catch asked in committer slack if there was anything else to do here. That caused us, include @Gábor Hojtsy, to take a second look here. And there is nothing more to do. There is clear agreement on deprecating field_layout.

Thanks everyone for resolving this.

The work to implement this starts in 📌 [meta] Tasks to deprecate the Field Layout module Active .

Agreed with @kimpepper's feedback on the MR. Didn't do an in-depth review of everything.

The feedback from Tim Plunkett on the MR hasn't been addressed, and I agree this should do something other than check the route, which feels brittle.

First, I just want to note that we should not be iterating on patches since we're using an MR. Any work and iteration should happen in the MR.

Second, I'd propose that the logic of this new field be flipped: instead of "Hide when empty" and defaulting to checked, use "Show when empty" and default to unchecked. I will add an MR with this alternate approach.

I had issues with language negotiation, when using the detection method "Content language" (/admin/config/regional/language/detection) there were problems like: the translation link would actually be modifying the original English version or in a more complex situation the interface language would be set to French and the content language would be set to Japanese, the block would appear in Japanese but the translation from the contextual menu would be done to the French version.
I'm adding a fix for this based on the MR!10504 from #348. It applies to 10.4.x as well.

FYI, these were also useful patches if you use these modules:
Paragraphs:
"3090200 - #45 Paragraph access check incorrect revision": " https://www.drupal.org/files/issues/2022-10-26/3084934-13_combine_309020... → "
Layout Builder Modal:
"3133695 - Incompatibility with layout builder": " https://www.drupal.org/files/issues/2022-12-15/3133695-13.patch → "

Seems already been reviewed and feedback has been addressed. Didn't see anything additional.