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

endpoint index and type getters #557

Merged
merged 1 commit into from
Mar 24, 2017
Merged

endpoint index and type getters #557

merged 1 commit into from
Mar 24, 2017

Conversation

ewgRa
Copy link
Contributor

@ewgRa ewgRa commented Mar 20, 2017

  1. Is it possible to add getIndex and getType getters for AbstractEndpoint?
    If not, what is a purpose to make it so private?

We need it for ruflin/Elastica#1275

  1. Also, why:
        public function setType($type)
    {
        if ($type === null) {
            return $this;
        }


so strange? If I want drop type for my endpoint, why client force me to recreate full endpoint? Or at least why it is so silent?

@polyfractal
Copy link
Contributor

Hi! Sorry for the delay, I've been sick this week :(

Is it possible to add getIndex and getType getters for AbstractEndpoint? If not, what is a purpose to make it so private?

++! There's no particular reason they didn't exist. The client didn't need the functionality previously, but I don't see any problem adding the accessors.

so strange? If I want drop type for my endpoint, why client force me to recreate full endpoint? Or at least why it is so silent?

Historical, mostly. In the beginning, it was an attempt to help safety-proof the client against bad input. Over time my views on that have changed, but the code remained the same. And no one has ever requested the ability to "unset" a param/type on an endpoint (remarkably enough), so this code has remained.

We can certainly remove these checks, I don't have a strong opinion about keeping them.

Sorry again fro the delay!

@polyfractal
Copy link
Contributor

PR LGTM, merging!

@polyfractal polyfractal merged commit f0f636a into elastic:master Mar 24, 2017
@ewgRa
Copy link
Contributor Author

ewgRa commented Mar 24, 2017

Thanks!

polyfractal pushed a commit that referenced this pull request Mar 30, 2017
p365labs pushed a commit to p365labs/elasticsearch-php that referenced this pull request Sep 10, 2017
p365labs pushed a commit to p365labs/elasticsearch-php that referenced this pull request Sep 10, 2017
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