Field storage and instance call toArray() in __wakeup() is very slow, remove it?

As long as you don't have any resources references or other objects references, there's great chances that basic PHP serialization remains safe.

@pounard: Yep the __sleep() will remove all of these so removing the wakeUp is fine.

@Berdir nice one! Good to go.

The logic in __sleep() / __wakeup() so far was :
- __sleep() saves to the same array that is used for regular config storage
- on that array, __wakeup() performs the same steps than when reading from config storage (i.e pass the array through __construct())

With the patch, we "downcast" with toArray(), but no longer "upcast" through __construct(). This happens to work now because FieldStorageConfig / FieldInstanceConfig __construct() have been simplified quite a bit in the past couple weeks, less critical stuff happens there. But still, that feels incorrect. The contract for toArray() is "scale the object down to an array of values for storage, those values are what your __construct() will receive when the object is re-instantiated".

If it's toArray() that is costly, what if we just remove the toArray() call, but keep $this->__construct(get_object_vars($this)); ?
TBH, I don't really remember why the array_intersect_key() thing was added...

Also : the __sleep() implementations have this :

    // Serialize $entityTypeId property so that toArray() works when waking up.
    $properties[] = 'entityTypeId';

Not needed anymore if __wakeup() no longer calls toArray().

Hm, right, my #5 is outdated, "serialize to toArray() like on regular config storage" was what we were doing initially when we used php's Serializable interface.

But then we moved to __sleep() / __wakeup() because of those weird fails across PHP versions, and __sleep() doesn't let specifiy *values* to serialize, only the *names* of the properties of the current object to include in the serialization. So we used array_keys(toArray()) as a way to separate "basic properties" from "runtime, lazy-loaded or lazy-computed properties that shouldn't be serialized". We need to intersect with the actual properties present in the actual object, because FieldInstanceConfig::toArray() adds 'field_type' for config storage, which is currently not present as an actual property in the object, and would trigger notices.

[Note that we *should* probably keep "field_type" as an actual property in FieldInstanceConfig, since you pointed out there are critical code paths where we only load the FieldStorage to be able to get the field type - and we do save that info in the yml file, just to ditch it on load... But that's probably a different task]

I still think the behavior described in #5 (mimick what happens on regular config storage, i.e serialization as storage in a different backend) would be the right generic approach for all ConfigEntities, since they too get serialized, but, unlike ContentEntities, don't have a generic mechanism - I tried that in #1977206: Default serialization of ConfigEntities, but stalled. Looks like __sleep() / __wakeup() won't allow that though.

Meanwhile, I guess I'm fine with the current patch : keep __sleep(), remove __wakeup() and stop going through the __construct(), since we don't do anything important in there.

Regarding $properties[] = 'entityTypeId':
It was added in #2256679: Use config schema to determine which config entity properties to export regardless of whether they are public or protected, which added a base implementation of ConfigEntityBase::toArray() that, well, is suddenly more expensive :-), and that relies on $this->getEntityType() (and thus $this->getEntityTypeId(), and thus $this->entityTypeId).

It wouldn't be needed anymore if __wakeup() stopped calling toArray() but still called __construct(). But if we stop calling __construct() too, then I guess it's still needed, otherwise it looks like an unserialized object would have no $this->entityTypeId at all ? We'd just need to update the comment in __sleep(), because it wrongfully points at a toArray() call in __wakeup() that no longer exists.

Well, that's precisely what I wanted to avoid with #1977206: Default serialization of ConfigEntities - have a generic serialization code that works for all config entity types, rather than have each ConfigEntity type figure out its serialization code (or break slient havoc if it doesn't - as we know, some entity types don't even realize they're being serialized by some stuff somewhere).

The current code in __sleep() (or the one in patch #1) could potentially work for all entity types. IIRC from #1977206: Default serialization of ConfigEntities, it was Views config entities that were choking, but then it's more Views that would be a special case that need specific overrides.

I'd personnally rather stick with #1 (with the comment fix mentioned in #7) and not lose all hopes of #1977206: Default serialization of ConfigEntities :-). Anyone feel free to disagree though, I won't oppose #9 if people favor that.

Well, internal properties like entityTypeId & originalID were taken care of by calling the __construct() on __wakeup() ;-)

But well,
- running the __construct() would make sense if what we stored were the values of toArray() (= what CMI storage does), and makes less sense if we store the values of the live object as we currently do. Storing the values of toArray() would require moving to Serializable instead of __sleep() / __wakeup(), so not for this issue.
- agreed with the reasoning that, ideally, serialization would be taken care of generically in ConfigEntityBase, and then that code having to deal with the internals of ConfigEntities is fine.

So, I'm fine with moving forward with #9 for now, with the hope that we can get #1977206: Default serialization of ConfigEntities right with Serializable.

@Berdir: FYI, opened #2313875: Preserve the 'field_type' within FieldInstanceConfig

This is running against old code, let's make sure it still passes.

Committed 54a49d5 and pushed to 8.x. Thanks!