schmittjoh / serializer

Library for (de-)serializing data of any complexity (supports JSON, and XML)
http://jmsyst.com/libs/serializer
MIT License
2.32k stars 589 forks source link

Fix memory leaks #1473

Closed scyzoryck closed 1 year ago

scyzoryck commented 1 year ago
Q A
Bug fix? yes
New feature? no
Doc updated no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets https://github.com/schmittjoh/serializer/pull/1431
License MIT
goetas commented 1 year ago

Yea, that is exactly what had in mind. @scyzoryck is this ready?

scyzoryck commented 1 year ago

Great! Yes, it is ready. It contains also changes from #1431

ro0NL commented 1 year ago

For people googling Notice: Undefined property: JMS\Serializer\JsonSerializationVisitor::$navigator ;)

When getResult is accessed in a decorated visitorProperty ... :boom:

Worked around it! Feel free to ignore.

scyzoryck commented 1 year ago

Thanks @ro0NL. If you have some example how to reproduce that error, let me know - I will try to fix it :)

ro0NL commented 1 year ago

@scyzoryck sure, we produce JSON with ESI includes:

    public function visitProperty(PropertyMetadata $metadata, $data): void
    {
        if (is_string($data) && is_array($metadata->groups) && in_array(self::ESI_GROUP, $metadata->groups, true)) {
            $placeholder = "\0".md5(spl_object_hash($metadata)."\0".$data)."\0";
            $this->esiReplacements[$this->visitor->getResult($placeholder)] = $data;
            $this->visitor->setNavigator($this->navigator); // added
            $this->visitor->visitProperty($metadata, $placeholder);

            return;
        }

        $this->visitor->visitProperty($metadata, $data);
    }

    public function getResult($data)
    {
        $result = $this->visitor->getResult($data);

        unset($this->navigator); // added 

        return $this->esiReplacements ? strtr($result, $this->esiReplacements) : $result;
    }

im fine with the current workaround

scyzoryck commented 1 year ago

So, let's keep it for now as it is as I cannot see easy wins here. If more people will have a similar issue let's think about the solution then. :)