NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[FrameworkBundle] Semantic config for app/system/pool caches#18667

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 2 commits intosymfony:masterfromnicolas-grekas:cache-config

May 4, 2016

Merged

[FrameworkBundle] Semantic config for app/system/pool caches#18667

fabpot merged 2 commits intosymfony:masterfromnicolas-grekas:cache-config

May 4, 2016

Conversation

Copy link

Member

nicolas-grekas commentedApr 28, 2016•
edited
Loading

Q	A
Branch?	3.1
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#18625
License	MIT
Doc PR	-

carsonbot added the Status: Needs Review label

Apr 28, 2016

Copy link

MemberAuthor

nicolas-grekas commentedApr 28, 2016•
edited
Loading

I'd happily accept some help to fix the XSD please, that should be the only remaining thing left.

nicolas-grekas mentioned this pull request

Apr 28, 2016

[FrameworkBundle] Add cache adapters in semantic configuration#18625

Closed

nicolas-grekas force-pushed thecache-config branch from09320b9 tobb8e5d7Compare

April 29, 2016 06:37

GuilhemN reviewed

Apr 29, 2016

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php Outdated

		->scalarNode('adapter')
		->info('The cache pool adapter service to use as template definition.')
		->defaultValue('cache.adapter.shared')
		->info('The create a new pool based on this adapter service.')

Copy link

Contributor

GuilhemNApr 29, 2016•
edited
Loading

Choose a reason for hiding this comment

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

should beTo create

Copy link

MemberAuthor

nicolas-grekas commentedApr 29, 2016

Hold on, I'm going to change a few things after some discussions with@javiereguiluz

nicolas-grekas force-pushed thecache-config branch frombb8e5d7 tob2df9d6Compare

April 29, 2016 19:10

Copy link

MemberAuthor

nicolas-grekas commentedApr 29, 2016•
edited
Loading

Following suggestions from@javiereguiluz and@fabpot, configuration is now the following:

Pre-configured cache pool services are now:cache.app,cache.system,cache.serializer andcache.validator. Cache adapters (abstract pools) are prefixed bycache.adapter.*.~~cache.adapter.local~~ is replaced bycache.system.~~cache.adapter.shared~~ is replaced bycache.app. They both can be reused as a parent to define new "app" or "system" pools reusing the same base adapter. They both default tocache.adapter.filesystem.

Configuration is made easy for those two, using full service names to define which adapter should be used for each of them.

E.g. to use APCu for all system caches:

framework:cache:system:cache.adapter.apcu

To use Redis for all app related caches:

framework:cache:app:cache.adapter.redisdefault_redis_provider:redis://1.2.3.4:5678

As you can see, this PR also adds a factory for creating Redis connection based on a DSN.

Then of course you can go to more advanced configurations to redefine either custom adapters or regular pools (pre-defined or not).

Adapter and pool configurations are merged in a singleframework.cache.pools option. Note that pool names should be given as full service names.

E.g. to define a new system cache pool accessible with$container->get('my_local_cache_pool'):

framework:cache:pools:my_local_cache_pool:adapter:cache.system# overwrite the default which is "cache.app"public:true# new cache pools are private by default

E.g. to use Redis for all system pools and usehttps://github.com/snc/SncRedisBundle to configure the connection:

framework:cache:system:cache.adapter.redisdefault_redis_provider:snc_redis_connection_service

a verbose variant of this could be:

framework:cache:system:app.redis_cachepools:app.redis_cache:adapter:cache.adapter.redisprovider:snc_redis_connection_service

Or to use a Doctrine Cache provider for all app related caches:

framework:cache:app:cache.adapter.doctrinedefault_doctrine_provider:doctrine_cache_provider_service

or verbose variant:

framework:cache:app:app.doctrine_cachepools:app.doctrine_cache:adapter:cache.adapter.doctrineprovider:doctrine_cache_provider_service

The same would go for configuring a pool on top of a PSR-6 cache adapter.

nicolas-grekas force-pushed thecache-config branch 4 times, most recently from22bd298 tob80a8ccCompare

April 29, 2016 21:58

nicolas-grekas added the Feature label

Apr 29, 2016

Copy link

MemberAuthor

nicolas-grekas commentedApr 29, 2016•
edited
Loading

Last tweaks pushed, green, ready for review+merge:

cache.system is now a public pool by default (alongside withcache.app)
those two special pools are reserved names in thepools config section: they must be configured through their dedicated options undercache (see examples above)
new pools are private by default to allow easily changing the adapter of predefined pools (cache.serializer &cache.validation today). IMHO creating custom pools is an even more advanced use case that won't suffer from having this requirement of addingpublic: true when needed.

nicolas-grekas force-pushed thecache-config branch 4 times, most recently fromeeb561e tof84e588Compare

April 30, 2016 08:57

nicolas-grekas changed the title~~[FrameworkBundle] Semantic configuration for redis/apcu/pools/adapters caches~~[FrameworkBundle] Semantic config for app/system/pool caches

Apr 30, 2016

nicolas-grekas force-pushed thecache-config branch 2 times, most recently from2318540 toa34a005Compare

April 30, 2016 09:20

xabbuh reviewed

Apr 30, 2016

View reviewed changes

src/Symfony/Component/Cache/Adapter/RedisAdapter.php Outdated

		if (preg_match('#[^-+_.A-Za-z0-9]#',$namespace,$match)) {
		thrownewInvalidArgumentException(sprintf('RedisAdapter namespace contains "%s" but only characters in [-+_.A-Za-z0-9] are allowed.',$match[0]));
		}
		if (!$redisConnection->isConnected()) {

Copy link

Member

xabbuhApr 30, 2016

Choose a reason for hiding this comment

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

Are you sure that it is a good idea to require an established connection in the constructor?

Copy link

MemberAuthor

nicolas-grekasApr 30, 2016

Choose a reason for hiding this comment

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

good catch, check removed

Copy link

Member

xabbuh commentedApr 30, 2016

@nicolas-grekas I'd be happy to split the PR in two (one for the Redis related changes and one for the semantic config). That would make reviewing a bit easier.

xabbuh reviewed

Apr 30, 2016

View reviewed changes

UPGRADE-3.1.md

		cache:
		pools:
		serializer:
		cache.serializer:

Copy link

Member

xabbuhApr 30, 2016

Choose a reason for hiding this comment

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

Why do we repeat thecache. prefix here. Imo this looks weird as it repeats what we already know from the config tree (I mean we are configuring the cache, why do we want to reflect that in the name here too?)?

Copy link

MemberAuthor

nicolas-grekasApr 30, 2016

Choose a reason for hiding this comment

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

because I've been told that Symfony core prefers explicit over implicit & "magic" conventions. This in fact is much more flexible and doesn't force any service prefix. See examples above where anapp.doctrine_cache pool is created. We don't have to teach any special convention here. Just give a service name.

Copy link

Member

xabbuhMay 2, 2016

Choose a reason for hiding this comment

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

I understand that we want the service to becache.serializer. But imo we should then add thecache. prefix here:https://github.com/nicolas-grekas/symfony/blob/cache-config/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L1059

Otherwise, the full config path we use here isframework.cache.pools.cache.serializer which imo is not really clear why I should use thecache part twice here.

Copy link

Member

xabbuhMay 2, 2016

Choose a reason for hiding this comment

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

And by the way, adding thecache. prefix in the extension when registering the definition will also prevent us from potentially having conflicting service names because when people configure their own cache pools, they will not always think of adding the prefix here explicitly.

Copy link

Member

fabpotMay 2, 2016

Choose a reason for hiding this comment

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

I think I prefer to be explicit. Whenever possible we try to use the full service name in configuration and that's what is done here. If the end user does not want to prefix their service name withcache., it's not a problem.

Copy link

Member

xabbuhMay 2, 2016

Choose a reason for hiding this comment

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

But we then still have the point that the configured names are directly used as service ids which can easily lead to conflicts (we don't do that anywhere else, do we?).

Copy link

Member

xabbuhMay 2, 2016

Choose a reason for hiding this comment

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

After re-reading#18667 (comment) think we can stop the discussion here as it was based on a misunderstand on my end.

👍 for how the PR is right now.

javiereguiluz added the FrameworkBundle label

Apr 30, 2016

nicolas-grekas force-pushed thecache-config branch froma34a005 to40265e5Compare

April 30, 2016 12:06

nicolas-grekas mentioned this pull request

Apr 30, 2016

[Cache] Add DSN based Redis connection factory#18681

Merged

Copy link

MemberAuthor

nicolas-grekas commentedApr 30, 2016

To be rebased on top of#18681 (which is included for now)

nicolas-grekas force-pushed thecache-config branch 2 times, most recently from51bfb86 to493c337Compare

May 1, 2016 08:49

fabpot added a commit that referenced this pull request

May 1, 2016

feature#18681[Cache] Add DSN based Redis connection factory (nicola…

8166094

…s-grekas)This PR was merged into the 3.1-dev branch.Discussion----------[Cache] Add DSN based Redis connection factory| Q             | A| ------------- | ---| Branch?       | 3.1| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -Required by#18667Commits-------3073efb [Cache] Add DSN based Redis connection factory

Copy link

Member

fabpot commentedMay 1, 2016

#18681 merged now

[FrameworkBundle] Add cache adapters in semantic configuration

80a5508

nicolas-grekas force-pushed thecache-config branch from493c337 toc24745bCompare

May 1, 2016 12:04

Copy link

MemberAuthor

nicolas-grekas commentedMay 1, 2016

rebased

nicolas-grekas force-pushed thecache-config branch fromc24745b to8c5e9aeCompare

May 2, 2016 06:33

nicolas-grekas reviewed

May 2, 2016

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Resources/config/cache.xml Outdated

		</service>

		<serviceid="cache.adapter.redis"class="Symfony\Component\Cache\Adapter\RedisAdapter"abstract="true">
		<serviceid="cache.adapter.redis"class="Symfony\Component\Cache\Adapter\RedisAdapter"abstract="true"lazy="true">

Copy link

MemberAuthor

nicolas-grekasMay 2, 2016

Choose a reason for hiding this comment

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

I just madecache.adapter.redis lazy so that no connection is opened unless needed.

Copy link

MemberAuthor

nicolas-grekasMay 2, 2016

Choose a reason for hiding this comment

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

but I'm not sure this is required, wdyt?

Copy link

Member

fabpotMay 4, 2016

Choose a reason for hiding this comment

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

we don't have any lazy service in Symfony core as it depends on a deps that is not a hard dep. Lazy means that the service is wrapped with a proxy, which adds its own layer of complexity, so I would avoid that if possible.

nicolas-grekas force-pushed thecache-config branch 2 times, most recently frome4abdc8 to8c5e9aeCompare

May 2, 2016 06:42

Copy link

Member

xabbuh commentedMay 2, 2016

Apart from how cache pool names are now treated I am 👍 for the changes.

Copy link

Member

dunglas commentedMay 2, 2016

👍

nicolas-grekas mentioned this pull request

May 3, 2016

[FrameworkBundle] Wire PhpArrayAdapter with a new cache warmer for annotations#18533

Closed

nicolas-grekas force-pushed thecache-config branch from8c5e9ae to893afdaCompare

May 3, 2016 11:42

nicolas-grekas mentioned this pull request

May 3, 2016

[PropertyAccess] Add PSR-6 cache#16838

Closed

nicolas-grekas added the Ready label

May 3, 2016

[FrameworkBundle] Simplify config for app/system/pool caches

a2a567d

nicolas-grekas force-pushed thecache-config branch from893afda toa2a567dCompare

May 4, 2016 15:36

Copy link

Member

fabpot commentedMay 4, 2016

Thank you@nicolas-grekas.

fabpot merged commita2a567d intosymfony:master

May 4, 2016

fabpot added a commit that referenced this pull request

May 4, 2016

feature#18667[FrameworkBundle] Semantic config for app/system/pool …

2849654

…caches (tgalopin, nicolas-grekas)This PR was merged into the 3.1-dev branch.Discussion----------[FrameworkBundle] Semantic config for app/system/pool caches| Q             | A| ------------- | ---| Branch?       | 3.1| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#18625| License       | MIT| Doc PR        | -Commits-------a2a567d [FrameworkBundle] Simplify config for app/system/pool caches80a5508 [FrameworkBundle] Add cache adapters in semantic configuration