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

Add PHP 7.2 testing #1811

Merged
merged 6 commits into from
May 1, 2017
Merged

Add PHP 7.2 testing #1811

merged 6 commits into from
May 1, 2017

Conversation

scaytrase
Copy link
Contributor

  • Include PHP 7.2 to travis matrix
  • Fix TCP proxy url (trailing slash parsing fails on PHP 7.2)
  • Update to phpunit ^4.0 || ^5.0 (4.0 is no longer supported, but required for testing against PHP 5.5)
  • Add tests for countables

At the moment no more failures found for 7.2 and tests are green. Further patches are matter of test coverage

Incorporates #1809
cc. @sagikazarmark

@sagikazarmark
Copy link
Member

We already have a proposed fix for the proxy thing in #1792

It's not decided yet what should be the final solution though.

@scaytrase
Copy link
Contributor Author

@sagikazarmark I'm not fixing it, but just "hiding" the problem with testing against correct URI.

If\When you decide to fix this in a proper way you choose you can add more tests against slash-trailed URIs to check that behavior is correct

@@ -37,6 +37,11 @@ public function testCreatesFromArray()
$this->assertCount(2, $jar);
}

public function testEmptyJarIsCountable()
{
self::assertCount(0, new CookieJar());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why self:: not $this->. I think code should be uniform.

Copy link
Contributor

Choose a reason for hiding this comment

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

May be $this->assertInstanceOf will be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accepted $this->, will fix this.

Did not understand about instanceOf. This test explicitly invokes the count() method to be sure, that internals of CookieJar is countable from construction with 7.2

Copy link
Contributor

Choose a reason for hiding this comment

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

As I understand, you want to check the existence of count method. Am I right? But actually you check that count == 0. So I'm suggesting you to check implementing of the \Countable interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I wan't to actually invoke the count method. Because PHP 7.2 throws a warning when trying to count a non-countable things like null, which could be done internally (i.e counting internal property which was not initialized) . So I want to check that CookieJar does not throws the warning when calling count directly after constructing.

https://wiki.php.net/rfc/counting_non_countables

The same thing I did with the MockHandler, but we have to fix it (see array default)

Copy link
Contributor

@Aliance Aliance Apr 29, 2017

Choose a reason for hiding this comment

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

You wrote:

So I want to check that CookieJar does not throws the warning when calling count

But actually you

you check that count == 0

So nothing changes in my mind if you will check implementing of the \Countable interface, because it guarantees that calling count won't fail.

But it's kind of a holywar... =)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having count method implemented does not guarantees that id does not fail. Short example:

class foo implements \Countable {
   private $things;

   public function count() { return count($this->things); }

   public function push($element) { $this->things[] = $element; } 
}

$f = new \foo();
$f->count(); // this will pass on PHP < 7.2 and throw warning on PHP 7.2
$f->push(1);
$f->count(); // this will pass

So having count method implemented does not guarantee that it works properly internally.

Checking that count == 0 is needed because the future phpunit versions will mark tests without assertions as risky, actually I could just call the (new CookieJar())->count() without any assertions.

@@ -270,6 +270,7 @@ public function testAddsProxy()
public function testAddsProxyByProtocol()
{
$url = str_replace('http', 'tcp', Server::$url);
$url = rtrim($url, '/');
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a quick not why we are doing this? Just to make sure we find it. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I've referred your #1823 issue

@sagikazarmark
Copy link
Member

Ok, one small comment, then it's good to go.

@sagikazarmark
Copy link
Member

Thanks for your work.

@sagikazarmark sagikazarmark merged commit 703160e into guzzle:master May 1, 2017
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

3 participants