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