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 configuring caches through DoctrineCacheBundle #981

Merged

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Jun 11, 2019

With this PR, we are deprecating configuring cache drivers for ORM through DoctrineCacheBundle, as the bundle is deprecated (see doctrine/DoctrineCacheBundle#156). Instead of assuming array as default, the bundle now uses an appropriate cache pool (app or system depending on the cache in question) if no explicit configuration was given. This avoids deprecation notices for people that did not add any cache configuration.

Caches should either be configured by passing a symfony/cache pool or service ID. The latter allows using doctrine/cache classes, but without the comfort of configuring the caches through bundle config. The implicit requirement on doctrine/cache-bundle will be removed in DoctrineBundle 2.0.

@alcaeus alcaeus added this to the 1.12.0 milestone Jun 11, 2019
@alcaeus alcaeus self-assigned this Jun 11, 2019
@alcaeus alcaeus changed the base branch from master to 1.12.x June 11, 2019 20:59
@alcaeus alcaeus force-pushed the deprecate-doctrine-cache-bundle-support branch from 8dd300b to 0c2bd37 Compare June 11, 2019 21:10
@alcaeus
Copy link
Member Author

alcaeus commented Jun 11, 2019

Coverage tests are broken because of a deprecation:

1x: The "Symfony\Component\HttpKernel\EventListener\TranslatorListener" class is deprecated since Symfony 4.3 and will be removed in 5.0, use LocaleAwareListener instead.
    1x in ImportMappingDoctrineCommandTest::setup from Doctrine\Bundle\DoctrineBundle\Tests\Command

Can't figure out what's using it though, especially since this doesn't happen in any of the other builds. Could this be a problem because of the code coverage whitelist?

Edit: also can't reproduce the failure locally 🤷‍♂️

@alcaeus alcaeus force-pushed the deprecate-doctrine-cache-bundle-support branch from 0c2bd37 to 4615b7f Compare June 11, 2019 21:32
@SenseException
Copy link
Member

The service of the event-subscriber is called depending on some configuration.

https://github.com/symfony/framework-bundle/blob/v4.3.1/DependencyInjection/FrameworkExtension.php#L1085

It seems to depend on !$this->isConfigEnabled($container, $config) above. Can you confirm that? Is there an enabled or disabled translator?

@alcaeus alcaeus force-pushed the deprecate-doctrine-cache-bundle-support branch 2 times, most recently from a80fd3e to 4615b7f Compare June 12, 2019 10:17
@alcaeus
Copy link
Member Author

alcaeus commented Jun 12, 2019

Turns out the reason for the failure was that the coverage tests installed symfony/messenger in version 4.2. This also locks the FrameworkBundle to 4.2, which still uses the now deprecated class. Locally, I had symfony/messenger 4.3 installed, which doesn't cause the issue. I've fixed .travis.yml in #983 which will be merged up soon.

@alcaeus alcaeus force-pushed the deprecate-doctrine-cache-bundle-support branch from a80fd3e to 94552bb Compare June 12, 2019 10:27
@alcaeus alcaeus force-pushed the deprecate-doctrine-cache-bundle-support branch from 94552bb to 8c6874c Compare July 14, 2019 20:11
@alcaeus alcaeus requested review from xabbuh and stof July 14, 2019 20:13
@alcaeus alcaeus requested review from a team and removed request for stof and xabbuh July 22, 2019 19:28
composer.json Outdated Show resolved Hide resolved
DependencyInjection/DoctrineExtension.php Outdated Show resolved Hide resolved
@stof
Copy link
Member

stof commented Jul 24, 2019

the rule for marked a test as @group legacy (and so allowing it to use the BC layer to test it) is that this test gets removed when removing the corresponding deprecated feature (or converted to a test expected an exception, if the deprecation is meant to be replaced by an exception forbidding a given case). Many tests here should be adapted to keep covering different parts of the bundle without using deprecated APIs.

@alcaeus alcaeus force-pushed the deprecate-doctrine-cache-bundle-support branch 3 times, most recently from 2663cd0 to 389289a Compare July 30, 2019 20:24
@alcaeus
Copy link
Member Author

alcaeus commented Jul 30, 2019

@stof you're right - I didn't think of those. I've updated the test and included some legacy tests to test existing functionality. There may be a few more changes necessary once we drop old stuff, but that's alright.

@alcaeus alcaeus force-pushed the deprecate-doctrine-cache-bundle-support branch 2 times, most recently from fd040a3 to e10438f Compare July 30, 2019 20:58
@alcaeus alcaeus force-pushed the deprecate-doctrine-cache-bundle-support branch from e10438f to 0dc30c8 Compare August 24, 2019 09:25
@alcaeus alcaeus merged commit 8766458 into doctrine:1.12.x Aug 24, 2019
@alcaeus alcaeus deleted the deprecate-doctrine-cache-bundle-support branch August 24, 2019 10:26
@nanasess nanasess mentioned this pull request Nov 27, 2019
13 tasks
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.

5 participants