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

Support DSN in Client constructor for config argument (#1640) #1652

Merged
merged 1 commit into from
Aug 28, 2019

Conversation

alamirault
Copy link
Contributor

This PR allow to pass DSN to the Elastica\Client constructor as discussed in #1640 .

For example: $client = new Client('https://user:[email protected]:9200?persistent=false&retryOnConflict=2');

I have just a doubt about url config parameter. Should I build it from transport, host, port and path ? Or it works when these args are filled ?

Another question: Is there a doc to update ? Elastica.io appears to be outdated.

Copy link
Owner

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

This looks really good and thanks for adding all the tests.

For url: What you mean by "it works when these args are filled"?

For the docs: Use the phpdocs as extensive as possible. Elastica.io didn't get all the love it should get recently, but it is the place for the docs which don't fit the source code.

lib/Elastica/Dsn.php Outdated Show resolved Hide resolved
@alamirault
Copy link
Contributor Author

I reworked my PR. Now the client configuration is exported in new class. It avoid code duplication.
I also allow any extra option (removed hardcoded part)

@ruflin
Copy link
Owner

ruflin commented Aug 27, 2019

I like the change to use a separate class for the config object. I'm trying to poke holes into this change to see what side effects it could have. So far the only thing I could think of is that something that inherits the client could mess around with _config but even this should be fine as it can still be an array.

It seems everything should continue to works as before?

@alamirault
Copy link
Contributor Author

I think is only side effect. Indeed everything works without any changes.

I also would like deprecate setConfig,getConfig,setConfigValue,getConfigValue and add a ClientConfiguration getter in Client. WDYT ?

@ruflin ruflin changed the title [WIP] Support DSN in Client constructor for config argument (#1640) Support DSN in Client constructor for config argument (#1640) Aug 28, 2019
@ruflin ruflin merged commit e9c85c9 into ruflin:master Aug 28, 2019
@ruflin
Copy link
Owner

ruflin commented Aug 28, 2019

@alamirault Your proposal SGTM. I already merged the PR as I think these additional changes / deprecations should go into a separate PR. I hope you are ok with that?

@alamirault
Copy link
Contributor Author

I agree. I will make another PR when I have time.

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.

2 participants