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

Remove support for Doctrine Cache #4626

Merged
merged 1 commit into from Jun 7, 2021

Conversation

derrabus
Copy link
Member

Q A
Type improvement
BC Break yes
Fixed issues N/A

Summary

This PR resolves the deprecations introduced by #4620 and #4624.

@derrabus derrabus marked this pull request as draft April 29, 2021 19:03
@greg0ire greg0ire added this to the 4.0.0 milestone Apr 29, 2021
@morozov morozov added the Cache label May 9, 2021
@morozov
Copy link
Member

morozov commented Jun 6, 2021

@derrabus I believe you can proceed here.

@derrabus derrabus marked this pull request as ready for review June 6, 2021 19:14
@derrabus
Copy link
Member Author

derrabus commented Jun 6, 2021

The PR is ready. 🙂

Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

Please also adapt caching.rst in the docs (https://github.com/doctrine/dbal/blob/4.0.x/docs/en/reference/caching.rst) where $config->setResultCacheImpl($cache); is used in an example.

@derrabus
Copy link
Member Author

derrabus commented Jun 6, 2021

@SenseException The documentation will be fixed with #4652. Let's keep it out of this PR.

@morozov
Copy link
Member

morozov commented Jun 6, 2021

Let's keep documentation in sync with the code within each commit. Since the PR removes a method, it must remove it from the documentation as well.

@greg0ire
Copy link
Member

greg0ire commented Jun 7, 2021

Since that method is deprecated, the documentation can be removed before the method itself is removed, so if we merge #4652 , there will be nothing to remove (unless there are more occurrences than the one in #4652 ?). Let's avoid the necessary conflicts, and block this PR until #4652 is merged up?

@greg0ire

This comment has been minimized.

greg0ire
greg0ire previously approved these changes Jun 7, 2021
@morozov
Copy link
Member

morozov commented Jun 7, 2021

I still see that setResultCacheImpl() is referenced in the documentation although it's being removed in this PR.

SenseException
SenseException previously approved these changes Jun 7, 2021
Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

@morozov The changes in caching.rst are already merged and up-merged into 4.0.x. Or do you refer to other occurrences of the method calls?

@morozov
Copy link
Member

morozov commented Jun 7, 2021

I was referring to

<?php
$cache = new \Doctrine\Common\Cache\ArrayCache();
$config = $conn->getConfiguration();
$config->setResultCacheImpl($cache);

If this has been addressed in 4.0.x, please feel free to merge.

@derrabus
Copy link
Member Author

derrabus commented Jun 7, 2021

I've rebased my PR onto 4.0.x. The docs should be correct now. 🙂

@morozov morozov merged commit 08efef7 into doctrine:4.0.x Jun 7, 2021
@morozov
Copy link
Member

morozov commented Jun 7, 2021

Thanks, @derrabus!

@derrabus derrabus deleted the remove/doctrine-cache branch June 7, 2021 23:15
@morozov morozov mentioned this pull request Oct 17, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants