[FrameworkBundle][Serializer] Fix a deprecation triggered by the ClassMetadataFactory #18630

GuilhemN · 2016-04-24T15:18:47Z

Q	A
Branch?	master
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets
License	MIT

Without apparent reasons, FOSRestBundle's tests fail since #18561.

Passing a Doctrine Cache instance as 2nd parameter of the "Symfony\Component\Serializer\Mapping\Factory\ClassMetadataFactory" constructor is deprecated. This parameter will be removed in Symfony 4.0. Use the "Symfony\Component\Serializer\Mapping\Factory\CacheClassMetadataFactory" class instead: 6x
    1x in ErrorWithTemplatingFormatTest::testSerializeExceptionHtml from FOS\RestBundle\Tests\Functional
    1x in SerializerErrorTest::testSerializeExceptionJson from FOS\RestBundle\Tests\Functional
    1x in SerializerErrorTest::testSerializeExceptionJsonWithoutDebug from FOS\RestBundle\Tests\Functional
    1x in SerializerErrorTest::testSerializeExceptionXml from FOS\RestBundle\Tests\Functional
    1x in SerializerErrorTest::testSerializeInvalidFormJson from FOS\RestBundle\Tests\Functional
    1x in SerializerErrorTest::testSerializeInvalidFormXml from FOS\RestBundle\Tests\Functional

We don't use cache in our tests but some of them are not in debug mode (will change soon) so the cache is automatically used.

This PR fixes this deprecation by detecting if the cache used by the serializer is psr6 compliant or not (if it is, then it replaces the default metadata factory by an instance of the new class CacheClassMetadataFactory, otherwise the second parameter of the ClassMetadataFactory is used).

GuilhemN · 2016-04-24T15:28:00Z

The tests failure seems unrelated.

nicolas-grekas · 2016-04-25T06:13:01Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

@@ -513,7 +513,7 @@ private function addSerializerSection(ArrayNodeDefinition $rootNode)
                    ->canBeEnabled()
                    ->children()
                        ->booleanNode('enable_annotations')->defaultFalse()->end()
-                        ->scalarNode('cache')->defaultValue('serializer.mapping.cache.symfony')->end()
+                        ->scalarNode('cache')->defaultValue('cache.pool.serializer')->end()


this is a BC break to me, isn't it?

If this is merged in 3.1 where it was introduced, then I think it isn't.

javiereguiluz · 2016-04-25T06:20:45Z

@Ener-Getick please, don't remove any of the rows of the PR table. In your table we miss the Branch row, which is were you propose where to merge this. If you are not sure, it's OK to say so. Thanks!

GuilhemN · 2016-04-25T06:26:34Z

@javiereguiluz I thought it was enough to target the right branch, added ☺

xabbuh · 2016-04-25T07:22:07Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/SerializerCachePass.php

+{
+    public function process(ContainerBuilder $container)
+    {
+        if (!$container->hasAlias('serializer.mapping.cache') && !$container->hasDefinition('serializer.mapping.cache')) {


You can just use $container->has('serializer.mapping.cache').

xabbuh · 2016-04-25T07:31:02Z

Can't we always use the CacheClassMetadataFactory when the cache is configured and optionally wrap a Doctrine cache instance in a DoctrineAdapter?

nicolas-grekas · 2016-04-25T08:02:14Z

Understood, this is for 3.1 :)
Then I have a few suggestions:

change the deprecation message to add the version: ...constructor is deprecated since version 3.1....
replace the compiler pass by a deprecation of the cache configuration key and a few wiring in the DI extension.

nicolas-grekas · 2016-04-25T08:42:49Z

If we don't want to deprecate the cache configuration key, then I'd suggest doing what @xabbuh suggests, and do it at runtime using a static method as factory added to CacheClassMetadataFactory, tagged @internal

xabbuh · 2016-04-25T09:35:08Z

I agree with @nicolas-grekas. Now that we have the cache.pool.serializer pool we do not need the cache config option for the serializer anymore. People can now configure the cache through the semantic config of the cache subsystem.

GuilhemN · 2016-04-25T16:17:20Z

@nicolas-grekas @xabbuh then the default value introduced in #18561 should be removed right ?

nicolas-grekas · 2016-04-25T16:29:42Z

Yep, can you do that in this PR, as a first commit?

GuilhemN · 2016-04-25T16:34:01Z

@nicolas-grekas yes, I'm gonna split my commit

nicolas-grekas · 2016-04-25T16:50:18Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -982,7 +982,9 @@ private function registerSerializerConfiguration(array $config, ContainerBuilder

        $chainLoader->replaceArgument(0, $serializerLoaders);

-        if (!$container->getParameter('kernel.debug')) {
+        if (isset($config['cache']) && $config['cache']) {
+            @trigger_error('Using the option "framework.serializer.cache" is deprecated since version 3.1. This option will be removed in 4.0. You can configure a cache pool called "serializer" under "framework.cache" instead.', E_USER_DEPRECATED);


What about:
The "framework.serializer.cache" option is deprecated since Symfony 3.1 and will be removed in 4.0. You can configure a cache pool called "serializer" under "framework.cache.pools" instead.

…ion for serializer" This reverts commit 4f0b8be.

GuilhemN · 2016-04-25T17:52:00Z

Rebased and updated.

I also added notes in the upgrading files to inform about this new deprecation.

nicolas-grekas · 2016-04-25T18:00:21Z

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php

+        $this->assertFalse($container->hasDefinition('serializer.mapping.cache_class_metadata_factory'));
+    }
+
+    public function testDeprecatedSerializerCacheOption()


Should be tagged @group legacy so that we don't forget removing it when preparing 4.0

nicolas-grekas · 2016-04-25T18:05:35Z

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/serializer_cache.php

@@ -0,0 +1,8 @@
+<?php
+
+$container->loadFromExtension('framework', array(


I suggest adding the "legacy" word in these fixtures so we can easily find them when preparing 4.0

I mean in the filename

👍, renamed to serializer_legacy_cache

nicolas-grekas · 2016-04-25T19:13:35Z

👍 with only one wonder: should the UPGRADE files tell about the serializer pool, and/or tell about cache.pool.local, which is the centralized way of configuring e.g. APCu for all internal adapters at once?

GuilhemN · 2016-04-25T19:20:09Z

I didn't think about cache.pool.local, this is indeed probably better to use it.
And in case the user wants something more specific, we can add a link to the documentation (which doesn't exist yet).

nicolas-grekas · 2016-04-27T07:21:39Z

UPGRADE-3.1.md

+
+   ```yaml
+   framework:
+       serializer: ~


this level should be removed, cache is a direct child of framework

no it should not be removed. If you remove the serializer key entirely, it disables the serializer in FrameworkBundle

See https://github.com/symfony/symfony/pull/18648/files

nicolas-grekas · 2016-04-27T07:24:57Z

Thank you @Ener-Getick.

…by the ClassMetadataFactory (Ener-Getick) This PR was merged into the 3.1-dev branch. Discussion ---------- [FrameworkBundle][Serializer] Fix a deprecation triggered by the ClassMetadataFactory | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | | License | MIT Without apparent reasons, [FOSRestBundle's tests fail](https://travis-ci.org/FriendsOfSymfony/FOSRestBundle/jobs/124384888) since #18561. ``` Passing a Doctrine Cache instance as 2nd parameter of the "Symfony\Component\Serializer\Mapping\Factory\ClassMetadataFactory" constructor is deprecated. This parameter will be removed in Symfony 4.0. Use the "Symfony\Component\Serializer\Mapping\Factory\CacheClassMetadataFactory" class instead: 6x 1x in ErrorWithTemplatingFormatTest::testSerializeExceptionHtml from FOS\RestBundle\Tests\Functional 1x in SerializerErrorTest::testSerializeExceptionJson from FOS\RestBundle\Tests\Functional 1x in SerializerErrorTest::testSerializeExceptionJsonWithoutDebug from FOS\RestBundle\Tests\Functional 1x in SerializerErrorTest::testSerializeExceptionXml from FOS\RestBundle\Tests\Functional 1x in SerializerErrorTest::testSerializeInvalidFormJson from FOS\RestBundle\Tests\Functional 1x in SerializerErrorTest::testSerializeInvalidFormXml from FOS\RestBundle\Tests\Functional ``` We don't use cache in our tests but some of them are not in ``debug`` mode (will change soon) so the cache is automatically used. This PR fixes this deprecation by detecting if the cache used by the serializer is psr6 compliant or not (if it is, then it replaces the default metadata factory by an instance of the new class ``CacheClassMetadataFactory``, otherwise the second parameter of the ``ClassMetadataFactory`` is used). Commits ------- 15579d5 [FrameworkBundle] Deprecate framework.serializer.cache eccbffb [Serializer] Improve a deprecation message 96e418a Revert "[FrameworkBundle] Fallback to default cache system in production for serializer"

@nicolas-grekas

… regressions (Ener-Getick) This PR was merged into the 3.1-dev branch. Discussion ---------- [FrameworkBundle] Fix a typo and add a test to prevent new regressions | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | This fixes a typo introduced in #18630 and adds a new functional test to avoid most regressions in service definitions in the future when features are only available in non-debug mode. cc @nicolas-grekas Commits ------- 61872ce Fix a typo and add a check to prevent regressions

…r-Getick) This PR was merged into the 3.1-dev branch. Discussion ---------- Do not use validator.cache and serializer.cache anymore ``framework.serializer.cache`` has been deprecated in symfony/symfony#18630 in favor of the new caching system. see symfony/symfony#18630 Commits ------- e7a2085 Use the new cache system configuration

carsonbot added the Status: Needs Review label Apr 24, 2016

GuilhemN force-pushed the SERIALIZER branch from 406b5a8 to 7d8ee14 Compare April 24, 2016 15:21

GuilhemN mentioned this pull request Apr 24, 2016

Fix the tests FriendsOfSymfony/FOSRestBundle#1442

Closed

nicolas-grekas reviewed Apr 25, 2016
View reviewed changes

javiereguiluz added Bug Serializer labels Apr 25, 2016

xabbuh reviewed Apr 25, 2016
View reviewed changes

GuilhemN changed the title ~~[Serializer] Fix a deprecation triggered by the ClassMetadataFactory~~ [FrameworkBundle][Serializer] Fix a deprecation triggered by the ClassMetadataFactory Apr 25, 2016

GuilhemN force-pushed the SERIALIZER branch from 7d8ee14 to e363223 Compare April 25, 2016 16:32

nicolas-grekas reviewed Apr 25, 2016
View reviewed changes

GuilhemN force-pushed the SERIALIZER branch 2 times, most recently from 205f808 to 0d7ecc7 Compare April 25, 2016 17:25

GuilhemN added 2 commits April 25, 2016 19:26

Revert "[FrameworkBundle] Fallback to default cache system in product…

96e418a

…ion for serializer" This reverts commit 4f0b8be.

[Serializer] Improve a deprecation message

eccbffb

GuilhemN force-pushed the SERIALIZER branch 5 times, most recently from 60d94c6 to f44cb14 Compare April 25, 2016 17:51

nicolas-grekas reviewed Apr 25, 2016
View reviewed changes

GuilhemN force-pushed the SERIALIZER branch from f44cb14 to afdd0af Compare April 25, 2016 18:01

nicolas-grekas reviewed Apr 25, 2016
View reviewed changes

GuilhemN force-pushed the SERIALIZER branch from afdd0af to 7b44ba3 Compare April 25, 2016 18:40

[FrameworkBundle] Deprecate framework.serializer.cache

15579d5

GuilhemN force-pushed the SERIALIZER branch from 7b44ba3 to 15579d5 Compare April 25, 2016 18:47

nicolas-grekas reviewed Apr 27, 2016
View reviewed changes

nicolas-grekas merged commit 15579d5 into symfony:master Apr 27, 2016

GuilhemN mentioned this pull request Apr 27, 2016

Do not use validator.cache and serializer.cache anymore symfony/symfony-standard#965

Merged

GuilhemN deleted the SERIALIZER branch April 27, 2016 12:52

GuilhemN mentioned this pull request Apr 27, 2016

[FrameworkBundle] Fix a typo and add a test to prevent new regressions #18657

Merged

fabpot mentioned this pull request May 13, 2016

Release v3.1.0-BETA1 #18776

Merged

xabbuh mentioned this pull request Jul 9, 2017

Stop using the deprecated "serializer.mapping.cache.apc" service symfony/symfony-docs#8133

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FrameworkBundle][Serializer] Fix a deprecation triggered by the ClassMetadataFactory #18630

[FrameworkBundle][Serializer] Fix a deprecation triggered by the ClassMetadataFactory #18630

GuilhemN commented Apr 24, 2016 •

edited

Loading

GuilhemN commented Apr 24, 2016

nicolas-grekas Apr 25, 2016

GuilhemN Apr 25, 2016

javiereguiluz commented Apr 25, 2016

GuilhemN commented Apr 25, 2016

xabbuh Apr 25, 2016

xabbuh commented Apr 25, 2016

nicolas-grekas commented Apr 25, 2016

nicolas-grekas commented Apr 25, 2016

xabbuh commented Apr 25, 2016

GuilhemN commented Apr 25, 2016 •

edited

Loading

nicolas-grekas commented Apr 25, 2016

GuilhemN commented Apr 25, 2016

nicolas-grekas Apr 25, 2016 •

edited

Loading

GuilhemN Apr 25, 2016

GuilhemN commented Apr 25, 2016 •

edited

Loading

nicolas-grekas Apr 25, 2016

GuilhemN Apr 25, 2016

nicolas-grekas Apr 25, 2016

nicolas-grekas Apr 25, 2016

GuilhemN Apr 25, 2016

nicolas-grekas commented Apr 25, 2016

GuilhemN commented Apr 25, 2016

nicolas-grekas Apr 27, 2016

stof Apr 27, 2016

nicolas-grekas Apr 27, 2016

nicolas-grekas commented Apr 27, 2016

		@@ -0,0 +1,8 @@
		<?php

		$container->loadFromExtension('framework', array(

[FrameworkBundle][Serializer] Fix a deprecation triggered by the ClassMetadataFactory #18630

[FrameworkBundle][Serializer] Fix a deprecation triggered by the ClassMetadataFactory #18630

Conversation

GuilhemN commented Apr 24, 2016 • edited Loading

GuilhemN commented Apr 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz commented Apr 25, 2016

GuilhemN commented Apr 25, 2016

Choose a reason for hiding this comment

xabbuh commented Apr 25, 2016

nicolas-grekas commented Apr 25, 2016

nicolas-grekas commented Apr 25, 2016

xabbuh commented Apr 25, 2016

GuilhemN commented Apr 25, 2016 • edited Loading

nicolas-grekas commented Apr 25, 2016

GuilhemN commented Apr 25, 2016

nicolas-grekas Apr 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuilhemN commented Apr 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Apr 25, 2016

GuilhemN commented Apr 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Apr 27, 2016

GuilhemN commented Apr 24, 2016 •

edited

Loading

GuilhemN commented Apr 25, 2016 •

edited

Loading

nicolas-grekas Apr 25, 2016 •

edited

Loading

GuilhemN commented Apr 25, 2016 •

edited

Loading