Entity displays fetch the bundle field definitions for deleted bundles

Can we maybe switch to only loading the field definitions when we actually need them instead of upfront in the constructor?

Yeah, I think this makes sense, assigning to @yched to get feedback from him.

A bit tedious to have to explicitely inititialize in so many entry points, but yeah, I vaguely suspected that at some point we'd have to untie that init() part from the object construction.

Only nit : feels slightly weird to see the various methods repeatedly and unconditionnally "initialize" the object, with the "initialize" method internally checking "nothing to do, actually, I've been initialized already".
--> Alternatively, we could leave init() untouched, and add $this->initialized || $this->init(); in those various places that need to make sure the Display is initialized ? Semantically, init() is done only once.

As bonus, this should allow us to remove the slow __wakeup(), see #2313053: Field storage and instance call toArray() in __wakeup() is very slow, remove it?.

Should be a Novice'ish task to re-roll this.

Rerolled.

Alright, never mind #5, works for me.
RTBC if green.

+++ b/core/modules/field_ui/src/Tests/EntityDisplayTest.php
@@ -293,7 +293,7 @@ public function testRenameDeleteBundle() {
+      'module' => array('entity_test', 'text')

@tstoeckler I found the problem here and I resolve it but I'm not sure if this bundle needs "User" dependence or not. I think it isn't necessary... Any can review it? :-)

Can you try to add a init() call to calculateDependencies() and check if that results in the previous dependencies?

@Berdir Yes, I will try it in my local enviroment ant test it, but I think that the testfail isn't for this patch because I execute the test in a clean environment and it had not fail it.

(I will try it anyway, thanks :) ).

I tried it but the test couldn't be execute (ajax error in the process). I don't know if I did something wrong or not. ^^

My suggestion seems to be working.

Indeed, looks reasonable.

FWIW, when testing this patch (and variants of it) locally, I get around 5500 fails, not just 1.

Probably a needed followup once this is fixed: the test is actually trying to install all contrib modules, not just core: maybe this is not such a great idea, as some of them might have unexpected side effects (like switching the DB connection to MongoDB, with no way back).

Not novice anymore :-)

Re-roll.

Ok, this is not going to work without the sorting from #2350537: Enforce order of display of components in config export, the initializing apparently changes the order sometimes, so we need to make sure that it is consistent.

Maybe we can summon @yched here as well :)

This might also help to convince him that sorting alphabetically is "the right thing to do"?

Yeah, as I said in #2350537: Enforce order of display of components in config export, I'll stop fighting on alphabetical sort :-)

Just wondering how the 'initialized' property should play with __sleep() and __wakeup().
It's currently not part of the serialized keys in __sleep(), so an unserialized Display will come out with 'initialized' == FALSE.
Yet the role of init() is to massage $this->content and $this->hidden, and those do get serialized.
So a Display that was initialized, will come out of unserialization in the correct "already initialized" state ?
Meaning we should also include 'initialized' in the serialized keys, to avoid needlessly initializing it again ?

re my own #35 : well, I guess that depends on what happened on the site between the moment the Display got serialized/persisted somewhere, and the moment it's read back/unserialized...

Maybe it's safer if unserialized displays have to replay init() to account for the current runtime context. At any rate, that's what current HEAD does (with init() called in __construct(), and __construct() called in __wakeup()). So yeah, never mind #35.

Then, it's just unclear to me why we have to ksort in init() as well.
preSave() takes care of sorting before yamls are written.
init() is about preparing valid runtime values for $this->content / $this->hidden - why is order important in the runtime values ?

I am not 100% sure, but it was definitely required.

I think it is related to the fact that we rebuild hidden, the test fails I had were because the order in hidden no longer matches.

Reroll after
#2407801: Views generic field handler does not work with base fields
#2220559: Entity displays must have a settings array
#2094499: Convert "Manage (form) display" forms to be the official entity edit forms for EntityView[Form]Display objects

Will look into #37 / #38 in a testbot helper issue

For those interested, this has been bugging me for a while, filed a patch for it : #2417817: Keep contrib modules out of ConfigImportAllTest

So, removing the ksort in init() makes ConfigImportAllTest fail - #2417357: Testbot helper issue for [#2248795]

Using the debug snippet from #2230637-93: Create a Language field widget and the related formatter, looks like it's about the langcode field again. Investigating.

Side note : opened #2417839: Add code to ease debugging of ConfigImportAllTest fails to keep that debug snippet at hand :-)

Related : if the patch changes the order of entries saved in core.entity_form_display.* / core.entity_view_display.*, we should also re-generate the ones shipped in core's default config ?

It seems that doing the sorting in toArray() rather than preSave() makes it not needed in init().

I did not fully figure out why putting it in preSave() causes fails in ConfigImportAllTest, this was about 'langcode' appearing in different places, so this looks like a similar tricky race condition as we had in #2230637-103: Create a Language field widget and the related formatter. I don't really feel like repeating the same detective work that we had back then, and I think toArray() is the right place for this anyway :-)

Updates of the core.entity_[form|view]_display.* shipped in default config with the new ordering.

Works for me, although I'm not 100% convinced that toArray() is the right place. Your changes look fine to me, good call on updating the existing default configuration.

Who is going to RTBC this now? :)

Reroll, the config file conflicts were a bit painful, I just deleted all the changes and then sorted it again.

Another issue added some sorting in the meantime, let's see if this works.

rerolled - indeed painful patch for that :)

Here I provided a fresh patch with only changes to EntityDisplayBase.

I am not sure if we still need to re-order default config files. I ran only Entity Related Test Cases, all of them passed.

Fixed the failed test case by adding below link before fetching the components

 $display = entity_load('entity_view_display', $display->id());

All previous tests, for example, testEntityDisplayCRUD loaded the entity using entity_load before checking it's component values. I hope, this is the correct way to test, please correct me if there is another way to make the test case pass.

Resolving conflicts and re-rolling for 8.4.x and 8.3.x respectively.

Added a reroll of patch #60

The patch needs a general overhaul, much has changed in the init() method. The content and hidden variables are not accessed directly anymore, the(set|remove)Component methods are called now, so the current approach causes an infinite loop.

Re-rolled with some changes.
Moved the setting of the initialized variable to the beginning of the init method to avoid infinite recursion. The drawback is that the entity is already marked as initialized even if the procedure is not over yet. But using two variables seems overkill at the moment.

Whopsie wrong file extension for interdiff.

There is still recursion happening. \Drupal\field_layout\Entity\FieldLayoutEntityDisplayTrait::init() extends the original init method, calling another method that ends up calling \Drupal\Core\Entity\EntityDisplayBase::getComponents() which calls the overridden init and so on.
Uploading a patch for fixing the this part, let's see what else is left out.
It's not very clean the new addition, but giving it a go for now.

Two methods, ::toArray() and ::onDependencyRemoval(), were missing a call to the init method.

+++ b/core/modules/field_layout/src/Entity/FieldLayoutEntityDisplayTrait.php
@@ -103,7 +103,16 @@ protected function doGetLayout($layout_id, array $layout_settings) {
+    // The ::ensureLayout() method will methods that require initialization.

Missing verb "invoke".

The remaining test failures are caused by the FieldLayoutEntityFormDisplay/FieldLayoutEntityViewDisplay classes which need a layout id to be set. The ensureLayout() method is called during init, so normally during object construction. With this patch approach is not, so we either move the invocation of that method into the __construct or call ensureLayout in some/most of the methods of FieldLayoutEntityDisplayTrait.
The problem is that we have to make sure that also getLayoutId() has run the ensureLayout() first.
I'm giving it a go by moving the call to ensureLayout() back in the constructor. Since ensureLayout() calls setLayoutId() which indirectly calls the init() method and ends up needing the layout id to be set, we have to wrap the ensureLayout() so that the init is not called. This replicates the current state of the code, where the ensureLayout() is called before init.

I think it still needs test to cover for the bug described.

Rerolled patch #79 with Drupal 9.4.x

Fixed failed commands on #86