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 overriding TRUSTED_HOSTS and MERCURE_PUBLIC_URL variables #2666

Merged
merged 2 commits into from Apr 3, 2024

Conversation

J3m5
Copy link
Contributor

@J3m5 J3m5 commented Mar 18, 2024

Modifying the SERVER_NAME to disable HTTPS, as outlined in the API Platform documentation, leads to improper configurations for TRUSTED_HOSTS and MERCURE_PUBLIC_URL when using formats like http://localhost or :80.

This results in values like ^:80|php$ or ^http://localhost|php$ for TRUSTED_HOSTS, and URLs such as https://:80/.well-known/mercure or https://http://localhost/.well-known/mercure for MERCURE_PUBLIC_URL.

This PR adopts "nested interpolation" from the Docker documentation to allow precise overrides, introducing TRUSTED_PROXIES and CADDY_MERCURE_PUBLIC_URL as new, adjustable variables, with room for naming discussions.

…C_URL variables

Fix issue with invalid TRUSTED_HOSTS and MERCURE_PUBLIC_URL when using http://localhost or :80 as SERVER_NAME
compose.yaml Outdated Show resolved Hide resolved
compose.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@vincentchalamon vincentchalamon left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

LGTM. Could you update the docs to mention these new variables (the current example is still broken, even with your changes, because the new variables must be explicitly set) and also update Symfony Docker accordingly? https://github.com/dunglas/symfony-docker/blob/main/compose.yaml

@J3m5
Copy link
Contributor Author

J3m5 commented Mar 19, 2024

LGTM. Could you update the docs to mention these new variables (the current example is still broken, even with your changes, because the new variables must be explicitly set)

Of course.

In which branch this change will be merged?
I suppose I'll need to handle the documentation with the new variables and without in previous branches, right @dunglas ?
How do I should proceed?

and also update Symfony Docker accordingly? https://github.com/dunglas/symfony-docker/blob/main/compose.yaml

Done: dunglas/symfony-docker#591

@dunglas
Copy link
Member

dunglas commented Mar 19, 2024

Target the 3.3 branch for the docs. We don't have branches for the distribution.

@soyuka soyuka merged commit 5033aa3 into api-platform:main Apr 3, 2024
2 checks passed
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.

None yet

4 participants