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

wp-env: Fix redirect loop for instances on port 80 (and 443) #50282

Closed
wants to merge 2 commits into from

Conversation

Luehrsen
Copy link
Contributor

@Luehrsen Luehrsen commented May 3, 2023

What?

This Pull Request introduces an exception within the addOrReplacePort function, ensuring that if the specified port is either 80 or 443, the input remains unaltered.

Why?

The modifications introduced in #49883 mandate the inclusion of port numbers in the constants WP_TESTS_DOMAIN, WP_SITEURL, and WP_HOME. Unfortunately, there doesn't appear to be a configuration option to bypass the addition of these ports.

As a result, this negatively impacts all wp-env instances operating on ports 80 or 443. Modern browsers automatically remove these default ports, which leads to a redirection loop in the frontend, rendering the site inaccessible to users

@Luehrsen Luehrsen requested a review from noahtallen as a code owner May 3, 2023 09:39
Copy link
Contributor

@ObliviousHarmony ObliviousHarmony left a comment

Choose a reason for hiding this comment

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

Thanks for catching this and working on it!

I don't think the function signature has the correct type for port anymore. We are assuming that it's a string, but as per the validation done when parsing configuration files, it should always be number.

Could we:

  • Update the addOrReplacePort JSDoc for port to be {number}.
  • Update tests to pass a number instead of a string.

@ObliviousHarmony
Copy link
Contributor

When I reviewed this @Luehrsen I noticed a small thing about port validation. I fixed that in another PR, but while I was there, couldn't help but bundle in this change 😅 You can take a look at #50300, but, I took this and made the small change I suggested!

@Luehrsen
Copy link
Contributor Author

Luehrsen commented May 4, 2023

@ObliviousHarmony Yeah, I think we can close this and work on #50300.

Just make sure to credit appropriately and mention #49883 for better visibility! :)

@ObliviousHarmony
Copy link
Contributor

Alright @Luehrsen,

I've gone ahead and updated that PR's description to better reflect the primary fix it contains.

Thanks!

@noahtallen
Copy link
Member

Closing now that #50300 has landed!

@noahtallen noahtallen closed this May 4, 2023
@Luehrsen Luehrsen deleted the fix/wp-env-port-80 branch May 9, 2023 08:14
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.

3 participants