Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
- After an entity bundle has been deleted, the entity displays attached to that bundle are deleted as well by
ConfigEntityBundleBase::deleteDisplays()
- When a display (which is an entity itself) gets deleted it gets loaded by
entity_delete_multiple()
- When a display gets loaded it initializes itself in
EntityDisplayBase::__construct()
- When a display gets initialized it fetches the entity field definitions in
EntityDisplayBase::init()
EntityDisplayBase::getFieldDefinitions()
then callsEntityManager::getFieldDefinitions()
with the bundle being the one that got deleted- This calls
EntityManager::buildBundleFieldDefinitions()
which in turn callsContentEntityInterface::bundleFieldDefinitions()
Therefore ContentEntityInterface::bundleFieldDefinitions()
can currently be called with non-existant bundles (primarily for entity types that use configuration entities as bundles).
The Node
entity, for example, does a $node_type = node_type_load($bundle);
in its bundleFieldDefinitions()
which fails in these cases. The only time $node_type
is used currently is wrapped inside a if (isset($node_type->title_label))
, which is why this doesn't fail currently.
Proposed resolution
Only load the field definitions when we actually need them instead of upfront in the constructor.
Remaining tasks
User interface changes
none.
API changes
none.
Comment | File | Size | Author |
---|---|---|---|
#87 | interdiff_86-87.txt | 1.2 KB | pooja saraah |
#87 | 2248795-87.patch | 5.11 KB | pooja saraah |
#86 | 2248795-86.patch | 5.1 KB | Shubham Chandra |
#79 | interdiff-76-79.txt | 1.25 KB | sardara |
#79 | 2248795-79.patch | 5.41 KB | sardara |
Comments
Comment #1
BerdirCan we maybe switch to only loading the field definitions when we actually need them instead of upfront in the constructor?
Comment #2
tstoecklerSomething like this maybe?
(The for-review patch is with -w)
Comment #3
tstoeckler2: 2248795-2-entity-display-delete-fail.patch queued for re-testing.
Comment #4
BerdirYeah, I think this makes sense, assigning to @yched to get feedback from him.
Comment #5
yched CreditAttribution: yched commentedA 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.Comment #8
BerdirAs 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?.
Comment #9
BerdirShould be a Novice'ish task to re-roll this.
Comment #10
estoyausenteRerolled.
Comment #11
estoyausenteComment #12
yched CreditAttribution: yched commentedAlright, never mind #5, works for me.
RTBC if green.
Comment #14
tstoecklerWill take a stab at this.
Comment #15
estoyausenteComment #16
estoyausente@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? :-)
Comment #17
star-szrComment #20
BerdirCan you try to add a init() call to calculateDependencies() and check if that results in the previous dependencies?
Comment #21
estoyausente@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 :) ).
Comment #22
estoyausenteI 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. ^^
Comment #23
BerdirMy suggestion seems to be working.
Comment #24
yched CreditAttribution: yched commentedIndeed, looks reasonable.
Comment #26
fgmFWIW, 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).
Comment #27
fgmComment #28
yched CreditAttribution: yched commentedNot novice anymore :-)
Comment #31
BerdirRe-roll.
Comment #33
BerdirOk, 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.
Comment #34
BerdirMaybe we can summon @yched here as well :)
This might also help to convince him that sorting alphabetically is "the right thing to do"?
Comment #35
yched CreditAttribution: yched commentedYeah, 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 ?
Comment #36
yched CreditAttribution: yched commentedre 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.
Comment #37
yched CreditAttribution: yched commentedThen, 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 ?
Comment #38
BerdirI 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.
Comment #39
yched CreditAttribution: yched commentedReroll 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
Comment #40
yched CreditAttribution: yched commentedFor those interested, this has been bugging me for a while, filed a patch for it : #2417817: Keep contrib modules out of ConfigImportAllTest
Comment #41
yched CreditAttribution: yched commentedSo, 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 :-)
Comment #42
yched CreditAttribution: yched commentedRelated : 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 ?
Comment #43
yched CreditAttribution: yched commentedIt 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 :-)
Comment #44
yched CreditAttribution: yched commentedUpdates of the core.entity_[form|view]_display.* shipped in default config with the new ordering.
Comment #45
BerdirWorks 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? :)
Comment #46
lokapujyaComment #49
BerdirReroll, 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.
Comment #51
swentel CreditAttribution: swentel commentedrerolled - indeed painful patch for that :)
Comment #53
pflame CreditAttribution: pflame commentedHere 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.
Comment #55
pflame CreditAttribution: pflame at Azri Solutions commentedFixed the failed test case by adding below link before fetching the components
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.
Comment #60
mohit_aghera CreditAttribution: mohit_aghera as a volunteer and at Axelerant commentedResolving conflicts and re-rolling for 8.4.x and 8.3.x respectively.
Comment #68
andypostComment #69
sandeep_jangra CreditAttribution: sandeep_jangra at OpenSense Labs commentedComment #70
sandeep_jangra CreditAttribution: sandeep_jangra at OpenSense Labs commentedAdded a reroll of patch #60
Comment #71
sardara CreditAttribution: sardara at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedThe 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.Comment #72
sardara CreditAttribution: sardara at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedRe-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.
Comment #73
sardara CreditAttribution: sardara at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedWhopsie wrong file extension for interdiff.
Comment #74
sardara CreditAttribution: sardara at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedThere 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.
Comment #76
sardara CreditAttribution: sardara at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedTwo methods,
::toArray()
and::onDependencyRemoval()
, were missing a call to the init method.Comment #77
sardara CreditAttribution: sardara at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedMissing verb "invoke".
Comment #79
sardara CreditAttribution: sardara at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedThe remaining test failures are caused by the
FieldLayoutEntityFormDisplay/FieldLayoutEntityViewDisplay
classes which need a layout id to be set. TheensureLayout()
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 callensureLayout
in some/most of the methods ofFieldLayoutEntityDisplayTrait
.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() callssetLayoutId()
which indirectly calls theinit()
method and ends up needing the layout id to be set, we have to wrap theensureLayout()
so that the init is not called. This replicates the current state of the code, where theensureLayout()
is called before init.Comment #80
sardara CreditAttribution: sardara at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedComment #81
sardara CreditAttribution: sardara at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedComment #82
sardara CreditAttribution: sardara at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedI think it still needs test to cover for the bug described.
Comment #86
Shubham Chandra CreditAttribution: Shubham Chandra as a volunteer and at Dotsquares Ltd. commentedRerolled patch #79 with Drupal 9.4.x
Comment #87
pooja saraah CreditAttribution: pooja saraah at Srijan | A Material+ Company for Drupal India Association commentedFixed failed commands on #86