-
Notifications
You must be signed in to change notification settings - Fork 64
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
Cache provider fix #408
base: 1.12
Are you sure you want to change the base?
Cache provider fix #408
Conversation
result_cache_driver: | ||
type: service | ||
id: doctrine.result_cache_provider | ||
entity_managers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both old "shortened configuration" and new syntax seems to be valid. https://symfony.com/doc/current/reference/configuration/doctrine.html#shortened-configuration-syntax
Yes, it would be good to use the same syntax for prod/doctrine.yaml
and test_cached/doctrine.yaml
, but this quirk is re-used across multiple repositories
So unless there is a functional need for this, let's keep it as it was for the sake of cross-repo maintainability
https://github.com/Sylius/InvoicingPlugin/blob/ac4ce3140d712ad1545f0330c2d2701e5bd31187/tests/Application/config/packages/prod/doctrine.yaml#L3-L11C47
https://github.com/Sylius/PluginTemplate/blob/b0730db3370132f44be5fdbf449a99a4cb720c22/tests/Application/config/packages/prod/doctrine.yaml#L3-L11
https://github.com/Sylius/PayPalPlugin/blob/8b1be9676c0e4dc8d865317c8ffb1c0ce6b4224d/tests/Application/config/packages/prod/doctrine.yaml#L3-L11
https://github.com/Sylius/PriceHistoryPlugin/blob/dd5eb32299ed6475b2ed7e27c010e207155e3153/tests/Application/config/packages/prod/doctrine.yaml#L3-L11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should upgrade to when@prod
syntax for multi env configs at a single file, which is already available to us https://symfony.com/blog/new-in-symfony-5-3-configure-multiple-environments-in-a-single-file
But it needs to be scheduled for all repos. @Sylius/core-team WDYT?
arguments: | ||
- '@doctrine.system_cache_pool' | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove extra empty line
|
||
services: | ||
doctrine.result_cache_provider: | ||
class: Symfony\Component\Cache\DoctrineProvider | ||
class: Doctrine\Common\Cache\Psr6\DoctrineProvider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which version of doctrine/cache
do you use? Sylius 1.12 is dependant on ^1.10
, while current Sylius 1.13 (unreleased) is ^2.2
, so I've a feeling this shouldn't have been an issue yet as this upgrade is only mentioned for 2.2
(But I didn't try for myself) https://www.doctrine-project.org/projects/doctrine-cache/en/2.2/index.html#use-with-psr-6
type: memcached | ||
host: localhost | ||
port: 11211 | ||
type: service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even with PSR6 cache upgrade, I think it should have stayed with memcached
, though I never looked at this part too closely before.
@jakubtobiasz should know more.
It looks like the plugin skeleton has an outdated doctrine configuration for the production and test_cached environment
Refs: #407