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
#74 1977206-73.patch22.71 KBjhodgdon
#72 1977206-config-entity-serialization-71.patch22.34 KBCameron Tod
#70 1977206-config-entity-serialization-70.patch25.35 KBCameron Tod
#62 1977206-62.patch23.17 KBmondrake
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 97,779 pass(es), 106 fail(s), and 133 exception(s). View
#58 interdiff_55-58.txt821 bytesmondrake
#58 1977206-58.patch23.13 KBmondrake
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 1977206-58.patch. Unable to apply patch. See the log in the details link for more information. View
#55 interdiff_53-55.txt1.56 KBmondrake
#55 1977206-55.patch23.11 KBmondrake
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,920 pass(es), 290 fail(s), and 76 exception(s). View
#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

Comments

yched’s picture

Status:Active» Needs review
FileSize
3.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
FileSize
4.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
FileSize
5.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,076 pass(es), 34 fail(s), and 10,538 exception(s). View
1.15 KB

Ouch - right, nice find :

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

FileSize
4.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,052 pass(es), 34 fail(s), and 10,559 exception(s). View
1.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
FileSize
4.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,473 pass(es), 23 fail(s), and 12 exception(s). View
578 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

FileSize
5.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,051 pass(es), 14 fail(s), and 6 exception(s). View
1.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
FileSize
5.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

FileSize
6.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
1.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

#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
FileSize
5.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,322 pass(es), 1,295 fail(s), and 509 exception(s). View
854 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: Add test for 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
FileSize
69.9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,806 pass(es), 153 fail(s), and 164 exception(s). View
5.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
FileSize
4.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

FileSize
21.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
FileSize
21.91 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,326 pass(es), 308 fail(s), and 114 exception(s). View
1.29 KB

Adapts ForumManager

Status:Needs review» Needs work

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

mondrake’s picture

Status:Needs work» Needs review
FileSize
23.11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,920 pass(es), 290 fail(s), and 76 exception(s). View
1.56 KB

Needed a reroll, plus tried to fix PHPUnit tests.

Status:Needs review» Needs work

The last submitted patch, 55: 1977206-55.patch, failed testing.

tim.plunkett’s picture

+++ b/core/modules/views_ui/tests/src/Unit/ViewUIObjectTest.php
@@ -122,6 +122,12 @@ public function testIsLocked() {
+    $entity_type = $this->getMock('\Drupal\views\ViewEntityInterface');

This is the name of the entity interface, the entity type interface is \Drupal\Core\Config\Entity\ConfigEntityType

mondrake’s picture

Status:Needs work» Needs review
FileSize
23.13 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 1977206-58.patch. Unable to apply patch. See the log in the details link for more information. View
821 bytes

Thanks a lot @tim.plunkett

Status:Needs review» Needs work

The last submitted patch, 58: 1977206-58.patch, failed testing.

mondrake queued 58: 1977206-58.patch for re-testing.

The last submitted patch, 58: 1977206-58.patch, failed testing.

mondrake’s picture

Status:Needs work» Needs review
FileSize
23.17 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 97,779 pass(es), 106 fail(s), and 133 exception(s). View

Rerolled.

#43

Many of these fails seem to come from
"Call to a member function getFieldStorageDefinition() on a non-object in FieldItemDataDefinition.php line 71"

I am curious to see if PHP 5.5 helps here

Status:Needs review» Needs work

The last submitted patch, 62: 1977206-62.patch, failed testing.

xjm’s picture

Version:8.0.x-dev» 8.1.x-dev
Status:Needs work» Postponed

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

xjm’s picture

Issue tags:+Triaged D8 major

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

Version:8.1.x-dev» 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhodgdon’s picture

I'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?

jhodgdon’s picture

Status:Postponed» Active
Issue tags:+Needs issue summary update

Also I think there is no reason this should still be postponed.

jhodgdon’s picture

Issue tags:+Needs reroll

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

Cameron Tod’s picture

Status:Active» Needs review
Issue tags:-Needs reroll
FileSize
25.35 KB

Reroll

Status:Needs review» Needs work

The last submitted patch, 70: 1977206-config-entity-serialization-70.patch, failed testing.

Cameron Tod’s picture

Status:Needs work» Needs review
FileSize
22.34 KB

catch snuck a commit in just as I uploaded #70. Have another reroll

Status:Needs review» Needs work

The last submitted patch, 72: 1977206-config-entity-serialization-71.patch, failed testing.

jhodgdon’s picture

Status:Needs work» Needs review
FileSize
22.71 KB

A 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

use Drupal\Core\Cache\CacheableDependencyInterface;

Let's try that... the only thing I added to the above patch... didn't make an interdiff file...

Status:Needs review» Needs work

The last submitted patch, 74: 1977206-73.patch, failed testing.

jhodgdon’s picture

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

jhodgdon’s picture

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

alexpott’s picture

+++ b/core/lib/Drupal/Core/Config/CachedStorage.php
@@ -17,7 +17,7 @@
-class CachedStorage implements StorageInterface, StorageCacheInterface {
+class CachedStorage implements StorageInterface, StorageCacheInterface, \Serializable {

+++ b/core/lib/Drupal/Core/Entity/EntityHandlerBase.php
@@ -21,7 +21,7 @@
-abstract class EntityHandlerBase {
+abstract class EntityHandlerBase implements \Serializable {

+++ b/core/lib/Drupal/Core/Form/FormBase.php
@@ -22,7 +22,7 @@
-abstract class FormBase implements FormInterface, ContainerInjectionInterface {
+abstract class FormBase implements FormInterface, ContainerInjectionInterface, \Serializable {

Why 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