seo-bundle icon indicating copy to clipboard operation
seo-bundle copied to clipboard

OriginalRouteExtractor Serialization of 'Closure' is not allowed

Open gomcodoctor opened this issue 7 years ago • 11 comments

When trying to use OriginalRouteExtractor

there is exception " Serialization of 'Closure' is not allowed " because of router service having a closure

public function serialize() { return serialize([ $this->extractors, $this->resource, $this->createdAt, ]); }

-configCacheFactory: ResourceCheckerConfigCacheFactory {#365 ▼ -resourceCheckers: RewindableGenerator {#363 ▼ -generator: Closure {#364 ▼ class: "appDevDebugProjectContainer" this: appDevDebugProjectContainer {#519 …14} file: "/var/cache/dev/appDevDebugProjectContainer.php" line: "2038 to 2041" } -count: 2 }

gomcodoctor avatar Sep 06 '17 09:09 gomcodoctor

Hi @gomcodoctor thank for reporting that but. Can you give some more information like the Symfony/PHP version and a real stack trace?

ElectricMaxxx avatar Sep 08 '17 04:09 ElectricMaxxx

@ElectricMaxxx thanks for replying

Serialization of 'Closure' is not allowed,

PHP version 7.1.9
Symfony 3.3.5

in vendor/symfony-cmf/seo-bundle/src/Cache/ExtractorCollection.php (line 88) SplObjectStorage->serialize() serialize(array(array(object(TitleExtractor), object(DescriptionExtractor), object(ExtrasExtractor), object(KeywordsExtractor), object(OriginalUrlExtractor), object(OriginalRouteExtractor)), '/home/*****/sylius/src/Gomco/OfferBundle/Entity/ProductVariant.php', 1504846997))

in vendor/symfony-cmf/seo-bundle/src/Cache/ExtractorCollection.php (line 88) ExtractorCollection->serialize() serialize(object(ExtractorCollection))

in vendor/symfony-cmf/seo-bundle/src/Cache/FileCache.php (line 71) FileCache->putExtractorsInCache('Gomco\OfferBundle\Entity\ProductVariant', object(ExtractorCollection))

in vendor/symfony-cmf/seo-bundle/src/SeoPresentation.php (line 173) SeoPresentation->getSeoMetadata(object(ProductVariant))

in vendor/symfony-cmf/seo-bundle/src/SeoPresentation.php (line 213)

gomcodoctor avatar Sep 08 '17 05:09 gomcodoctor

I have debugged it, issue occur when we serialize "$urlGenerator" ( which is router service ) in OriginalRouteExtractor, because router service has Closure {#364 ▼ class: "appDevDebugProjectContainer

gomcodoctor avatar Sep 08 '17 05:09 gomcodoctor

Maybe @wouterj can give some hints as he created the cache. My first idea is to implement Serializeableand to add/remove the dependency, but that isn't that easy.

ElectricMaxxx avatar Sep 08 '17 05:09 ElectricMaxxx

@ElectricMaxxx ya i think Serializeable is only solution or disable cache, lets wait for @wouterj 's reply

gomcodoctor avatar Sep 08 '17 05:09 gomcodoctor

In generall i would ask @wouterj why decided to cache an object that generates something instead of caching the result it had?

ElectricMaxxx avatar Sep 08 '17 06:09 ElectricMaxxx

as per my understanding - he is trying to cache all extractors OBJECT applicable to given object or resource, extractor which has dependency on other service is prone to this issue, instead of caching whole extractor object, caching name of extractor is enough i think, Please correct me if I am wrong, I am not that symfony expert

gomcodoctor avatar Sep 08 '17 06:09 gomcodoctor

Yeah, indeed, caching the name is probably enough. We then have to rework SeoPresentation a bit (if the extractor class names are cached, we have to filter all instances based on the cache name before using them), but it's probably the best solution.

@ElectricMaxxx caching values would mean we have to clear the cache each time the value changes. And that value most of the time is a getter, so it can depend on anything out there in the world. Caching here is used to already know which extractors apply to the object, which speed up things a bit already.

wouterj avatar Sep 08 '17 23:09 wouterj

@wouterj thanks for reply. There are few more suggestions if you going to edit cache part

  1. currently it do no take care of dev mode of environment, it always cache , cache should be disabled in dev mode.
  2. there should be option to enable and disable cache in config file

gomcodoctor avatar Sep 09 '17 05:09 gomcodoctor

@wouterj but where is the advantage to store a class name and instantiate and call it then, when it normally commes as service defintion from DI, which should be cached at all?

ElectricMaxxx avatar Sep 09 '17 08:09 ElectricMaxxx

@ElectricMaxxx you are right, I think we do not need cache when number of extractors are very small, cache will be useful when there are hundreds of extractors

gomcodoctor avatar Sep 09 '17 15:09 gomcodoctor