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

Add MultiLockHandler and MultiSemaphore helpers #13

Merged
merged 13 commits into from
May 25, 2022

Conversation

kufd
Copy link
Contributor

@kufd kufd commented Jan 16, 2016

I have added 2 helpers MultiLockHandler and MultiSemaphore which i use in my projects.
So i think it will be usefull for other developers

.gitignore Outdated
/build/
/consul-configuration/data-dir/
/vendor/
/.idea
Copy link
Member

Choose a reason for hiding this comment

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

This one should be removed. This should be in your personal global ignore list instead.

@kufd
Copy link
Contributor Author

kufd commented Jan 17, 2016

I have fixed all issues from comments

$result = true;

// Start a session
$session = $this->session->create('{"LockDelay":0, "TTL": "'.$this->ttl.'s"}')->json();
Copy link
Member

Choose a reason for hiding this comment

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

May be it's better to use json_encode here. For example, ttl value is never controlled...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

json_encode will not protect us from wrong ttl value
So i have added validation for ttl in constructor
Do you think it is fine?

@lyrixx
Copy link
Member

lyrixx commented Jan 29, 2016

Good work !

@kufd
Copy link
Contributor Author

kufd commented Jan 29, 2016

I have made changes for all issues from comments

@lyrixx
Copy link
Member

lyrixx commented May 4, 2022

@kufd Do you still need this PR? (sorry for the HUGE delay :) )

@kufd
Copy link
Contributor Author

kufd commented May 5, 2022

@lyrixx I do not need it

@lyrixx
Copy link
Member

lyrixx commented May 5, 2022

Can you

  • rebase
  • remove all factories
  • remove all interfaces

Thanks

@kufd
Copy link
Contributor Author

kufd commented May 5, 2022

@lyrixx would you like to merge this request?
I thought you will just close it

However, if you would like to merge the pull request I will prepare it.

@lyrixx
Copy link
Member

lyrixx commented May 5, 2022

I'm OK to merge it 👍🏼

@kufd
Copy link
Contributor Author

kufd commented May 5, 2022

I will prepare this PR i a few days

@kufd
Copy link
Contributor Author

kufd commented May 9, 2022

@lyrixx, i updated the pull request
could you, please, review it

Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

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

Can you add an entry in the CHANGELOG.md, and a bit of documentation in the README.md

Thanks

src/ConsulResponse.php Show resolved Hide resolved
src/ConsulResponse.php Outdated Show resolved Hide resolved
src/Helper/MultiLockHandler.php Outdated Show resolved Hide resolved
src/Helper/MultiLockHandler.php Outdated Show resolved Hide resolved
src/Helper/MultiLockHandler.php Show resolved Hide resolved
@kufd
Copy link
Contributor Author

kufd commented May 14, 2022

@lyrixx I added some documentation and made fixes

@lyrixx lyrixx merged commit a6b1d06 into FriendsOfPHP:master May 25, 2022
@lyrixx
Copy link
Member

lyrixx commented May 25, 2022

Here we go 🎉 !

Thanks for your patience and for the hard work.

@kufd
Copy link
Contributor Author

kufd commented May 25, 2022

Finally)))
Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants