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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: 1.12
Choose a base branch
from
Open

Cache provider fix #408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c6cc531
Change doctrine config in prod environment
liszkapawel Sep 23, 2023
e8de6ba
Change doctrine config in test_cached environment
liszkapawel Sep 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 16 additions & 11 deletions tests/Application/config/packages/prod/doctrine.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,32 @@
doctrine:
orm:
metadata_cache_driver:
type: service
id: doctrine.system_cache_provider
query_cache_driver:
type: service
id: doctrine.system_cache_provider
result_cache_driver:
type: service
id: doctrine.result_cache_provider
entity_managers:
Copy link
Member

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

Copy link
Member

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?

default:
metadata_cache_driver:
type: service
id: doctrine.system_cache_provider
query_cache_driver:
type: service
id: doctrine.system_cache_provider
result_cache_driver:
type: service
id: doctrine.result_cache_provider

services:
doctrine.result_cache_provider:
class: Symfony\Component\Cache\DoctrineProvider
class: Doctrine\Common\Cache\Psr6\DoctrineProvider
Copy link
Member

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

public: false
factory: ['Doctrine\Common\Cache\Psr6\DoctrineProvider', 'wrap']
arguments:
- '@doctrine.result_cache_pool'
doctrine.system_cache_provider:
class: Symfony\Component\Cache\DoctrineProvider
class: Doctrine\Common\Cache\Psr6\DoctrineProvider
public: false
factory: [ 'Doctrine\Common\Cache\Psr6\DoctrineProvider', 'wrap' ]
arguments:
- '@doctrine.system_cache_pool'


Copy link
Member

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

framework:
cache:
pools:
Expand Down
41 changes: 30 additions & 11 deletions tests/Application/config/packages/test_cached/doctrine.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,34 @@ doctrine:
orm:
entity_managers:
default:
result_cache_driver:
type: memcached
host: localhost
port: 11211
query_cache_driver:
type: memcached
host: localhost
port: 11211
metadata_cache_driver:
type: memcached
host: localhost
port: 11211
type: service
Copy link
Member

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.

id: doctrine.system_cache_provider
query_cache_driver:
type: service
id: doctrine.system_cache_provider
result_cache_driver:
type: service
id: doctrine.result_cache_provider

services:
doctrine.result_cache_provider:
class: Doctrine\Common\Cache\Psr6\DoctrineProvider
public: false
factory: ['Doctrine\Common\Cache\Psr6\DoctrineProvider', 'wrap']
arguments:
- '@doctrine.result_cache_pool'
doctrine.system_cache_provider:
class: Doctrine\Common\Cache\Psr6\DoctrineProvider
public: false
factory: [ 'Doctrine\Common\Cache\Psr6\DoctrineProvider', 'wrap' ]
arguments:
- '@doctrine.system_cache_pool'

framework:
cache:
pools:
doctrine.result_cache_pool:
adapter: cache.app
doctrine.system_cache_pool:
adapter: cache.system