[FrameworkBundle][Validator] Fix apc cache service deprecation #16822

ogizanagi · 2015-12-03T13:44:17Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

Related to #16795

I guess the deprecation was on the wrong service.
Also, no deprecation notice was triggered about using "apc" as the value of the framework.validation.cache configuration option. This PR adds the missing deprecation.

📝 NOTE: The standard edition will need to be updated here.

Tobion · 2015-12-03T14:24:54Z

IMO both validator.mapping.cache.apc and validator.mapping.cache.doctrine.apc (as it has been) should be deprecated. There is no point if providing an apc service but not any other cache driver like filesystem, redis etc. So it's better to be let developers define them themselves.

ogizanagi · 2015-12-03T14:41:57Z

@Tobion : If we remove those services accordingly to what you say, then we should remove the serializer.mapping.cache.apc one too, or any other persistent cache layer implementation from Symfony.

The main benefit of proposing such an implementation is that it only requires the APCu php extension and allow to suggest optimizations from the Standard Edition.
If we don't, those lines should be removed, and we loose an opportunity to promote ways to improve performances.

Some refs about related discussions:

[FrameworkBundle] Add a doctrine cache service definition for validator mapping #14429 (comment)

Tobion · 2015-12-03T15:47:32Z

@ogizanagi yes I would suggest to deprecate serializer.mapping.cache.apc in 3.1 then.

The standard-edition should IMO actually configure the caching using filesystem. So if the caching is really a performance gain, just use a filesystem one by default (filesystem access required by default anyway due to logging). For this, we can use the doctrine-cache-bundle, which is a dependecy as well and then just use it's service. If somebody wants to use a different cache adapter, it's really easy to change as the template is already there for the filesystem.

Tobion · 2015-12-03T23:17:38Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/validator.xml

@@ -47,7 +48,6 @@
                    </call>
                </service>
            </argument>
-            <deprecated>The "%service_id%" service is deprecated since Symfony 2.8 and will be removed in 3.0.</deprecated>


Even if this was unintentionally deprecated. I'd like to keep it that way, so it can stay removed in 3.0. I'm repeating myself, it doesn't make sense to only define the apc one but none else. This is only because of legacy reasons when APC was allaround and the defacto standard. But today apc is the least common cache I would say.

We can't remove it in 3.0 now, so let's revisit this later in 3.0 (see if we want to deprecate it there).

Nevermind, this is not in 3.0 - my mistake. But for the purposes of getting this merged in a timely manner, I still think it's best to add it back and then decide to deprecate it in 3.0 (for 4.0 removal) - i.e. keep the PR as it's written.

dunglas · 2015-12-04T23:26:34Z

@Tobion, most loggers including Monolog can be configured to log on stderr. On Heroku, LogPlex automatically catches and centralizes logs outputted that way.

IMO it's better to keep default APCu implementations as they are the best for such jobs: #16838 (comment)

Tobion · 2015-12-04T23:47:57Z

@dunglas I don't see the relation to loggers?

dunglas · 2015-12-04T23:57:25Z

@Tobion it was about "filesystem access required by default anyway due to logging" but I maybe I've missed something.

Tobion · 2015-12-05T00:02:05Z

I see but again, I was talking about the standard-edition, which currently logs to a filesystem by default. That is why I said, we should IMO also configure filesystem caches in SE for most stuff by default as well, e.g. your property-accessor cache, validator etc. If people want to use APC or anything else, they only need to change the service definition very easily (or doctrine-cache-bundle config).

stof · 2015-12-05T10:00:36Z

It is too late to add such deprecation in 2.8 anyway, given that 2.8 and 3.0 are already released.

Tobion · 2015-12-05T10:57:57Z

@stof it's not too late because 3.0 is broken with ApcCache. See #16795

jderusse · 2015-12-06T21:27:31Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/validator.xml

@@ -37,6 +37,7 @@

        <service id="validator.mapping.cache.apc" class="%validator.mapping.cache.apc.class%" public="false">
            <argument>%validator.mapping.cache.prefix%</argument>
+            <deprecated>The "%service_id%" service is deprecated since Symfony 2.8 and will be removed in 3.0.</deprecated>


Is it Symfony 2.8 or 2.5 as described by @wouterj in https://github.com/symfony/symfony/pull/16865/files#diff-c2459e2c1a786adcaaab00b70630b67fR39

The ApcCache class is actually deprecated since 2.5, but the deprecation tag was added on the service in 2.8 as a feature (#16011). So I guess it's still correct to fix this in the 2.8 branch.

Anyway, I'll update the deprecated tag to mention the right version (as it has been done for other services in #16011). Thanks.

sstok · 2015-12-08T10:32:27Z

UPGRADE-2.8.md

+   ```yaml
+   # app/config.services.yml
+   services:
+     app.validator.cache.filesystem:


Wrong indention, should be with 4 spaces not two.

Fixed. Thank you.

dunglas · 2015-12-10T07:30:15Z

👎 we need a decent cache system in the core or it will be painful to get correct performance with many components (including - at least - Validator, Serializer, PropertyInfo and PropertyAccess) in prod. This is a regression.

ogizanagi · 2015-12-15T10:23:43Z

Second commit removed to finally keep the the APCu cache service in 3.0.
To be eventually deprecated in 3.1 and removed in 4.0, but for now, only fix the deprecation on the wrong service.

layanto · 2015-12-16T06:23:23Z

Now that there is ApcuCache in doctrine cache (doctrine/cache#115) and ApcCache deprecated, should validator.mapping.cache.doctrine.apc becomes validator.mapping.cache.doctrine.apcu?

dunglas · 2015-12-16T06:40:40Z

@layanto it will be a BC break. I'm working on APCu support. Except a PR when this one will be merged.

dunglas · 2015-12-16T06:43:06Z

👍

fabpot · 2015-12-28T09:11:52Z

👍

xabbuh · 2015-12-28T12:22:36Z

👍

layanto · 2015-12-28T21:16:08Z

Dumb question, but since symfony requires doctrine\common and doctrine\common requires doctrine\cache, why not just retain the use of validator.mapping.cache.apc but point to DoctrineCache instead of ApcCache? Still using apc just via doctrine\cache so not really BC break? No change required from users who use apc for validator as apc will continue to work, via doctrine\cache.

layanto · 2015-12-28T21:18:42Z

Benefit of above: no change required from user, shorter and consistent naming with serializer.mapping.cache.apc which already points to DoctrineCache.

weaverryan · 2016-01-16T20:25:16Z

👍

In summary, this:

Deprecates using apc as the cache key - you must use a service
Re-adds the validator.mapping.cache.doctrine.apc service, so you can use it for convenience (and we will discuss later if we want to deprecate this service for 4.0 removal).

After this is merged into 3.0, the ability to pass apc as the key to cache can be completely removed (https://github.com/symfony/symfony/pull/16795/files#diff-5e7347edce37c5886ec67b7ba02f3a9cL439). This is not a BC break, because it's impossible to use this already on 3.0, because using this config key gives you the validator.mapping.cache.apc service, which is connected with the ApcCache that no longer exists in 3.0: https://github.com/symfony/symfony/pull/16795/files#diff-c2459e2c1a786adcaaab00b70630b67fL31

So, we need to:

A) merge this PR (and merge up to 3.0)
B) remove the Configuration code that allowed you to use the apc key in 3.0 (e.g. cherry-pick https://github.com/symfony/symfony/pull/16795/files#diff-5e7347edce37c5886ec67b7ba02f3a9cL439).

And I think we're ready to do this now.

fabpot · 2016-01-19T11:26:45Z

What about @Tobion comments?

ogizanagi · 2016-01-19T15:20:17Z

@fabpot : Those comments are "outdated" after the decisions taken in this PR & #16795 (I answered to them in first place, but then remove the commit fixing it, in order to deprecate the apc implementations in 3.1 and remove them in 4.0).

@Tobion : Can you please validate the situation ? :)

fabpot · 2016-01-21T07:28:19Z

Thank you @ogizanagi.

…tion (ogizanagi) This PR was merged into the 2.8 branch. Discussion ---------- [FrameworkBundle][Validator] Fix apc cache service deprecation | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Related to #16795 I guess the deprecation was on the wrong service. Also, no deprecation notice was triggered about using `"apc"` as the value of the `framework.validation.cache` configuration option. This PR adds the missing deprecation. > 📝 _NOTE_: The standard edition will need to be updated [here](https://github.com/symfony/symfony-standard/blob/2.8/app/config/config_prod.yml#L6). Commits ------- 907bbec [FrameworkBundle][Validator] Fix apc cache service deprecation

…g (ogizanagi) This PR was merged into the 3.0 branch. Discussion ---------- [FrameworkBundle][Validator] Fix apc cache service & config | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #16793 | License | MIT | Doc PR | - _Keep track of #16794 (comment) _NOTE_: This PR is on standby. If #16822 is merged, this one might probably be closed, as everything will be done during the merge. Commits ------- 94a1728 [FrameworkBundle][Validator] Fix apc cache service & config

This PR was submitted for the master branch but it was merged into the 2.8 branch instead (closes #935). Discussion ---------- Fixed the suggested validation cache driver In reference to symfony/symfony#16822. Commits ------- a795660 Fixed the suggested validation cache driver

…me (tgalopin) This PR was merged into the 3.1-dev branch. Discussion ---------- [FrameworkBundle][Serializer] Fix APC cache service name | Q | A | ------------- | --- | Branch? | master or 3.0 (not sure) | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - In the commit symfony/symfony-standard@0080556, we introduced in the standard edition the usage of `serializer.mapping.cache.doctrine.apc` instead of `serializer.mapping.cache.apc` in `config_prod.yml` comments. Earlier, we introduced the validator equivalent modification (`validator.mapping.cache.doctrine.apc` instead of `validator.mapping.cache.apc`) but while we adapted the validator configuration in the FrameworkBundle in #16822, we did not adapt the FrameworkBundle configuration for the serializer. I tested the current master of symfony-standard and it's indeed failing: ``` [Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException] The service "serializer" has a dependency on a non-existent service "serializer.mapping.cache.doctrine.apc". ``` This PR renames the serializer APCu cache service name to fix this issue. However, I'm not sure when the validator cache service modification was merged and released so I'm not sure how this PR should handle this. Is this a bug? Or is this a new feature and we should trigger a depreciation but keep the service `serializer.mapping.cache.apc` usable? Commits ------- 88ef89c [FrameworkBundle][Serializer] Fix APC cache service name and deprecate old name

carsonbot added the Status: Needs Review label Dec 3, 2015

ogizanagi mentioned this pull request Dec 3, 2015

[FrameworkBundle][Validator] Fix apc cache service & config #16795

Merged

Tobion reviewed Dec 3, 2015
View reviewed changes

This was referenced Dec 4, 2015

Use Apcu_fetch instead of apc_fetch #16830

Closed

[PropertyAccess] Add PSR-6 cache #16838

Closed

Tobion mentioned this pull request Dec 5, 2015

[2.8] Deprecate the correct APC cache service #16865

Closed

jderusse reviewed Dec 6, 2015
View reviewed changes

[FrameworkBundle][Validator] Fix apc cache service deprecation

907bbec

sstok reviewed Dec 8, 2015
View reviewed changes

dunglas mentioned this pull request Dec 16, 2015

[FrameworkBundle] Add APCu support for the Serializer #17025

Closed

ogizanagi mentioned this pull request Jan 9, 2016

[Validator] Remove pre 3.0 ApcCache leftovers #17317

Closed

fabpot merged commit 907bbec into symfony:2.8 Jan 21, 2016

ogizanagi deleted the fix_validator_apc_service_deprecation branch January 21, 2016 10:34

peterrehm mentioned this pull request Jan 25, 2016

Fixed the suggested validation cache driver symfony/symfony-standard#935

Closed

fabpot mentioned this pull request Feb 3, 2016

Release v3.0.2 #17665

Merged

fabpot mentioned this pull request Feb 28, 2016

Release v2.8.3 #17952

Merged

tgalopin mentioned this pull request Apr 16, 2016

[FrameworkBundle][Serializer] Fix APC cache service name #18567

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FrameworkBundle][Validator] Fix apc cache service deprecation #16822

[FrameworkBundle][Validator] Fix apc cache service deprecation #16822

ogizanagi commented Dec 3, 2015

Tobion commented Dec 3, 2015

ogizanagi commented Dec 3, 2015

Tobion commented Dec 3, 2015

Tobion Dec 3, 2015

weaverryan Jan 16, 2016

weaverryan Jan 16, 2016

dunglas commented Dec 4, 2015

Tobion commented Dec 4, 2015

dunglas commented Dec 4, 2015

Tobion commented Dec 5, 2015

stof commented Dec 5, 2015

Tobion commented Dec 5, 2015

jderusse Dec 6, 2015

ogizanagi Dec 6, 2015

sstok Dec 8, 2015

ogizanagi Dec 8, 2015

dunglas commented Dec 10, 2015

ogizanagi commented Dec 15, 2015

layanto commented Dec 16, 2015

dunglas commented Dec 16, 2015

dunglas commented Dec 16, 2015

fabpot commented Dec 28, 2015

xabbuh commented Dec 28, 2015

layanto commented Dec 28, 2015

layanto commented Dec 28, 2015

weaverryan commented Jan 16, 2016

fabpot commented Jan 19, 2016

ogizanagi commented Jan 19, 2016

fabpot commented Jan 21, 2016

[FrameworkBundle][Validator] Fix apc cache service deprecation #16822

[FrameworkBundle][Validator] Fix apc cache service deprecation #16822

Conversation

ogizanagi commented Dec 3, 2015

Tobion commented Dec 3, 2015

ogizanagi commented Dec 3, 2015

Tobion commented Dec 3, 2015

Tobion Dec 3, 2015

Choose a reason for hiding this comment

weaverryan Jan 16, 2016

Choose a reason for hiding this comment

weaverryan Jan 16, 2016

Choose a reason for hiding this comment

dunglas commented Dec 4, 2015

Tobion commented Dec 4, 2015

dunglas commented Dec 4, 2015

Tobion commented Dec 5, 2015

stof commented Dec 5, 2015

Tobion commented Dec 5, 2015

jderusse Dec 6, 2015

Choose a reason for hiding this comment

ogizanagi Dec 6, 2015

Choose a reason for hiding this comment

sstok Dec 8, 2015

Choose a reason for hiding this comment

ogizanagi Dec 8, 2015

Choose a reason for hiding this comment

dunglas commented Dec 10, 2015

ogizanagi commented Dec 15, 2015

layanto commented Dec 16, 2015

dunglas commented Dec 16, 2015

dunglas commented Dec 16, 2015

fabpot commented Dec 28, 2015

xabbuh commented Dec 28, 2015

layanto commented Dec 28, 2015

layanto commented Dec 28, 2015

weaverryan commented Jan 16, 2016

fabpot commented Jan 19, 2016

ogizanagi commented Jan 19, 2016

fabpot commented Jan 21, 2016