Skip to content
This repository has been archived by the owner on Jan 31, 2020. It is now read-only.

make host lowercase #29

Closed
wants to merge 4 commits into from

Conversation

samsonasik
Copy link
Contributor

Based on https://tools.ietf.org/html/rfc3986#section-3.2.2 , the host subcomponent is case-insensitive, so the "www.EXAMPLE.com" is same with "www.example.com". I think it should be normalized to lowercase within Uri::setHost() call.

@samsonasik
Copy link
Contributor Author

Is there anything I can do to get it merged? Thank you.

@weierophinney
Copy link
Member

I'm not sure what this change buys us.

Case insensitivity here simply means that, for purposes of equivalency, different cases are the same. It does not mean that there MUST be a single representation of the host name.

Casting it to lower case has some ramifications, particularly when we consider unicode domain names, as calling strtolower() on those may actually cause breakage. Additionally, when it comes to logging, changing the case may make matching the URI to one logged by the web server harder.

As such, I'm closing this at this point. If developers need to do URI hostname comparisons, they should ensure they use case-insensitive comparison operations.

@samsonasik
Copy link
Contributor Author

If we check Zend\Diactoros\Uri, there is strtolower against host, is concistency something can be considered?

@weierophinney
Copy link
Member

If we check Zend\Diactoros\Uri, there is strtolower against host, is concistency something can be considered?

Hmmm... I swear I checked that yesterday, but somehow I must have missed the strtolower() call (it's at line 318 for those who are curious).

Re-opening. I still maintain it could be considered a BC break, but we'll do another minor release to call out the change, as it does make it more consistent.

@weierophinney weierophinney reopened this Feb 27, 2019
weierophinney added a commit that referenced this pull request Feb 27, 2019
weierophinney added a commit that referenced this pull request Feb 27, 2019
@weierophinney
Copy link
Member

Thanks, @samsonasik! Merged to develop for an immediate 2.7.0 release, with a CHANGELOG entry calling out the change in behavior.

@samsonasik samsonasik deleted the lowercase-host branch February 28, 2019 02:44
@bmxmale
Copy link

bmxmale commented Mar 12, 2019

@weierophinney you was right with breakage. Simple case from Magento 2.3. Host is not always domain or IP address. When we using docker we can call host via container names.

Example:

host = MAGENTO2_varnish

When we call strtolower then we getting error:

[2019-03-12 11:10:23] main.CRITICAL: Unable to connect to magento2_varnish:80 . Error #0: stream_socket_client(): unable to connect to magento2_varnish:80 (php_network_getaddresses: getaddrinfo failed: Name or service not known) {"method":"GET","url":"http:/","invalidateInfo":{"server":"[object] (Zend\\Uri\\Uri: http://magento2_varnish:80/)","tagsPattern":".*"}} []

So MAGENTO2_varnish != magento2_varnish.

If anyone ( like me ) using aliases on magento then this patch break eg. Varnish Purge request.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants