[FrameworkBundle] Add ability to use existing service as lock/semaphore resource #58249

HypeMC · 2024-09-13T00:58:45Z

Q	A
Branch?	7.2
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	-
License	MIT

Contains #58250

Currently, it's not possible to set an existing service as the lock/semaphore resource via the config, even though both factories support this functionality, eg:

symfony/src/Symfony/Component/Lock/Store/StoreFactory.php

Lines 27 to 36 in 44395ab

    
           public static function createStore(#[\SensitiveParameter] object|string $connection): PersistingStoreInterface 
        
           { 
        
               switch (true) { 
        
                   case $connection instanceof \Redis: 
        
                   case $connection instanceof Relay: 
        
                   case $connection instanceof \RedisArray: 
        
                   case $connection instanceof \RedisCluster: 
        
                   case $connection instanceof \Predis\ClientInterface: 
        
                       return new RedisStore($connection);

HypeMC · 2024-09-13T01:40:30Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -2004,6 +2004,9 @@ private function registerLockConfiguration(array $config, ContainerBuilder $cont
            $storeDefinitions = [];
            foreach ($resourceStores as $resourceStore) {
                $storeDsn = $container->resolveEnvPlaceholders($resourceStore, null, $usedEnvs);
+                if (!($usedEnvs || preg_match('#^[a-z]++://#', $resourceStore))) {
+                    $resourceStore = new Reference($resourceStore);
+                }


symfony/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

Lines 1230 to 1235 in 44395ab

}

$container->resolveEnvPlaceholders($config['handler_id'], null, $usedEnvs);

if ($usedEnvs || preg_match('#^[a-z]++://#', $config['handler_id'])) {

$id = '.cache_connection.'.ContainerBuilder::hash($config['handler_id']);

stof · 2024-09-13T15:24:05Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd

        </xsd:sequence>
        <xsd:attribute name="enabled" type="xsd:boolean" />
+        <xsd:attribute name="resource" type="xsd:string" />


why allowing a resource attribute here ? this does not make to me: we still need a name for the lock being registered, so <framework:resource name="my_lock">my_service</framework:resource> is still the expected XML format to me.

same for semaphore of course

@stof This is the equivalent of doing:

framework: lock: dsn-or-service

If it's a string, the default name is used:

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

Lines 1392 to 1394 in 4e70834

->beforeNormalization()

->ifString()->then(fn ($v) => ['default' => $v])

->end()

The issue is that the XML schema allows providing both the attribute and child elements, which would do weird stuff. I'd rather keep the clean XML schema as we have right now, even though it does not support one of the shortcut notation.

If someone is using XML configuration file, they will not complain to us that their configuration is too verbose due to that missing shortcut, as using XML in itself would be responsible for a lot more verbosity. Projects using XML configuration file generally do it to benefit from the XML schema.

@stof Makes sense, see https://github.com/symfony/symfony/compare/c2a12b45832d1262ddb2e0ea9ccf545bef91fc74..34e5112e9afaf2ef4a9fb348a90b8bfeae71c6db

…aphore config (HypeMC) This PR was merged into the 6.4 branch. Discussion ---------- [FrameworkBundle] Finish incomplete tests for lock & semaphore config | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | - | License | MIT I noticed this while working on #58249. Basically, the test files were partially added but never used, so the tests were incomplete. ~Also, there's a bug in the schema, as the configuration in those files didn't work.~ Commits ------- 0574c86 [FrameworkBundle] Fix schema & finish incomplete tests for lock & semaphore config

nicolas-grekas · 2024-09-16T15:05:12Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -2004,6 +2004,9 @@ private function registerLockConfiguration(array $config, ContainerBuilder $cont
            $storeDefinitions = [];
            foreach ($resourceStores as $resourceStore) {
                $storeDsn = $container->resolveEnvPlaceholders($resourceStore, null, $usedEnvs);
+                if (!($usedEnvs || preg_match('#^[a-z]++://#', $resourceStore))) {


that should be enough IMHO:

Suggested change

if (!($usedEnvs || preg_match('#^[a-z]++://#', $resourceStore))) {

if (!$usedEnvs && !str_contains($resourceStore, '://')) {

Done, I've also changed it for the session handler config https://github.com/symfony/symfony/compare/34e5112e9afaf2ef4a9fb348a90b8bfeae71c6db..f53ef173a29e4921f5661369391e30ae980a7b3f

nicolas-grekas

LGTM

…re resource

fabpot · 2024-09-18T16:07:46Z

Thank you @HypeMC.

ostrolucky · 2024-09-22T07:52:09Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -2004,6 +2004,9 @@ private function registerLockConfiguration(array $config, ContainerBuilder $cont
            $storeDefinitions = [];
            foreach ($resourceStores as $resourceStore) {
                $storeDsn = $container->resolveEnvPlaceholders($resourceStore, null, $usedEnvs);
+                if (!$usedEnvs && !str_contains($resourceStore, '://')) {


This breaks BC in some use cases, like doctrine-bundle tests revealed https://github.com/doctrine/DoctrineBundle/actions/runs/10976729897/job/30477816481

…vice IDs (HypeMC) This PR was merged into the 7.2 branch. Discussion ---------- [FrameworkBundle][Lock] Fix certain DSNs identified as service IDs | Q | A | ------------- | --- | Branch? | 7.2 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #58249 (comment) | License | MIT Follow up to #58249 The first PR introduced a BC break, as lock DSNs can also be in the following formats: ```yaml framework: lock: 'flock' lock: 'semaphore' lock: 'in-memory' lock: 'mysql:host=127.0.0.1;dbname=app' lock: 'pgsql:host=127.0.0.1;dbname=app' lock: 'pgsql+advisory:host=127.0.0.1;dbname=app' lock: 'sqlsrv:server=127.0.0.1;Database=app' lock: 'oci:host=127.0.0.1;dbname=app' ``` This PR introduces two changes: 1. The framework bundle now treats any string containing a `:` as a DSN, instead of only those with `://` 2. `flock`, `semaphore`, and `in-memory` are now treated as special cases Commits ------- 9abfbe0 [FrameworkBundle][Lock] Fix certain DSNs identified as service IDs

…vice IDs (HypeMC) This PR was merged into the 7.2 branch. Discussion ---------- [FrameworkBundle][Lock] Fix certain DSNs identified as service IDs | Q | A | ------------- | --- | Branch? | 7.2 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix symfony/symfony#58249 (comment) | License | MIT Follow up to #58249 The first PR introduced a BC break, as lock DSNs can also be in the following formats: ```yaml framework: lock: 'flock' lock: 'semaphore' lock: 'in-memory' lock: 'mysql:host=127.0.0.1;dbname=app' lock: 'pgsql:host=127.0.0.1;dbname=app' lock: 'pgsql+advisory:host=127.0.0.1;dbname=app' lock: 'sqlsrv:server=127.0.0.1;Database=app' lock: 'oci:host=127.0.0.1;dbname=app' ``` This PR introduces two changes: 1. The framework bundle now treats any string containing a `:` as a DSN, instead of only those with `://` 2. `flock`, `semaphore`, and `in-memory` are now treated as special cases Commits ------- 9abfbe056d [FrameworkBundle][Lock] Fix certain DSNs identified as service IDs

carsonbot added Status: Needs Review Feature FrameworkBundle labels Sep 13, 2024

carsonbot added this to the 7.2 milestone Sep 13, 2024

HypeMC mentioned this pull request Sep 13, 2024

[FrameworkBundle] Finish incomplete tests for lock & semaphore config #58250

Merged

HypeMC commented Sep 13, 2024

View reviewed changes

stof reviewed Sep 13, 2024

View reviewed changes

HypeMC force-pushed the use-services-in-lock-and-semaphore branch from c2a12b4 to 34e5112 Compare September 13, 2024 17:15

nicolas-grekas reviewed Sep 16, 2024

View reviewed changes

nicolas-grekas approved these changes Sep 16, 2024

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Sep 16, 2024

HypeMC force-pushed the use-services-in-lock-and-semaphore branch from 34e5112 to f53ef17 Compare September 16, 2024 15:26

nicolas-grekas approved these changes Sep 16, 2024

View reviewed changes

HypeMC force-pushed the use-services-in-lock-and-semaphore branch from f53ef17 to e91d7aa Compare September 17, 2024 22:56

[FrameworkBundle] Add ability to use existing service as lock/semapho…

670ee5d

…re resource

HypeMC force-pushed the use-services-in-lock-and-semaphore branch from e91d7aa to 670ee5d Compare September 18, 2024 15:08

fabpot merged commit c8137fe into symfony:7.2 Sep 18, 2024
9 of 10 checks passed

fabpot mentioned this pull request Sep 18, 2024

[FrameworkBundle] Add ability to use existing service as lock/semaphore… symfony/symfony-docs#20230

Closed

HypeMC deleted the use-services-in-lock-and-semaphore branch September 18, 2024 16:09

ostrolucky reviewed Sep 22, 2024

View reviewed changes

HypeMC mentioned this pull request Oct 3, 2024

[FrameworkBundle][Lock] Fix certain DSNs identified as service IDs #58445

Merged

HypeMC mentioned this pull request Oct 9, 2024

The service "doctrine.orm.listeners.lock_store_schema_listener" has a dependency on a non-existent service "semaphore". #58383

Closed

fabpot mentioned this pull request Oct 27, 2024

Release v7.2.0-BETA1 #58686

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FrameworkBundle] Add ability to use existing service as lock/semaphore resource #58249

[FrameworkBundle] Add ability to use existing service as lock/semaphore resource #58249

HypeMC commented Sep 13, 2024 •

edited

Loading

HypeMC Sep 13, 2024 •

edited

Loading

stof Sep 13, 2024

stof Sep 13, 2024

HypeMC Sep 13, 2024

stof Sep 13, 2024

HypeMC Sep 13, 2024

nicolas-grekas Sep 16, 2024

HypeMC Sep 16, 2024

nicolas-grekas left a comment

fabpot commented Sep 18, 2024

ostrolucky Sep 22, 2024

	public static function createStore(#[\SensitiveParameter] object\|string $connection): PersistingStoreInterface
	{
	switch (true) {
	case $connection instanceof \Redis:
	case $connection instanceof Relay:
	case $connection instanceof \RedisArray:
	case $connection instanceof \RedisCluster:
	case $connection instanceof \Predis\ClientInterface:
	return new RedisStore($connection);

	}
	$container->resolveEnvPlaceholders($config['handler_id'], null, $usedEnvs);

	if ($usedEnvs \|\| preg_match('#^[a-z]++://#', $config['handler_id'])) {
	$id = '.cache_connection.'.ContainerBuilder::hash($config['handler_id']);

	->beforeNormalization()
	->ifString()->then(fn ($v) => ['default' => $v])
	->end()

	if (!($usedEnvs \|\| preg_match('#^[a-z]++://#', $resourceStore))) {
	if (!$usedEnvs && !str_contains($resourceStore, '://')) {

[FrameworkBundle] Add ability to use existing service as lock/semaphore resource #58249

[FrameworkBundle] Add ability to use existing service as lock/semaphore resource #58249

Conversation

HypeMC commented Sep 13, 2024 • edited Loading

HypeMC Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

fabpot commented Sep 18, 2024

Choose a reason for hiding this comment

HypeMC commented Sep 13, 2024 •

edited

Loading

HypeMC Sep 13, 2024 •

edited

Loading