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

[Console] Add a way to use custom lock factory in lockableTrait#54135

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

nicolas-grekas merged 1 commit intosymfony:7.1fromVincentLanglet:lockableTrait

Mar 18, 2024

Merged

[Console] Add a way to use custom lock factory in lockableTrait#54135

nicolas-grekas merged 1 commit intosymfony:7.1fromVincentLanglet:lockableTrait

Mar 18, 2024

Conversation

Copy link

Contributor

VincentLanglet commentedMar 2, 2024•
edited
Loading

Q	A
Branch?	7.1
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	Fix #...
License	MIT

The LockableTrait only use SemaphoreStore or FlockStore, but currently we cannot use MemcachedStore or RedisStore.

When using a custom LockFactory everywhere in the codebase, it would be useful to chose if the commande need a server-related store or not.

I'm not sure if I have a way to provide autowire to thesetLockFactory method without introducing a BC break for people who rely on the fact that the LockableTrait use a different LockFactory than the one configured theframework.lock dsn.

VincentLanglet requested a review fromchalasr as acode owner

March 2, 2024 10:46

carsonbot added Status: Needs Review Console Feature labels

Mar 2, 2024

carsonbot added this to the7.1 milestone

Mar 2, 2024

Copy link

carsonbot commentedMar 2, 2024

Hey!

Thanks for your PR. You are targeting branch "7.1" but it seems your PR description refers to branch "7.1 for features".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

Copy link

Contributor

94noni commentedMar 2, 2024

it could be a valuable addition indeed,
but this reminds me adiscussion of mine (other account)

Copy link

ContributorAuthor

VincentLanglet commentedMar 2, 2024

it could be a valuable addition indeed,
but this reminds me adiscussion of mine (other account)

I also sawsymfony/symfony-docs#20210.

I currently find the LockableTrait really confusing because when using it (without reading the code inside) you could expect that the trait use the default configured lock factory instead of creating a new one.

I was trying to improve the situation...

Copy link

Contributor

94noni commentedMar 3, 2024

Yes and you do good i think but may it will leverage discussion
Perhaps create another trait and name it specifically ?
and yes i’ve also seen the issue discussion and the doc and/or the class can be improved to give more context to the dev