Movatterモバイル変換

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

		->children()
		->booleanNode('enable_annotations')->defaultFalse()->end()
		->scalarNode('cache')->defaultValue('serializer.mapping.cache.symfony')->end()
		->scalarNode('cache')->defaultValue('cache.pool.serializer')->end()

Copy link

Member

nicolas-grekasApr 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

GuilhemNApr 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

javiereguiluz added Bug Serializer labels

Copy link

Member

javiereguiluz commentedApr 25, 2016

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

Copy link

ContributorAuthor

GuilhemN commentedApr 25, 2016

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

xabbuh reviewed

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

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

Copy link

Member

xabbuhApr 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

xabbuh commentedApr 25, 2016

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

Copy link

Member

nicolas-grekas commentedApr 25, 2016

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

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

Copy link

Member

nicolas-grekas commentedApr 25, 2016

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 toCacheClassMetadataFactory, tagged@internal

Copy link

Member

xabbuh commentedApr 25, 2016

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

Copy link

ContributorAuthor

GuilhemN commentedApr 25, 2016•
edited
Loading

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

GuilhemN changed the title~~[Serializer] Fix a deprecation triggered by the ClassMetadataFactory~~[FrameworkBundle][Serializer] Fix a deprecation triggered by the ClassMetadataFactory

Copy link

Member

nicolas-grekas commentedApr 25, 2016

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

GuilhemN force-pushed theSERIALIZER branch from7d8ee14 toe363223Compare

April 25, 2016 16:32

Copy link

ContributorAuthor

GuilhemN commentedApr 25, 2016

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

nicolas-grekas reviewed

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


		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);

Copy link

Member

nicolas-grekasApr 25, 2016•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

ContributorAuthor

GuilhemNApr 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM 👍

GuilhemN force-pushed theSERIALIZER branch 2 times, most recently from205f808 to0d7ecc7Compare

April 25, 2016 17:25

GuilhemN added2 commits

April 25, 2016 19:26

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

96e418a

…ion for serializer"This reverts commit4f0b8be.

[Serializer] Improve a deprecation message

eccbffb

GuilhemN force-pushed theSERIALIZER branch 5 times, most recently from60d94c6 tof44cb14Compare

April 25, 2016 17:51

Copy link

ContributorAuthor

GuilhemN commentedApr 25, 2016•
edited
Loading

Rebased and updated.

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

nicolas-grekas reviewed

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

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

		publicfunctiontestDeprecatedSerializerCacheOption()

Copy link

Member

nicolas-grekasApr 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

GuilhemNApr 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

done

GuilhemN force-pushed theSERIALIZER branch fromf44cb14 toafdd0afCompare

April 25, 2016 18:01

nicolas-grekas reviewed

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

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

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

Copy link

Member

nicolas-grekasApr 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

nicolas-grekasApr 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I mean in the filename

Copy link

ContributorAuthor

GuilhemNApr 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

👍, renamed toserializer_legacy_cache

GuilhemN force-pushed theSERIALIZER branch fromafdd0af to7b44ba3Compare

April 25, 2016 18:40

[FrameworkBundle] Deprecate framework.serializer.cache

15579d5

GuilhemN force-pushed theSERIALIZER branch from7b44ba3 to15579d5Compare

April 25, 2016 18:47

Copy link

Member

nicolas-grekas commentedApr 25, 2016

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

Copy link

ContributorAuthor

GuilhemN commentedApr 25, 2016

I didn't think aboutcache.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 reviewed

Seehttps://github.com/symfony/symfony/pull/18648/files

UPGRADE-3.1.md


		```yaml
		framework:
		serializer:~

Copy link

Member

nicolas-grekasApr 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

this level should be removed,cache is a direct child offramework

Copy link

Member

stofApr 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

nicolas-grekasApr 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Copy link

Member

nicolas-grekas commentedApr 27, 2016

Thank you @Ener-Getick.

nicolas-grekas merged commit15579d5 intosymfony:master

nicolas-grekas added a commit that referenced this pull request

bug#18630[FrameworkBundle][Serializer] Fix a deprecation triggered …

f2228d5

…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       | MITWithout 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.cacheeccbffb [Serializer] Improve a deprecation message96e418a Revert "[FrameworkBundle] Fallback to default cache system in production for serializer"

GuilhemN mentioned this pull request

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

Merged

GuilhemN deleted the SERIALIZER branch

April 27, 2016 12:52

GuilhemN mentioned this pull request