Skip to content
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

Deprecate Validator's CacheInterface in favor of PSR-6 #33414

Closed
nicolas-grekas opened this issue Sep 1, 2019 · 6 comments

Comments

@nicolas-grekas
Copy link
Member

commented Sep 1, 2019

We don't need the Symfony\Component\Validator\Mapping\Cache\Interface nor any of its implementations, isn't it?

Let's deprecate them all and use PSR-6 directly instead.

@nicolas-grekas nicolas-grekas created this issue from a note in Help Wanted (Nicolas') Sep 1, 2019

@xabbuh xabbuh added the Validator label Sep 2, 2019

@fancyweb

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

Symfony\Component\Validator\ValidatorBuilder::setMetadataCache() (public method from a not internal / final class) use it as its mandatory argument type. How to stay backward compatible and still be able to be "deprecation free" on 4.4 in such a case? 🤔

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

deprecate the method in favor of a new setMappingCache()?

@fancyweb

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

This trick again 😬 My concern with it is that the current method name is just good, there is no benefit in changing it (like for a better understanding for example). However, if it's the only solution...

@derrabus

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

deprecate the method in favor of a new setMappingCache()?

Just setCache()? After all, the metadata cache is the only cache that the builder knows about. Also, ValidatorBuilder should be finalized, imho. That would make future changes to that class easier and people could still decorate the class if they feel the need for extending its functionality.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2019

the cache operates in the Mapping namespace, I think it's better to hint about the intent with the name
making the class @final LGTM

@derrabus

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

nicolas-grekas added a commit that referenced this issue Sep 5, 2019
feature #33459 [Validator] Deprecated CacheInterface in favor of PSR-…
…6 (derrabus)

This PR was merged into the 4.4 branch.

Discussion
----------

[Validator] Deprecated CacheInterface in favor of PSR-6

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #33414
| License       | MIT
| Doc PR        | TODO

With PSR-6, the validator component does not need its own `CacheInterface`, so let's remove it.

Note that I still use the now deprecated `Psr6Cache` class to keep the logic inside `LazyLoadingMetadataFactory` simple. My plan would be to inline all logic from `Psr6Cache` into `LazyLoadingMetadataFactory` for 5.0.

Commits
-------

0b08040 [Validator] Deprecated CacheInterface in favor of PSR-6.
nicolas-grekas added a commit that referenced this issue Sep 5, 2019
minor #33460 [Validator] Removed CacheInterface in favor of PSR-6 (de…
…rrabus)

This PR was merged into the 5.0-dev branch.

Discussion
----------

[Validator] Removed CacheInterface in favor of PSR-6

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #33414
| License       | MIT
| Doc PR        | symfony/symfony-docs#12276

This PR removes the deprecation layer of #33459.

Commits
-------

34b141c [Validator] Removed CacheInterface in favor of PSR-6.

@xabbuh xabbuh closed this Sep 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.