Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Mar 18, 2024

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Mar 2, 2024

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 the setLockFactory method without introducing a BC break for people who rely on the fact that the LockableTrait use a different LockFactory than the one configured the framework.lock dsn.

@carsonbot
Copy link

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

@94noni
Copy link
Contributor

94noni commented Mar 2, 2024

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

@VincentLanglet
Copy link
Contributor Author

it could be a valuable addition indeed,

but this reminds me a discussion of mine (other account)

I also saw #53923.

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...

@94noni
Copy link
Contributor

94noni commented Mar 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

Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

Tests? 🙃

src/Symfony/Component/Console/Command/LockableTrait.php Outdated Show resolved Hide resolved
@VincentLanglet
Copy link
Contributor Author

Tests? 🙃

Indeed, I added some now @derrabus

@nicolas-grekas
Copy link
Member

Thank you @VincentLanglet.

@nicolas-grekas nicolas-grekas merged commit a2fc092 into symfony:7.1 Mar 18, 2024
7 of 10 checks passed
@fabpot fabpot mentioned this pull request May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants