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

Allow base url without ending slash #2161

Open
wants to merge 1 commit into
base: 8.x
Choose a base branch
from

Conversation

jaroslavtyc
Copy link

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:[email protected], 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:[email protected]_shop_product instead of correct https://foo:[email protected]/bitbag_shop_product

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

Accept both with and without trailing slash URL as Elastic source
@ruflin
Copy link
Owner

ruflin commented May 17, 2023

This reminds me of #1777 Can you check where we landed there? Need to read up on it again.

@jaroslavtyc
Copy link
Author

@ruflin In that issue I have found opposite problems

  1. it is broken without trailing slash Trailing slash in host causes issues #1777 (comment)
  2. it is broken with trailing slash Trailing slash in host causes issues #1777 (comment)

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 #1777, but it will solve without any detect-server-and-client-expectations magic single problem now and will be harmless later.

@ruflin
Copy link
Owner

ruflin commented May 26, 2023

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.

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