EntityDisplayBase::__sleep() serializes to toArray()
EntityDisplayBase::__wakeup() re-runs a toArray() to filter out

- Not sure why that is really needed
- That was written back when toArray() was a straightforward, simple operation. It is much more costly now that it leverages config schema.

We should check whether we can remove that toArray() call in EntityDisplayBase::__wakeup().

CommentFileSizeAuthor
#3 2413841-EntityDisplayBase_wakeup-3.patch1.64 KByched
#1 2413841-EntityDisplayBase_wakeup-1.patch654 bytesyched

Comments

yched’s picture

yched commented
Status: Active » Needs review
StatusFileSize
new 2413841-EntityDisplayBase_wakeup-1.patch654 bytes

Let's see what breaks.

Status: Needs review » Needs work

The last submitted patch, 1: 2413841-EntityDisplayBase_wakeup-1.patch, failed testing.

yched’s picture

yched commented
Status: Needs work » Needs review
StatusFileSize
new 2413841-EntityDisplayBase_wakeup-3.patch1.64 KB

OK, yeah, we cannot pass the whole set of $this properties into the __construct(), because that would cause $this->pluginManager to stay to its default value of NULL.

We really need to pass only the values from toArray() into the __construct(), but we need to know which those are without running toArray() again.

This should do the trick.
Ugly because of the way __sleep() / __wakeup() works, we should really try get back to Serializable :-/
Also, #2405165: Entity::setOriginalId() does enforceIsNew(FALSE), that is wrong for ConfigEntities would help making __wakeup() a bit clearer by removing the isNew() / enforceIsNew() dance.

amateescu’s picture

amateescu commented
Status: Needs review » Reviewed & tested by the community

Indeed, the new code is quite ugly but since we don't have any better way to do it now, I think the performance of not involving the schema in these operations is worth it.

we should really try get back to Serializable :-/

As far as I remember, the problem was some difference between 5.3 and 5.4, but now that we require PHP 5.4, I think it should be possible?

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

OK it is indeed not pretty, but I don't have a better idea and it's self-contained until we can sort out Serializable. Committed/pushed to 8.0.x, thanks!

  • catch committed 9c71082 on 8.0.x 
    Issue #2413841 by yched: EntityDisplayBase::__wakeup() should avoid...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.