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

[WIP] Broaden HTTPS Check #201

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jbh
Copy link

@jbh jbh commented Nov 14, 2019

Because:

  • It is common for end users to use ports other than 443 for HTTPS
  • Mature systems, like IBM i, like to uppercase values

Provide a narrative description of what you are trying to accomplish:

  • Are you fixing a bug?
    • Detail how the bug is invoked currently.
      If you try building an API with HTTPS without using 443, the self links that are built and returned will be HTTP. If you try building an API on IBM i, even using 443, it will still send HTTP, because IBM i will uppercase ON and HTTPS values for the SERVER properties.
    • Detail the original, incorrect behavior.
      As stated above, the self links that are generated in API responses will be HTTP instead of HTTPS.
    • Detail the new, expected behavior.
      Self links will properly send HTTPS.
    • Base your feature on the master branch, and submit against that branch.
    • Add a regression test that demonstrates the bug, and proves the fix.
      This is difficult to accomplish, as the test I am doing runs on IBM i.
    • Add a CHANGELOG.md entry for the fix.
      TODO

Because:
- It is common for end users to use ports other than 443 for HTTPS
- Mature systems, like IBM i, like to uppercase values
@jbh
Copy link
Author

jbh commented Nov 14, 2019

Forgot to mention that this will resolve #194.

@jbh
Copy link
Author

jbh commented Nov 14, 2019

All checks failed? Ouch.

@froschdesign
Copy link
Member

@jbh

It is common for end users to use ports other than 443 for HTTPS.

"common" – I do not think so; and if it were so, then something more must be change in the class and in some other components too. (e.g. zend-uri, zend-diactoros)

Mature systems, like IBM i, like to uppercase values

Sounds conclusive.
We need unit tests to cover these changes. Thanks in advance!

All checks failed? Ouch.

Right, see the results on Travis: https://travis-ci.org/zendframework/zend-view/jobs/611967352#L439-L447

@jbh
Copy link
Author

jbh commented Nov 14, 2019

@froschdesign I work with many enterprise agencies, and they have plenty of internal, and even some external applications that they use HTTPS on different ports for. They would be pretty offended to hear that isn't common... This is a deal breaker for them when they try to use Apigility.

I'm trying to find a decent way to handle the tests. Thanks for the input, and pointing me to the proper error in Travis.

Edited to add a question
If the maintainers insist I do not remove the 443 check, which I can agree is non-standard (not uncommon, though) what do they suggest I do to support applications built with HTTPS on the non-standard port?

@weierophinney
Copy link
Member

@froschdesign and @jbh

The way we handle it in Diactoros (per the PSR-7 specification) is to omit the port when it corresponds to the default port for the scheme used. So, for HTTP, you omit :80, and for HTTPS, you omit :443.

The IETF says that you can use the port in all cases, but suggests that it should be omitted when it is the default, which is why we went that route for Diactoros.

That's all that needs to happen here.

@weierophinney
Copy link
Member

This repository has been closed and moved to laminas/laminas-view; a new issue has been opened at laminas/laminas-view#1.

@weierophinney
Copy link
Member

This repository has been moved to laminas/laminas-view. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

  • Squash all commits in your branch (git rebase -i origin/{branch})
  • Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
  • Run the laminas/laminas-migration tool on the code.
  • Clone laminas/laminas-view to another directory.
  • Copy the files from the second bullet point to the clone of laminas/laminas-view.
  • In your clone of laminas/laminas-view, commit the files, push to your fork, and open the new PR.
    We will be providing tooling via laminas/laminas-migration soon to help automate the process.

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