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.

Files: 
CommentFileSizeAuthor
#53 interdiff.txt1.29 KByched
#53 1977206-ConfigEntity-serialize-53.patch21.91 KByched
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,326 pass(es), 308 fail(s), and 114 exception(s).
[ View ]
#49 1977206-ConfigEntity-serialize-49.patch21.02 KByched
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#48 1977206-ConfigEntity-serialize-48.patch4.95 KByched
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,141 pass(es), 142 fail(s), and 153 exception(s).
[ View ]
#41 patch_only-do-not-test.patch5.49 KByched
#41 1977206-ConfigEntity-serialize-41_with_2432791.patch69.9 KByched
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,806 pass(es), 153 fail(s), and 164 exception(s).
[ View ]
#34 interdiff.txt854 bytesyched
#34 1977206-ConfigEntity-serialize-34.patch5.66 KByched
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,322 pass(es), 1,295 fail(s), and 509 exception(s).
[ View ]
#33 interdiff.txt1.04 KByched
#33 1977206-ConfigEntity-serialize-33.patch5.67 KByched
#23 interdiff.txt1.26 KByched
#23 1977206-ConfigEntity-serialize-23.patch6.02 KByched
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#21 1977206-ConfigEntity-serialize-21.patch5.7 KByched
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#17 interdiff.txt1.6 KBswentel
#17 ConfigEntity-serialize-1977206-17.patch5.55 KBswentel
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,051 pass(es), 14 fail(s), and 6 exception(s).
[ View ]
#14 interdiff.txt578 bytesyched
#14 ConfigEntity-serialize-1977206-14.patch4.7 KByched
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,473 pass(es), 23 fail(s), and 12 exception(s).
[ View ]
#11 interdiff.txt1.05 KByched
#11 ConfigEntity-serialize-1977206-11.patch4.66 KByched
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,052 pass(es), 34 fail(s), and 10,559 exception(s).
[ View ]
#10 interdiff.txt1.15 KByched
#10 ConfigEntity-serialize-1977206-10.patch5.67 KByched
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,076 pass(es), 34 fail(s), and 10,538 exception(s).
[ View ]
#6 ConfigEntity-serialize-1977206-6.patch4.38 KByched
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#1 ConfigEntity-serialize-1977206-1.patch3.16 KByched
FAILED: [[SimpleTest]]: [MySQL] 54,912 pass(es), 8 fail(s), and 4 exception(s).
[ View ]

Comments

yched’s picture

Status:Active» Needs review
StatusFileSize
new3.16 KB
FAILED: [[SimpleTest]]: [MySQL] 54,912 pass(es), 8 fail(s), and 4 exception(s).
[ View ]

Patch.

Status:Needs review» Needs work

The last submitted patch, ConfigEntity-serialize-1977206-1.patch, failed testing.

andypost’s picture

There'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

yched’s picture

So, 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.

plach’s picture

plach’s picture

Issue summary:View changes

Phrasing

yched’s picture

Issue summary:View changes
Issue tags:-Field API+Entity Field API
StatusFileSize
new4.38 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Reviving this after #2205367: [HEAD BROKEN] PHP 5.4.

Content entities have default serialization code in ContentEntityBase now, config entities should have theirs too.

yched’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 6: ConfigEntity-serialize-1977206-6.patch, failed testing.

Berdir’s picture

I 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.

yched’s picture

Status:Needs work» Needs review
StatusFileSize
new5.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,076 pass(es), 34 fail(s), and 10,538 exception(s).
[ View ]
new1.15 KB

Ouch - right, nice find :

<?php
PHPUnit_Framework_MockObject_Generator
::getObject() :
if (
$callOriginalConstructor) {
  ...
} else {
 
// Use a trick to create a new object of a class
  // without invoking its constructor.
 
$object = unserialize('O:%d:"%s":0:{}', strlen($className), $className);
}
?>

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.

yched’s picture

StatusFileSize
new4.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,052 pass(es), 34 fail(s), and 10,559 exception(s).
[ View ]
new1.05 KB

Altough another approach could be to add a safeguard to our __wakeup()

Alternate to #10, interdiff is against #6.

The last submitted patch, 10: ConfigEntity-serialize-1977206-10.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 11: ConfigEntity-serialize-1977206-11.patch, failed testing.

yched’s picture

Status:Needs work» Needs review
StatusFileSize
new4.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,473 pass(es), 23 fail(s), and 12 exception(s).
[ View ]
new578 bytes

Right, FieldConfig::__sleep() had an additional safeguard: the keys returned by getExportProperties() do not necessarily correspond to actual object properties.

Status:Needs review» Needs work

The last submitted patch, 14: ConfigEntity-serialize-1977206-14.patch, failed testing.

swentel’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -268,4 +268,32 @@ public function url($rel = 'edit-form', $options = array()) {
+    // PHPUnit uses unserilalize() on a made up string as a trick when bypassing

unserilalize - sounds very musical :)

swentel’s picture

StatusFileSize
new5.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,051 pass(es), 14 fail(s), and 6 exception(s).
[ View ]
new1.6 KB

Had 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.

swentel’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 17: ConfigEntity-serialize-1977206-17.patch, failed testing.

The last submitted patch, 17: ConfigEntity-serialize-1977206-17.patch, failed testing.

yched’s picture

Status:Needs work» Needs review
StatusFileSize
new5.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Restarting 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.

Status:Needs review» Needs work

The last submitted patch, 21: 1977206-ConfigEntity-serialize-21.patch, failed testing.

yched’s picture

StatusFileSize
new6.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new1.26 KB

Taking over the workaround for PHPUnit mock creation from the earlier patches.

yched’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 23: 1977206-ConfigEntity-serialize-23.patch, failed testing.

yched’s picture

Ouch. 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 :

$config_name = $this->getEntityType()->getConfigPrefix() . '.' . $this->id();
$definition = $this->getTypedConfig()->getDefinition($config_name);

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 :-/

yched’s picture

As 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 :-/

Gábor Hojtsy’s picture

Should 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

yched’s picture

Should we add a method that returns a dummy ID to be used for this discovery

Well, 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]).

The schema should not depend on the ID

Agreed. Thus, requiring an ID from wich we backtrack-guess the schema, as we currently do, feels wrong :-)

Gábor Hojtsy’s picture

Well, 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.

yched’s picture

That 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...

yched’s picture

Regarding that topic of the predictability of config schema entries - related issue mentioned by @alexpott : #2100203: Make config entities use dots in machine names consistently

yched’s picture

StatusFileSize
new5.67 KB
new1.04 KB

#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.

yched’s picture

Status:Needs work» Needs review
StatusFileSize
new5.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,322 pass(es), 1,295 fail(s), and 509 exception(s).
[ View ]
new854 bytes

Meh, wrong logic for unserialize().

Also, giving the testbot a try...

Status:Needs review» Needs work

The last submitted patch, 34: 1977206-ConfigEntity-serialize-34.patch, failed testing.

yched’s picture

Also : 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.

yched’s picture

FTR, #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...

mondrake’s picture

It seems this issue could help with #2393387: WSOD editing image effect when configuration form is Ajax enabled, too. Bumping to major as that issue is major.

yched’s picture

#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 ?

Fabianx’s picture

Sounds like a great idea to me.

yched’s picture

Status:Needs work» Needs review
StatusFileSize
new69.9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,806 pass(es), 153 fail(s), and 164 exception(s).
[ View ]
new5.49 KB

Here'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.

Status:Needs review» Needs work

The last submitted patch, 41: 1977206-ConfigEntity-serialize-41_with_2432791.patch, failed testing.

yched’s picture

No 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...)

yched’s picture

My bet would be that it's related to #33, which the latest patch didn't look into yet :

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.

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".

dawehner’s picture

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.

What 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__ ?

yched’s picture

@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"...

yched’s picture

yched’s picture

Status:Needs work» Needs review
StatusFileSize
new4.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,141 pass(es), 142 fail(s), and 153 exception(s).
[ View ]

Forgot to upload the reroll.

yched’s picture

StatusFileSize
new21.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

Then, giving a shot at moving everything to \Serializable : DependencySerializationTrait, Entity, ConfigEntityBase.
No interdiff, wouldn't make much sense.

Status:Needs review» Needs work

The last submitted patch, 49: 1977206-ConfigEntity-serialize-49.patch, failed testing.

The last submitted patch, 48: 1977206-ConfigEntity-serialize-48.patch, failed testing.

yched’s picture

Didn't see that ForumManager overrides __sleep() / __wakeup(), those will need to be moved to serialize() / unserialiaze() too.

yched’s picture

Status:Needs work» Needs review
StatusFileSize
new21.91 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,326 pass(es), 308 fail(s), and 114 exception(s).
[ View ]
new1.29 KB

Adapts ForumManager

Status:Needs review» Needs work

The last submitted patch, 53: 1977206-ConfigEntity-serialize-53.patch, failed testing.