symfony-cmf / seo-bundle

A SEO Solution for duplicate contents, page titles, etc.
https://cmf.symfony.com
47 stars 27 forks source link

OriginalRouteExtractor Serialization of 'Closure' is not allowed #332

Open gomcodoctor opened 7 years ago

gomcodoctor commented 7 years ago

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 }

ElectricMaxxx commented 7 years ago

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

gomcodoctor commented 7 years ago

@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 commented 7 years ago

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

ElectricMaxxx commented 7 years ago

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.

gomcodoctor commented 7 years ago

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

ElectricMaxxx commented 7 years ago

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

gomcodoctor commented 7 years ago

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

wouterj commented 7 years ago

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.

gomcodoctor commented 7 years ago

@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
ElectricMaxxx commented 7 years ago

@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?

gomcodoctor commented 7 years ago

@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