Entities tend to get serialized in various places : cache, forms...
ConfigEntities, being more business oriented, additionally tend to get more business specific helper methods, possibly relying on internal properties / derived data / external services, other than their raw data, and which you usually don't want to serialize.
Nice thing is, ConfigEntities have a pretty straightforward serialization format: the exported properties that end up in their CMI file...
We had to do this for Field / FieldInstance entities, that fall exactly in this case above, but the code is pretty agnostic and might make sense for all ConfigEntity types.
(since then, #2205367: [HEAD BROKEN] PHP 5.4 duplicated that same code in EntityFormDisplay - leaving the symmetrical EntityViewDisplay out).
Basically, serialize to the raw array returned by getExportProperties() (as if it was going to CMI), and unserialize by passing this array to __construct() (just as if it was read from config). Same format, different storage.
Patch once I get a node id.
Comment | File | Size | Author |
---|---|---|---|
#74 | 1977206-73.patch | 22.71 KB | jhodgdon |
#72 | 1977206-config-entity-serialization-71.patch | 22.34 KB | Cameron Tod |
#70 | 1977206-config-entity-serialization-70.patch | 25.35 KB | Cameron Tod |
#62 | 1977206-62.patch | 23.17 KB | mondrake |
#58 | interdiff_55-58.txt | 821 bytes | mondrake |
Comments
Comment #1
yched CreditAttribution: yched commentedPatch.
Comment #3
andypostThere's related issue #1818574: Support config entities in typed data EntityAdapter and I hit the bug in #1907960-148: Helper issue for "Comment field" where unserialized value turns into string
Comment #4
yched CreditAttribution: yched commentedSo, yeah, Views config entities seem to have an issue with this, because there's a 2-way crossreference with the associated ViewExecutable. Got lost trying to unfold this, would welcome feedback from the Views team.
#1875992: Add EntityFormDisplay objects for entity forms is another example of a config entity that would need to duplicate the exact same serialize / unserialize logic added here.
Comment #5
plachRelated issue: #2027795: Optimize content entity serialization
Comment #5.0
plachPhrasing
Comment #6
yched CreditAttribution: yched commentedReviving this after #2205367: [HEAD BROKEN] PHP 5.4.
Content entities have default serialization code in ContentEntityBase now, config entities should have theirs too.
Comment #7
yched CreditAttribution: yched commentedComment #9
BerdirI had a quick look at why this is failing (fatal errors is usually phpunit fails) and the reason is that mocking a class without invoking the constructor goes through unserialize(), which in turn calls __wakeup(), which then calls the constructor anyway o.0
See PHPUnit_Framework_MockObject_Generator::getObjects().
The test that is failing is TourTest.
Comment #10
yched CreditAttribution: yched commentedOuch - right, nice find :
Nasty.
Fixed TourTest to call the Tour config entity constructor, which requires placing a mocked plugin.manager.tour.tip in the container :-/
Other unit tests are green at home.
Comment #11
yched CreditAttribution: yched commentedAltough another approach could be to add a safeguard to our __wakeup()
Alternate to #10, interdiff is against #6.
Comment #14
yched CreditAttribution: yched commentedRight, FieldConfig::__sleep() had an additional safeguard: the keys returned by getExportProperties() do not necessarily correspond to actual object properties.
Comment #16
swentel CreditAttribution: swentel commentedunserilalize - sounds very musical :)
Comment #17
swentel CreditAttribution: swentel commentedHad a quick peek in the editor admin test and remembered this issue: #2207777: Can not configure editor whilst creating a new text format
So changing the test a little fixes the issue, but that's probably not the right way since it didn't fail before .. posting the patch though as it may trigger ideas.
Comment #18
swentel CreditAttribution: swentel commentedComment #21
yched CreditAttribution: yched commentedRestarting after the discussion in #2313053: Field storage and instance call toArray() in __wakeup() is very slow, remove it?.
If we want ConfigEntity serialization to mimick what's done on config write, we need to use Serialzable interface rather than __sleep() / __wakeup(), since __sleep() doesn't let you specify arbitrary serialized values, only the names of the object properties whose values will be part of the serialization.
Let's see what breaks. Views were a special snowflake IIRC.
Comment #23
yched CreditAttribution: yched commentedTaking over the workaround for PHPUnit mock creation from the earlier patches.
Comment #24
yched CreditAttribution: yched commentedComment #26
yched CreditAttribution: yched commentedOuch. The new default implementation of ConfigEntityBase::toData() doesn't work for fresh ConfigEntity objects because it relies on $entity->id() to get the config schema :
On a fresh ConfigEntity, e.g when visiting admin/config/content/formats/add (this form serializes a fresh filter_format entity as part of the form state, like all entity forms) :
$this->id() is NULL, since the $format->format is still NULL,
$config_name is thus 'filter.format.',
and this does not match any known schema entry. 'filter.format.*' would.
AFAICT, we have no way of knowing the name of the config schema entry for a given entity type ('filter.format.*', or 'field.instance.*.*.*', with the right number of '*' parts), that information isn't formalized anywhere that I know of.
TypedConfigManager only knows how to find the right entry given an actual config name like field.instance.foo.bar.baz through trial and error with its (protected) getFallbackName() method.
Damn. Stalled again. Suggestions welcome :-/
Comment #27
yched CreditAttribution: yched commentedAs an example of why this patch would be helpful though:
FilterFormat currently doesn't do anything about its serialization, which means that serializing it (again, any entity form serializes its entity in the form_state) includes serializing its pluginBag, which includes the FilterPluginManager and its static cache of discovered plugin definitions :-/
Comment #28
Gábor HojtsyShould we add a method that returns a dummy ID to be used for this discovery and use that if $this->id is empty. The schema should not depend on the ID. While technically possible, it would be a nightmare :D
Comment #29
yched CreditAttribution: yched commentedWell, wouldn't that be equivalent to requiring an explicit
config_schema = "foo.bar.*.*.*"
in the entity type's annotation, and use that info to find the schema entry ?I mean, instead of guessing it from
($this->id() || $this->getDummyId())
as you propose ?(+ actually, $this->id() is not even necessarliy empty for fresh entities, it could be '..' (NULL . NULL . NULL) if the entity type uses compound names (e.g [entity_type].[bundle].[field_name]).
Agreed. Thus, requiring an ID from wich we backtrack-guess the schema, as we currently do, feels wrong :-)
Comment #30
Gábor HojtsyWell, I guess putting a config name template on the annotation would be doubling the information in some ways, since we are generating it from different components some of which are on the annotation already.
Comment #31
yched CreditAttribution: yched commentedThat would duplicate the config_prefix, yes.
But then I think we should ditch config_prefix = 'foo.bar' in favor of config_name_pattern = 'foo.bar.*.*.*', and deduce the former from the latter...
Comment #32
yched CreditAttribution: yched commentedRegarding that topic of the predictability of config schema entries - related issue mentioned by @alexpott : #2100203: Make config entities use dots in machine names consistently
Comment #33
yched CreditAttribution: yched commented#2094499-23: Convert "Manage (form) display" forms to be the official entity edit forms for EntityView[Form]Display objects showed that we need to take care of enforceIsNew as well.
Also, meanwhile, #2002138: Use an adapter for supporting typed data on ContentEntities added the _sleep() / _wakeup() based DependencySerializationTrait to all Entities, so this patch using /Serializable on ConfigEntities at the moment will conflict.
Anyway. This will still fail miserably, but for now, just a reroll, and the "enforceIsNew" thing.
Comment #34
yched CreditAttribution: yched commentedMeh, wrong logic for unserialize().
Also, giving the testbot a try...
Comment #36
yched CreditAttribution: yched commentedAlso : we probably could avoid the explicit call to ->enforceIsNew() on unserialize and just pass 'enforceIsNew' in the $values passed to _construct(), but this fails because ConfigEntityBase::_construct() calls $this->setOriginalId($this->id()), which wrongly does enforceIsNew(FALSE).
Opened #2405165: Entity::setOriginalId() does enforceIsNew(FALSE), that is wrong for ConfigEntities for that. Meanwhile, we do need that explicit enforceIsNew() call in unserialize() to switch that back.
Comment #37
yched CreditAttribution: yched commentedFTR, #2459873: FieldStorageConfig::__sleep() should unset ->original adds yet another one-off entity-type-specific fix for something that potentially any config entity might hit on serialization...
Comment #38
mondrakeIt seems this issue could help with #2393387: Add test for editing image effect when configuration form is Ajax enabled, too. Bumping to major as that issue is major.
Comment #39
yched CreditAttribution: yched commented#37 and #38 show that is still pretty major IMO.
The current situation has a high risk of fatals in edge cases (like in #38) / weird behavior (like in #37) / silent huge serialiazed form state, unless the ConfigEntity author takes deep care of how serialization happens.
The patch here is still blocked on toArray() relying on schema, which breaks on unsaved config entities for which no proper ID can be generated and thus no schema can be found (#26)
Maybe a middle-ground approach after #2432791: Skip Config::save schema validation of config data for trusted data could be to only provide default serialization logic in ConfigEntityBase for entity types that provide an explicit 'config_export' entry in their annotation and thus do not rely on schema for toArray() ?
Not perfect, but at least we can strongly recommend providing a config_export entry, and explicitly state that you need to take care of serialization if you don't ?
Comment #40
Fabianx CreditAttribution: Fabianx commentedSounds like a great idea to me.
Comment #41
yched CreditAttribution: yched commentedHere's what this could look like using 'config_export' from #2432791: Skip Config::save schema validation of config data for trusted data.
I still expect fails with Views because #4
And not sure how this plays with PHPUnit's use of unserialize() to mock objects :-/
Patch is rolled on top of #2432791: Skip Config::save schema validation of config data for trusted data,
"patch_only-do-not-test.patch" contains just the changes for this issue.
Comment #43
yched CreditAttribution: yched commentedNo time to debug right now, but note for later :
Many of these fails seem to come from
"Call to a member function getFieldStorageDefinition() on a non-object in FieldItemDataDefinition.php line 71"
(also seen a couple "Call to undefined method Drupal\Core\StringTranslation\TranslationWrapper::getFieldStorageDefinition() in FieldItemDataDefinition.php line 71", same class same line...)
Comment #44
yched CreditAttribution: yched commentedMy bet would be that it's related to #33, which the latest patch didn't look into yet :
Since this patch moves ConfigEntityBase to \Serializable interface, they now skip the __sleep() / __wakeup() provided by DependencySerializationTrait. Then weird thing ensues when the unserialized entity can't find its services like the TypedDataManager :-/
I guess it would be nice to move DependencySerializationTrait to the more modern and flexible \Serializable interface too - but : a trait
cannot implement an interface itself, so all classes currently using DependencySerializationTrait would need to explicitly state they implement \Serializable :-/. __sleep() / __wakeup() are more restrctive, but, as magic methods, they work "just by being there", which is kind of handy when rovided by a trait.
OTOH, switching back to __sleep() / __wakeup() for ConfigEntityBase default serialization here means we can't really serialize to toArray(), since __sleep() doesn't let you specify arbitrary serialized *values*, only the names of the object properties whose current values in $this will be part of the serialized data. Most we could do is serialize to "the keys present in toArray()". That kind of breaks the idempotency of "values from toArray() get fed to the __construct(), just like on a regular config save & read".
Comment #45
dawehnerWhat happens if you implement __sleep and \Serializable and throw an exception in _sleep telling the developer: hey it would be cool if you could add the implements Serializable to __class__ ?
Comment #46
yched CreditAttribution: yched commented@dawehner: you mean DependencySerializationTrait providing both :
- serialize()/unserialize() with the actual implementation ?
- __sleep() just throwing an exception like "if you hit this, it means you have used the trait without thinking of implementing Serializable, go fix your code" ?
Interesting :-)
A drawback I see is that one of the issues with D8 and serialiazation chains is that is hits a bit randomly - in the sense that some code somewhere can end up serializing a class in cases the class author didn't anticipate. Then the above would mean "my code/module works fine on my setup but not on others", or "my code works, and then I enable a module and get an exception" ?
Well, not sure that's really worse than other cases of "wrong combination of modules"...
Comment #47
yched CreditAttribution: yched commentedStraight reroll now that #2002138: Use an adapter for supporting typed data on ContentEntities is in
Comment #48
yched CreditAttribution: yched commentedForgot to upload the reroll.
Comment #49
yched CreditAttribution: yched commentedThen, giving a shot at moving everything to \Serializable : DependencySerializationTrait, Entity, ConfigEntityBase.
No interdiff, wouldn't make much sense.
Comment #52
yched CreditAttribution: yched commentedDidn't see that ForumManager overrides __sleep() / __wakeup(), those will need to be moved to serialize() / unserialiaze() too.
Comment #53
yched CreditAttribution: yched commentedAdapts ForumManager
Comment #55
mondrakeNeeded a reroll, plus tried to fix PHPUnit tests.
Comment #57
tim.plunkettThis is the name of the entity interface, the entity type interface is \Drupal\Core\Config\Entity\ConfigEntityType
Comment #58
mondrakeThanks a lot @tim.plunkett
Comment #62
mondrakeRerolled.
#43
I am curious to see if PHP 5.5 helps here
Comment #64
xjmDiscussed with @alexpott and postponing this to 8.1.x now that we are in RC. Reference: https://www.drupal.org/core/d8-allowed-changes
If we encounter any specific serialization bugs during 8.0.x, we should fix them individually.
Comment #65
xjmIt does make sense to fix this in a general way, since any config entity serialization could encounter this problem (like in #2393387: Add test for editing image effect when configuration form is Ajax enabled). So tagging as a triaged major.
Comment #67
jhodgdonI'm running into problems with this in my contrib/sandbox Configurable Help module. I didn't see it before today; the last time I know it was working was a few weeks ago, and since then I've switched from running 8.0.x to 8.1.x.
It's having errors when trying to serialize a form that has a config entity in it.
See #2688730: Add new item button is not working due to serialization problem for details... I'm not sure what to do about it... the issue summary here doesn't seem to be remotely up to date, and I don't know if it's something I can fix in my module or not?
Comment #68
jhodgdonAlso I think there is no reason this should still be postponed.
Comment #69
jhodgdonIncidentally... I was going to try to test and see if the latest patch here fixed the problem I'm seeing in a config entity editing form Ajax serialize/deserialize... but it doesn't apply. It's very hard to understand what is going on in this issue, because the patch doesn't seem to match the issue summary's plan of what to do at all ? I cannot tell.
Comment #70
Cameron Tod CreditAttribution: Cameron Tod as a volunteer commentedReroll
Comment #72
Cameron Tod CreditAttribution: Cameron Tod as a volunteer commentedcatch snuck a commit in just as I uploaded #70. Have another reroll
Comment #74
jhodgdonA lot of the test failures seem to be this:
PHP Fatal error: Interface 'Drupal\Core\Config\CacheableDependencyInterface' not found in /var/www/html/core/lib/Drupal/Core/Config/ConfigBase.php on line 32
So presumably that file ConfigBase is missing a
Let's try that... the only thing I added to the above patch... didn't make an interdiff file...
Comment #76
jhodgdonWell, that got it down to 88 test failures from 1000+. I'll leave the rest to the people actually working on this issue, but there seems to be a lot to fix.
Again, an update to the issue summary, with a hint to what modules might want to do in the meantime if they run into this issue, would be much appreciated! I'm very confused as to (a) what this patch is trying to do since it doesn't match the issue summary as far as I can tell and (b) what I can do in the meantime in my contrib module that is having a config entity serialization problem...
Comment #77
jhodgdonAs a note... I was able to fix the (critical) issue in my contrib module by implementing Serializable directly in my config entity, with code similar to what is in this patch. See issue #2688730: Add new item button is not working due to serialization problem on the Configurable Help module (which may or may not eventually make it into core).
Comment #78
alexpottWhy are all these things being changed here?
Also somewhat relatedly is the issue #2538348: Single config export UI exports the wrong entity properties and sometimes in the wrong format - and we should take into account the changes made by #2293773: Field allowed values use dots in key names - not allowed in config
Comment #82
jhodgdonI am revisiting this issue, because on #2920841: Fix serialization of help topics, andypost suggested that all I needed to do in my contrib_help module was to add config_export annotation to the config entity -- I had previously implemented \Serializable and added serialize/unserialize methods to my config entity, which fixed the problem I was having on #2688730: Add new item button is not working due to serialization problem.
So, I guess my question here is: is this issue still needed? If so, what for? It seems like it's been resolved, for purposes of Drupal forms Ajax. at least, on another issue and by a completely different method.
In any case, I think it still needs an issue summary update.
Comment #83
jhodgdonAs a note, when I try to serialize my config entity, I still get
So the config entity is not serializable, but Ajax forms seem to work now without that need. Again, I have no idea what has changed...