[FrameworkBundle] Allow user to specify folder for flock #26923

MaksSlesarenko · Apr 13, 2018

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
License	MIT
Doc PR

In case multiple applications running on same server allow user to specify folder for flock
example:


framework: 
   lock:` 'flock://var/flock' # var/flock will be provided as path to flock constructor
   lock: 'flock:///var/flock' # /var/flock will be provided as path to flock constructor
   lock: flock # works as usual, null is provided to constructor and system temp folder is used

nicolas-grekas

looks like a good idea to me
ping @jderusse for 2nd review
what about adding some tests?

nicolas-grekas · Apr 14, 2018

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

@@ -1374,8 +1374,18 @@ private function registerLockConfiguration(array $config, ContainerBuilder $cont
            foreach ($resourceStores as $storeDsn) {
                $storeDsn = $container->resolveEnvPlaceholders($storeDsn, null, $usedEnvs);
                switch (true) {
-                    case 'flock' === $storeDsn:


I think we should keep this case, and add a new one after

nicolas-grekas · Apr 14, 2018

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

@@ -1374,8 +1374,18 @@ private function registerLockConfiguration(array $config, ContainerBuilder $cont
            foreach ($resourceStores as $storeDsn) {
                $storeDsn = $container->resolveEnvPlaceholders($storeDsn, null, $usedEnvs);
                switch (true) {
-                    case 'flock' === $storeDsn:
-                        $storeDefinition = new Reference('lock.store.flock');
+                    case 0 === strpos($storeDsn, 'flock'):


case 0 === strpos($storeDsn, 'flock://'): + substr instead of str_replace below

nicolas-grekas · Apr 14, 2018

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

+
+                        if ($isMatched) {
+                            $storeDefinition = new Definition(FlockStore::class);
+                            $storeDefinition->setPublic(false);


not required anymore, this is the default since 4.0

nicolas-grekas · Apr 14, 2018

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

+                            $storeDefinition = new Definition(FlockStore::class);
+                            $storeDefinition->setPublic(false);
+                            $storeDefinition->setArguments(array($flockPath));
+                            $container->setDefinition($storeDefinitionId = 'lock.flock.store.'.$container->hash($storeDsn), $storeDefinition);


anticipating for #26921, the service name should start with a dot

I'd suggest this:

$storeDefinitionId = '.lock.flock.store.'.$container->hash($storeDsn); $container->register($storeDefinitionId, FlockStore::class)->addArgument($flockPath);

jderusse

Great idea, I like the use of DSN for such purpose.

Nothing to add to @nicolas-grekas 's comments :)

MaksSlesarenko · Apr 16, 2018

updated pr.
I don't mind adding couple tests, but I'm not seeing any for lock component in framework bundle to begin with

jderusse · Apr 16, 2018

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

@@ -1377,6 +1377,14 @@ private function registerLockConfiguration(array $config, ContainerBuilder $cont
                    case 'flock' === $storeDsn:
                        $storeDefinition = new Reference('lock.store.flock');
                        break;
+                    case 0 === strpos($storeDsn, 'flock'):


'flock://'

jderusse · Apr 16, 2018

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

+                    case 0 === strpos($storeDsn, 'flock'):
+                        $flockPath = substr($storeDsn, 8);
+
+                        $storeDefinitionId = 'lock.flock.store.'.$container->hash($storeDsn);


'.lock.flock.store.'.$container->hash($storeDsn); as per @nicolas-grekas 's comment

Can someone enlighten me about purpose of the starting dot?

see https://github.com/symfony/symfony/pull/26923/files/dae20b46a3a1515af3ba907882b828d9778b2224#r181552130 related to #26921

MaksSlesarenko · Apr 20, 2018

up

nicolas-grekas · Apr 20, 2018

see fabbot failure :)

xabbuh · Apr 20, 2018

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

+                        $flockPath = substr($storeDsn, 8);
+
+                        $storeDefinitionId = '.lock.flock.store.'.$container->hash($storeDsn);
+                        $container->register($storeDefinitionId, FlockStore::class)->addArgument($flockPath);


missing use statement for the FlockStore class

MaksSlesarenko · May 8, 2018

Is there anything else I forgot to fix?

xabbuh · May 9, 2018

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

+                        $storeDefinitionId = '.lock.flock.store.'.$container->hash($storeDsn);
+                        $container->register($storeDefinitionId, FlockStore::class)->addArgument($flockPath);
+
+                        $storeDefinition = new Reference($storeDefinitionId);


Do we need the reference? I mean if I don't miss anything, we do not reuse it anyway, so something like this should work the same:

$flockPath = substr($storeDsn, 8); $storeDefinitionId = '.lock.flock.store.'.$container->hash($storeDsn); $storeDefinition = $container->register($storeDefinitionId, FlockStore::class)->addArgument($flockPath);

Same can be said for both 'flock' and 'semaphore' cases. But I'm not sure that this PR is correct place for such refactoring. Besides '$storeDefinitions' already contains 'Reference' objects so it would be not cool to add strings or non-Reference type

MaksSlesarenko · May 21, 2018

up

fabpot · Sep 4, 2018

Thank you @MaksSlesarenko.

…ck (MaksSlesarenko) This PR was merged into the 4.2-dev branch. Discussion ---------- [FrameworkBundle] Allow user to specify folder for flock | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | License | MIT | Doc PR | In case multiple applications running on same server allow user to specify folder for flock example: ``` framework: lock:` 'flock://var/flock' # var/flock will be provided as path to flock constructor lock: 'flock:///var/flock' # /var/flock will be provided as path to flock constructor lock: flock # works as usual, null is provided to constructor and system temp folder is used ``` Commits ------- 244d762 added ability to specify folder for flock

…rence (HypeMC) This PR was submitted for the 4.2 branch but it was squashed and merged into the 4.3 branch instead (closes #11466). Discussion ---------- Add flock:// option to framework lock configuration reference Fixes #9779 & extends #11465 . Documents the `flock://` option which was added with symfony/symfony#26923 . **NOTE 1**: The xml configuration doesn't currently work, see symfony/symfony#31197 **NOTE 2**: This PR has all the changes that #11465 has plus some symfony 4.2 specific additions, so #11465 should probably be merged first. Commits ------- d9125fd Add flock:// option to framework lock configuration reference

carsonbot added Status: Needs Review Feature labels Apr 13, 2018

nicolas-grekas changed the title ~~Allow user to specify folder for flock~~ [FrameworkBundle] Allow user to specify folder for flock Apr 14, 2018

nicolas-grekas requested changes Apr 14, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Apr 14, 2018

nicolas-grekas added this to the 4.1 milestone Apr 14, 2018

jderusse approved these changes Apr 15, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Apr 15, 2018

jderusse reviewed Apr 16, 2018

View reviewed changes

carsonbot removed the Status: Reviewed label Apr 20, 2018

xabbuh requested changes Apr 20, 2018

View reviewed changes

carsonbot added the Status: Needs Work label Apr 20, 2018

fabpot modified the milestones: 4.1, next Apr 22, 2018

added ability to specify folder for flock

244d762

MaksSlesarenko force-pushed the patch-1 branch from 91fe019 to 244d762 Compare April 23, 2018 11:10

xabbuh added the FrameworkBundle label Apr 23, 2018

xabbuh reviewed May 9, 2018

View reviewed changes

fabpot approved these changes Sep 4, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Sep 4, 2018

fabpot merged commit 244d762 into symfony:master Sep 4, 2018

javiereguiluz mentioned this pull request Sep 7, 2018

Document the integration of the Lock component in the FrameworkBundle symfony/symfony-docs#9779

Closed

nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018

This was referenced Nov 3, 2018

Release v4.2.0-BETA1 #29072

Closed

Release v4.2.0-BETA1 #29073

Merged

HypeMC mentioned this pull request Apr 22, 2019

Add flock:// option to framework lock configuration reference symfony/symfony-docs#11466

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FrameworkBundle] Allow user to specify folder for flock #26923

[FrameworkBundle] Allow user to specify folder for flock #26923

MaksSlesarenko commented Apr 13, 2018 •

edited

Loading

nicolas-grekas left a comment

nicolas-grekas Apr 14, 2018

nicolas-grekas Apr 14, 2018

nicolas-grekas Apr 14, 2018

nicolas-grekas Apr 14, 2018

nicolas-grekas Apr 14, 2018

jderusse left a comment

MaksSlesarenko commented Apr 16, 2018

jderusse Apr 16, 2018

jderusse Apr 16, 2018

MaksSlesarenko Apr 17, 2018

jderusse Apr 17, 2018

MaksSlesarenko commented Apr 20, 2018

nicolas-grekas commented Apr 20, 2018

xabbuh Apr 20, 2018

MaksSlesarenko Apr 23, 2018

MaksSlesarenko commented May 8, 2018 •

edited

Loading

xabbuh May 9, 2018

MaksSlesarenko May 11, 2018 •

edited

Loading

MaksSlesarenko commented May 21, 2018

fabpot commented Sep 4, 2018

Search code, repositories, users, issues, pull requests...

[FrameworkBundle] Allow user to specify folder for flock #26923

[FrameworkBundle] Allow user to specify folder for flock #26923

Conversation

MaksSlesarenko commented Apr 13, 2018 • edited Loading

nicolas-grekas left a 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

jderusse left a comment

Choose a reason for hiding this comment

MaksSlesarenko commented Apr 16, 2018

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

MaksSlesarenko commented Apr 20, 2018

nicolas-grekas commented Apr 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaksSlesarenko commented May 8, 2018 • edited Loading

Choose a reason for hiding this comment

MaksSlesarenko May 11, 2018 • edited Loading

Choose a reason for hiding this comment

MaksSlesarenko commented May 21, 2018

fabpot commented Sep 4, 2018

MaksSlesarenko commented Apr 13, 2018 •

edited

Loading

MaksSlesarenko commented May 8, 2018 •

edited

Loading

MaksSlesarenko May 11, 2018 •

edited

Loading