-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FrameworkBundle][Validator] Fix apc cache service deprecation #16822
[FrameworkBundle][Validator] Fix apc cache service deprecation #16822
Conversation
IMO both |
@Tobion : If we remove those services accordingly to what you say, then we should remove the 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. Some refs about related discussions: |
@ogizanagi yes I would suggest to deprecate 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. |
@@ -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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@Tobion, most loggers including Monolog can be configured to log on IMO it's better to keep default APCu implementations as they are the best for such jobs: #16838 (comment) |
@dunglas I don't see the relation to loggers? |
@Tobion it was about "filesystem access required by default anyway due to logging" but I maybe I've missed something. |
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). |
It is too late to add such deprecation in 2.8 anyway, given that 2.8 and 3.0 are already released. |
@@ -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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it Symfony 2.8 or 2.5 as described by @wouterj in https://github.com/symfony/symfony/pull/16865/files#diff-c2459e2c1a786adcaaab00b70630b67fR39
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
```yaml | ||
# app/config.services.yml | ||
services: | ||
app.validator.cache.filesystem: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong indention, should be with 4 spaces not two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Thank you.
👎 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. |
Second commit removed to finally keep the the APCu cache service in 3.0. |
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? |
@layanto it will be a BC break. I'm working on APCu support. Except a PR when this one will be merged. |
👍 |
👍 |
1 similar comment
👍 |
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. |
Benefit of above: no change required from user, shorter and consistent naming with serializer.mapping.cache.apc which already points to DoctrineCache. |
👍 In summary, this:
After this is merged into 3.0, the ability to pass So, we need to: A) merge this PR (and merge up to 3.0) And I think we're ready to do this now. |
What about @Tobion comments? |
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
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 theframework.validation.cache
configuration option. This PR adds the missing deprecation.