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

Fix: use array adapter cache pools by default #1073

Merged
merged 1 commit into from Nov 27, 2019

Conversation

dmaicher
Copy link
Contributor

@dmaicher dmaicher commented Nov 26, 2019

This fixes #1069 and #1066

It restores the previous behavior from 1.11 where we use an array cache by default if no other cache is configured.

The cache pools will also show up in the profiler:

pools

TODOs:

default:
return 'cache.app';
}
$id = sprintf('cache.doctrine.orm.%s.%s', $objectManagerName, str_replace('_cache', '', $cacheName));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

every object-manager needs separate caches, right? Otherwise there could be weird key collisions I think

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what the EM uses as key for the entry, but this is definitely safer than using shared caches.

@dmaicher dmaicher marked this pull request as ready for review November 26, 2019 19:23
DependencyInjection/DoctrineExtension.php Outdated Show resolved Hide resolved
DependencyInjection/DoctrineExtension.php Outdated Show resolved Hide resolved
@greg0ire
Copy link
Member

Please kindly squash your commits together. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble.

How to do that?

  1. git rebase -i origin/1.12.x, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin/whatever-you-call-it repository.
  2. A window will show up with many lines, replace pick with fixup on every line but the first one
  3. Close your editor, git should do its magic, and you should end up with one commit
  4. Use git push --force to overwrite what you already push. Don't forget the --force option otherwise git will try to merge both things together.

Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

The separate cache pools are a really nice touch 👍

Copy link
Contributor

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

Nice work David 👌🏻

@alcaeus
Copy link
Member

alcaeus commented Nov 27, 2019

@dmaicher build is failing due to a deprecation in the 4.4 job. Do you want to take a look at that in a separate PR? Otherwise I'll do so in the afternoon.

@alcaeus alcaeus added this to 1.12 (bug fixes only) in Roadmap Nov 27, 2019
@dmaicher dmaicher force-pushed the fix_default_cache_pool branch 2 times, most recently from b8c1378 to 33b4cca Compare November 27, 2019 08:02
@dmaicher
Copy link
Contributor Author

@dmaicher build is failing due to a deprecation in the 4.4 job. Do you want to take a look at that in a separate PR? Otherwise I'll do so in the afternoon.

@alcaeus yeah I also saw it. Not sure I have time for it today 😢

@alcaeus
Copy link
Member

alcaeus commented Nov 27, 2019

Don't worry: #1075. Just need to rebase :)

@alcaeus alcaeus merged commit 1035bdd into doctrine:1.12.x Nov 27, 2019
@alcaeus
Copy link
Member

alcaeus commented Nov 27, 2019

Thanks David!

@dmaicher
Copy link
Contributor Author

Here is the recipe change: symfony/recipes#695

@dmaicher
Copy link
Contributor Author

@alcaeus @greg0ire @nicolas-grekas when we release this it will possibly cause some issues for users that don't also sync the latest recipe change (symfony/recipes#695)

It will leave them without proper caching for prod environments 😢 Not sure how we can avoid this though...

But maybe chances are high that new projects started with fresh 1.12/2.0 recipes are not running in production yet? 😄 🙈

@OskarStark
Copy link
Contributor

I think we cannot avoid it, but before it was an implicit dep on my project, so I ended up automatically using 2.0 via composer update. I had to manually require 1.x. But nevermind we need to fix it and help the Community to get a working state back again 👍🏻

@alcaeus
Copy link
Member

alcaeus commented Nov 28, 2019

Fixing this is not a question of if, but how. The current upgrade path may cause some pain for people in production if they don't upgrade their flex recipes or don't update their config manually, but I believe we can account for that by communicating the change appropriately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Roadmap
1.12 (bug fixes only)
Development

Successfully merging this pull request may close these issues.

None yet

4 participants