Skip to content

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

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

Merged

Conversation

tgalopin
Copy link
Contributor

@tgalopin tgalopin commented Apr 16, 2016

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?

@tgalopin
Copy link
Contributor Author

@dunglas: do you know what happened after your comment: #16795 (comment) ?

@nicolas-grekas
Copy link
Member

ping @wouterj

@wouterj
Copy link
Member

wouterj commented Apr 17, 2016

The validator service was renamed because the Validator component switched from using their own ApcCache class to Doctrine Cache library in Symfony 2.5. To be able to provide a clean upgrade path, the new service was added in 2.8 (refs #14429).

The serializer service was added in Symfony 2.7 (refs #13107). As it directly used doctrine cache, I think it should have been named serializer.mapping.cache.doctrine.apc for consistency. However, this was not done.

I think it makes sense to do this renaming now. Just like all other renamings, the old service should be deprecated in favor of the new service and it should be merged into master (probably 3.1). The old service should then be removed in Symfony 4.

Meanwhile, a PR should be submitted to the 2.8 branch of the Standard Edition to revert symfony/symfony-standard@0080556 This was a mistake (and it shows the need for consistency btw).

@tgalopin tgalopin force-pushed the serializer_apc_cache_service branch from bd84187 to 7d5e658 Compare April 18, 2016 16:10
@tgalopin
Copy link
Contributor Author

I update this PR to have the behavior @wouterj recomanded. I'm going to create the 2.8 PR.

@tgalopin tgalopin force-pushed the serializer_apc_cache_service branch 3 times, most recently from 3c6016d to 6293226 Compare April 18, 2016 17:55
@tgalopin
Copy link
Contributor Author

I think this is ready, targeting 3.1.

@nicolas-grekas
Copy link
Member

👍 for 3.1

@wouterj
Copy link
Member

wouterj commented Apr 18, 2016

Status: reviewed

Looks good. Can you please also update the CHANGELOG.md file in the FrameworkBundle and UPGRADE-3.1.md and UPGRADE-4.0.md to explain that this service is renamed?

@tgalopin
Copy link
Contributor Author

I'll do that tomorrow. Thanks for your review!

@tgalopin tgalopin force-pushed the serializer_apc_cache_service branch from 6293226 to d6b1f5d Compare April 19, 2016 06:24
@tgalopin
Copy link
Contributor Author

I did the change.

@@ -83,6 +83,11 @@ FrameworkBundle
- `"form.type.submit"`
- `"form.type.reset"`

* The service `serializer.mapping.cache.apc` has been renamed to
Copy link
Member

Choose a reason for hiding this comment

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

[...] has been deprecated in favor of [...]

@tgalopin tgalopin force-pushed the serializer_apc_cache_service branch from d6b1f5d to 5348344 Compare April 19, 2016 06:31
@tgalopin tgalopin force-pushed the serializer_apc_cache_service branch from 5348344 to 88ef89c Compare April 19, 2016 06:52
@tgalopin
Copy link
Contributor Author

I'm not sure what the PR in 2.8 should look like. Shouldn't we apply the same PR as this one on 2.8? The validator cache service was renamed in 2.8, we could rename the serializer in 2.8 too couldn't we?

@xabbuh
Copy link
Member

xabbuh commented Apr 19, 2016

@tgalopin It's about updating the commented config in the Symfony Standard Edition where we use the wrong service id: https://github.com/symfony/symfony-standard/blob/2.8/app/config/config_prod.yml#L8

@dunglas
Copy link
Member

dunglas commented Apr 19, 2016

What do you think about deprecating the class itself and recommending the PSR-6 backend instead?

@nicolas-grekas
Copy link
Member

@dunglas IMHO it's too early to deprecate for 3.1, but for sure we should consider deprecating once we're used to PSR-6 caching and we're sure this wouldn't prevent any valid use case (which needs some time to me to be confirmed).

@dunglas
Copy link
Member

dunglas commented Apr 19, 2016

So 👍 for this change.

@tgalopin
Copy link
Contributor Author

The failure is unrelated.

@xabbuh
Copy link
Member

xabbuh commented Apr 19, 2016

👍

@nicolas-grekas
Copy link
Member

Thank you @tgalopin.

@nicolas-grekas nicolas-grekas merged commit 88ef89c into symfony:master Apr 19, 2016
nicolas-grekas added a commit that referenced this pull request Apr 19, 2016
…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
@xabbuh
Copy link
Member

xabbuh commented Apr 22, 2016

@tgalopin Just noticed that we already have a PR for the Standard Edition (see symfony/symfony-standard#949). So no need to do anything there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants