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

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

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 1 commit intosymfony:masterfromMaksSlesarenko:patch-1

Sep 4, 2018

Merged

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

fabpot merged 1 commit intosymfony:masterfromMaksSlesarenko:patch-1

Sep 4, 2018

Conversation

Copy link

Contributor

MaksSlesarenko commentedApr 13, 2018•
edited
Loading

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

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

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

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

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

		foreach ($resourceStoresas$storeDsn) {
		$storeDsn =$container->resolveEnvPlaceholders($storeDsn,null,$usedEnvs);
		switch (true) {
		case'flock' ===$storeDsn:

Copy link

Member

Choose a reason for hiding this comment

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

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

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

		switch (true) {
		case'flock' ===$storeDsn:
		$storeDefinition =newReference('lock.store.flock');
		case0 ===strpos($storeDsn,'flock'):

Copy link

Member

Choose a reason for hiding this comment

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

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

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


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

Copy link

Member

Choose a reason for hiding this comment

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

not required anymore, this is the default since 4.0

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

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

Copy link

Member

Choose a reason for hiding this comment

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

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

Copy link

Member

Choose a reason for hiding this comment

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

I'd suggest this:

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

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Apr 14, 2018

nicolas-grekas added this to the4.1 milestone

Apr 14, 2018

jderusse approved these changes

Apr 15, 2018

View reviewed changes

Copy link

Member

jderusse left a comment

Choose a reason for hiding this comment

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

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

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

carsonbot added Status: Reviewed and removed Status: Needs Work labels

Apr 15, 2018

Copy link

ContributorAuthor

MaksSlesarenko commentedApr 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 reviewed

Apr 16, 2018

View reviewed changes

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

		case'flock' ===$storeDsn:
		$storeDefinition =newReference('lock.store.flock');
		break;
		case0 ===strpos($storeDsn,'flock'):

Copy link

Member

Choose a reason for hiding this comment

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

'flock://'

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

		case0 ===strpos($storeDsn,'flock'):
		$flockPath =substr($storeDsn,8);

		$storeDefinitionId ='lock.flock.store.'.$container->hash($storeDsn);

Copy link

Member

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Can someone enlighten me about purpose of the starting dot?

Copy link

Member

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

MaksSlesarenko commentedApr 20, 2018

Copy link

Member

nicolas-grekas commentedApr 20, 2018

see fabbot failure :)

carsonbot removed the Status: Reviewed label

Apr 20, 2018

xabbuh requested changes

Apr 20, 2018

View reviewed changes

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);

Copy link

Member

Choose a reason for hiding this comment

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

missinguse statement for theFlockStore class

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

done

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 thepatch-1 branch from91fe019 to244d762Compare

April 23, 2018 11:10

xabbuh added the FrameworkBundle label

Apr 23, 2018

Copy link

ContributorAuthor

MaksSlesarenko commentedMay 8, 2018•
edited
Loading

Is there anything else I forgot to fix?

xabbuh reviewed

May 9, 2018

View reviewed changes

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

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

		$storeDefinition =newReference($storeDefinitionId);

Copy link

Member

Choose a reason for hiding this comment

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

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);

Copy link

ContributorAuthor

MaksSlesarenkoMay 11, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

MaksSlesarenko commentedMay 21, 2018

fabpot approved these changes

Sep 4, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels

Sep 4, 2018

Copy link

Member

fabpot commentedSep 4, 2018

Thank you@MaksSlesarenko.

fabpot merged commit244d762 intosymfony:master

Sep 4, 2018

fabpot added a commit that referenced this pull request

Sep 4, 2018

feature#26923[FrameworkBundle] Allow user to specify folder for flo…

9ad492f

…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 flockexample:```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

javiereguiluz mentioned this pull request

Sep 7, 2018

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

Closed

nicolas-grekas modified the milestones:next,4.2

Nov 1, 2018

This was referencedNov 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 referencesymfony/symfony-docs#11466

Merged

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request

Sep 26, 2019

minor#11466Add flock:// option to framework lock configuration refe…

bca6cf3

…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 referenceFixes#9779 & extends#11465 .Documents the `flock://` option which was added withsymfony/symfony#26923 .**NOTE 1**: The xml configuration doesn't currently work, seesymfony/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

Labels

Feature FrameworkBundle Status: Reviewed

Movatterモバイル変換

Uh oh!

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

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

Uh oh!

Conversation

MaksSlesarenko commentedApr 13, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jderusse left a comment

Choose a reason for hiding this comment

Uh oh!

MaksSlesarenko commentedApr 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaksSlesarenko commentedApr 20, 2018

Uh oh!

nicolas-grekas commentedApr 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaksSlesarenko commentedMay 8, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaksSlesarenkoMay 11, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaksSlesarenko commentedMay 21, 2018

Uh oh!

fabpot commentedSep 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

MaksSlesarenko commentedApr 13, 2018•
edited
Loading

MaksSlesarenko commentedMay 8, 2018•
edited
Loading

MaksSlesarenkoMay 11, 2018•
edited
Loading