-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[Lock] Make various changes/updates on the Lock docs #17413
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
Conversation
I'd like to merge the two docs about Lock, is it still something we what to do overall? |
Yes. Moving as much of the docs to the main guide is still something we agree on as far as I know. What is still a bit unsure is where the "this is how you set-up the component outside the framework" docs need to be (component doc? minor section in main guide? component's README?) |
From what I understand, we've already moved "smaller" component docs to the README files. I think that's a good strategy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of nice fixes, thanks!
Am I correct that we should merge this in 5.4 instead of 6.2?
@@ -81,57 +81,57 @@ continue the job in another process using the same lock:: | |||
use Symfony\Component\Lock\Lock; | |||
|
|||
$key = new Key('article.'.$article->getId()); | |||
$lock = new Lock($key, $this->store, 300, false); | |||
$lock = new Lock($key, $this->store, autoRelease: false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given our BC promise doesn't guarantee BC for named arguments, do we want to use them in the documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it as I found it easier to understand. I suppose people are using them anyway, so why not use them in the docs. I can revert if you think it would send a signal that they are part of the BC promise.
Not sure about the version, it's really up to you. |
@wouterj I tried to merge this in 5.4 but I faced too many conflicts. If you have some time, please try to merge this yourself. Thanks! |
Rebased and merged in #17465 Thanks Fabien! |
No description provided.