ruflin / Elastica

Elastica is a PHP client for elasticsearch
http://elastica.io/
MIT License
2.26k stars 736 forks source link

Allow base url without ending slash #2161

Open jaroslavtyc opened 1 year ago

jaroslavtyc commented 1 year ago

Accept both with and without trailing slash URL as Elastic source.

I have spent few time to find out what is wrong with Elastic URL https://foo:bar@elastic-dev.example.com, getting error Couldn't resolve host

./bin/console fos:elastica:create

Creating bitbag_shop_product

In Http.php line 186:

  Couldn't resolve host  

fos:elastica:create [--index [INDEX]] [--no-alias]

Reason was missing trailing slash in URL, leading into invalid URL https://foo:bar@elastic-dev.example.combitbag_shop_product instead of correct https://foo:bar@elastic-dev.example.com/bitbag_shop_product

Suggested change ensures slash between base URL and request path (Elastic index name in this case).

ruflin commented 1 year ago

This reminds me of https://github.com/ruflin/Elastica/issues/1777 Can you check where we landed there? Need to read up on it again.

jaroslavtyc commented 1 year ago

@ruflin In that issue I have found opposite problems

  1. it is broken without trailing slash https://github.com/ruflin/Elastica/issues/1777#issuecomment-735692398
  2. it is broken with trailing slash https://github.com/ruflin/Elastica/issues/1777#issue-617992054

My proposal is to make sure there is slash if and only if there will be path appended to host. It can be redundant if you solve somehow the https://github.com/ruflin/Elastica/issues/1777, but it will solve without any detect-server-and-client-expectations magic single problem now and will be harmless later.

ruflin commented 1 year ago

Thanks for the clarification. I couldn't really think of a reason why there would ever be no / be needed in this situation except the path put in would start with a /. But then again, users don't put paths in but indexNames for example where / is not an allowed character. But now things become interesting: All tests fail so there seems to be a pretty strong side effect of this. Can you take a closer look on what happens?

Please also add a changelog entry.