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

feat: use https by default #53

Merged
merged 1 commit into from
Mar 31, 2020
Merged

feat: use https by default #53

merged 1 commit into from
Mar 31, 2020

Conversation

ericdeansanchez
Copy link
Contributor

To summarize the following, this PR changes the default behavior
of the UrlBuilder class. Before this change, the default behavior
used http by default. After this change, the default behavior uses
https by default.

When I began, I made a small change in the constructor definition and
ran the test. All passed (not intended, not expected). I removed the
change and began this PR by writing a failing test first. I re-ran the
test suite and the test,testExamplePlainUsesHttpsByDefault, failed
as intended. I then made the appropriate change in the constructor
definition, re-ran the tests, all passed.

  • first, I wrote a failing test; it tests that this SDK does use
    https by default, it failed
  • then the default constructor's default-parameter was changed to
    use $useHttps = true (instead of $useHttps = false)
  • this change is to ensure that this SDK uses https by default
  • with the new test testExamplePlainUsesHttpsByDefault, any
    subsequent changes to this behavior will be a "breaking change"
  • the naming of testExamplePlainUsesHttpsByDefault was used to
    emulate the naming-style present in the test suite
  • the structure of testExamplePlainUsesHttpsByDefault has been
    written to emulate the structure of similar tests (e.g.
    testExamplePlain)
  • note: with the given API, this change cannot be tested in
    isolation and requires a call to createUrl
  • note: the current API does not allow for incremental testing
    of individual builder-fields, e.g. there are no "getters" and the
    builder-fields are private:
private $domain;
private $useHttps;
private $signKey;
  • the above note is looking forward to future tweaks to our API to
    ensure more complete test coverage.

Ensure at least one test case fails before change:
Note that the expected value is https.

Time: 95 ms, Memory: 10.00 MB

There was 1 failure:

1) UrlBuilderTest::testExamplePlainUsesHttpsByDefault
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'https://demos.imgix.net/bridge.png?h=100&w=100'
+'http://demos.imgix.net/bridge.png?h=100&w=100'

/Users/Eric/GitHub/imgix-php/tests/Imgix/Tests/UrlBuilderTest.php:33

FAILURES!
Tests: 50, Assertions: 354, Failures: 1.

Ensure all tests pass after change:
Both the expected and actual values concur.

PHPUnit 9.0.1 by Sebastian Bergmann and contributors.

..................................................                50 / 50 (100%)

Time: 93 ms, Memory: 10.00 MB

OK (50 tests, 354 assertions)

This single test should be sufficient for our purposes, here and now. It
is the only point of entry (i.e. no "getters"). UrlHelper does have
a getUrl method, but that is not near this change (getUrl is not
called until UrlBuilder.createUrl returns).

To summarize the following, this PR changes the default behavior
of the `UrlBuilder` class. Before this change the default behavior
used `http` by default. After this change, the default behavior uses
`https` __by default__.

When I began, I made a small change in the constructor definition and
ran the test. All passed. I removed the change and began this PR by
writing a _failing test_ first. I re-ran the test suite and this test,
`testExamplePlainUsesHttpsByDefault`, failed as intended. I then made
the appropriate change in the constructor definition, re-ran the tests,
all passed.

- first, I wrote a failing test; it tests that this SDK does use
  https by default, it failed
- then the default constructor's default-parameter was changed to
  use `$useHttps = true` (instead of `$useHttps = false`)
- this change is to ensure that this SDK uses https by default
- with the new test `testExamplePlainUsesHttpsByDefault`, any
  subsequent changes to this behavior will be a "breaking change"
- the naming of `testExamplePlainUsesHttpsByDefault` was used to
  emulate the naming-style present in the test suite
- the structure of `testExamplePlainUsesHttpsByDefault` has been
  written to emulate the structure of similar tests (e.g.
  `testExamplePlain`)
- **note**: with the given API, this change cannot be tested in
  isolation and requires a call to `createUrl`
- **note**: the current API does not allow for incremental testing
  of individual builder-fields, e.g. there are no "getters" and the
  builder-fields are private:

```php
private $domain;
private $useHttps;
private $signKey;
```
- the above note is looking forward to future tweaks to our API to
  ensure more complete test coverage.

**Ensure at least one test case fails before change**:
Note that the **expected** value is `https`.
```
Time: 95 ms, Memory: 10.00 MB

There was 1 failure:

1) UrlBuilderTest::testExamplePlainUsesHttpsByDefault
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'https://demos.imgix.net/bridge.png?h=100&w=100'
+'http://demos.imgix.net/bridge.png?h=100&w=100'

/Users/Eric/GitHub/imgix-php/tests/Imgix/Tests/UrlBuilderTest.php:33

FAILURES!
Tests: 50, Assertions: 354, Failures: 1.
```

**Ensure all tests pass after change**:
Both the **expected** and **actual** values concur.
```
PHPUnit 9.0.1 by Sebastian Bergmann and contributors.

..................................................                50 / 50 (100%)

Time: 93 ms, Memory: 10.00 MB

OK (50 tests, 354 assertions)
```

This single test should be sufficient for our purposes, here and now. It
is the only point of entry (i.e. no "getters"). `UrlHelper` does have
a `getUrl` method, but that is not near this change (`getUrl` is not
called until `UrlBuilder.createUrl` returns).
Copy link
Contributor

@sherwinski sherwinski left a comment

Choose a reason for hiding this comment

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

All the code changes lgtm 👍

Can we also modify the documentation to reflect these changes before merging?

@ericdeansanchez
Copy link
Contributor Author

@sherwinski Yeah, we can. Let's keep this commit/PR to just the above. I'll open another PR to update the documentation after this has been merged.

@ericdeansanchez ericdeansanchez merged commit ac02e69 into master Mar 31, 2020
@ericdeansanchez ericdeansanchez deleted the use-https-by-default branch March 31, 2020 01:22
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