Drop php 5.4 and 5.5 support #205

snapshotpl · 2016-09-07T20:56:45Z

No description provided.

weierophinney · 2016-09-08T01:56:01Z

No can do, at least not yet. We provide support for versions prior to 5.6 to accommodate the symfony ecosystem (as v2 introduced the PSR-7 bridge, and the last LTS still supports 5.4). We can look at this next year.

weierophinney · 2016-10-11T13:03:09Z

I'm thinking what we'll do is one last minor version supporting pre-5.6 versions, and then mark that an LTS version that will only receive security and/or critical bugfixes. We can then do a 2.0 version sooner rather than later, and move any new features to that.

I'll come back to this in a few weeks so we can proceed.

samsonasik · 2016-12-17T11:37:28Z

test/CallbackStreamTest.php

    public function testTell()
    {
        $stream = new CallbackStream(function () {
        });
+
+        $this->setExpectedException(RuntimeException::class);


use expectException() instead, as setExpectedException() is deprecated and will be removed in phpunit 6

snapshotpl · 2016-12-28T20:54:20Z

@weierophinney a few weeks go away, what about this?

weierophinney · 2016-12-28T22:46:49Z

Currently on vacation, @snapshotpl; will review next week.

asgrim · 2017-03-11T17:47:01Z

test/Request/SerializerTest.php

            ->getMock();

-        $stream->expects($this->once())->method('isReadable')


Why remove the expects? Surely in these cases we want to ensure isReadable is only called once still? (same question applies to other similar changes). Only thing I can think of is that because an exception is thrown that the mock expectations are not checked? (I don't know off my head without checking)

asgrim · 2017-03-11T17:49:05Z

test/Response/JsonResponseTest.php

        new JsonResponse($resource);
    }

    public function testJsonErrorHandlingOfBadEmbeddedData()
    {
-        if (version_compare(PHP_VERSION, '5.5', 'lt')) {


asgrim · 2017-03-11T17:53:47Z

test/CallbackStreamTest.php

@@ -89,13 +89,13 @@ public function testGetSize()
        $this->assertNull($ret);
    }

-    /**
-     * @expectedException RuntimeException


Ideally we should try to avoid unrelated changes; as far as I know, there's nothing broken about @expectedException is there?

I'll allow it. Semantically, having the call to expectException() immediately prior to the code that raises the exception is better. While I realize it's unrelated, since this is updating the PHPUnit version already, we might as well do some cleanup.

weierophinney · 2017-08-18T17:15:32Z

I've changed my mind on this, and am scheduling this for 1.5.0.

weierophinney

This is looking great, and takes care of a rather largish patch I didn't really want to work on myself. 😄

There are a few issues, however, which I've noted. The big general issues that apply throughout, however, are:

expectException() does not behave the same as setExpectedException(); it does not accept a second argument with the expected exception message. You'll need to break your two-argument calls to this method into calls to expectException() + expectExceptionMessage().
In many places, you refactored method mocks to remove expects() calls; these need to be re-instated, as in most cases, we are testing for once() or any(), and want to be explicit about which is correct.

weierophinney · 2017-08-22T13:04:40Z

composer.json

  },
  "require-dev": {
-    "phpunit/phpunit": "^4.6 || ^5.5",
-    "zendframework/zend-coding-standard": "~1.0.0",
+    "phpunit/phpunit": "^5.6 || ^6.0",


We need to get a bit more specific on these, as there's breakage in early versions of 6.0 and 5.6 that have often led to test failures in other repos. Try ^5.7.16 || ^6.0.8 here.

weierophinney · 2017-08-22T13:05:16Z

composer.json

-    "phpunit/phpunit": "^4.6 || ^5.5",
-    "zendframework/zend-coding-standard": "~1.0.0",
+    "phpunit/phpunit": "^5.6 || ^6.0",
+    "zendframework/zend-coding-standard": "^1.0",


This is one of the few that needs to use the ~ operator, as we don't want new PRs to suddenly fail due to new rules being added to the standard.

weierophinney · 2017-08-22T13:06:45Z

test/CallbackStreamTest.php

@@ -89,13 +89,13 @@ public function testGetSize()
        $this->assertNull($ret);
    }

-    /**
-     * @expectedException RuntimeException


I'll allow it. Semantically, having the call to expectException() immediately prior to the code that raises the exception is better. While I realize it's unrelated, since this is updating the PHPUnit version already, we might as well do some cleanup.

weierophinney · 2017-08-22T13:09:37Z

test/MessageTraitTest.php

@@ -212,8 +216,9 @@ public function invalidHeaderValues()
     */
    public function testWithHeaderRaisesExceptionForInvalidValueType($value)
    {
-        $this->setExpectedException('InvalidArgumentException', 'Invalid header value');
-        $message = $this->message->withHeader('X-Foo', $value);
+        $this->expectException(InvalidArgumentException::class, 'Invalid header value');


This isn't doing what you think.

expectException() only accepts one argument, the exception type to test for. If you also want to test the message, you need to add an additional call to expectExceptionMessage().

weierophinney · 2017-08-22T13:10:30Z

test/MessageTraitTest.php

@@ -229,8 +234,9 @@ public function testWithHeaderReplacesDifferentCapitalization()
     */
    public function testWithAddedHeaderRaisesExceptionForNonStringNonArrayValue($value)
    {
-        $this->setExpectedException('InvalidArgumentException', 'must be a string');
-        $message = $this->message->withAddedHeader('X-Foo', $value);
+        $this->expectException(InvalidArgumentException::class, 'must be a string');


Same comment here, and throughout; any two-argument calls to setExpectedException() need to be re-written as two discrete calls to expectException() and expectExceptionMessage().

weierophinney · 2017-08-22T13:14:30Z

test/Request/SerializerTest.php


-        $stream->expects($this->once())->method('isSeekable')
-            ->will($this->returnValue(false));


I'm not sure I understand why these mocks were rewritten. The combination of getMockBuilder() + getMock() works in PHPUnit 5.6+ and 6.0+, as does the syntax of expects(). The changes you have introduced loosen the expectations by removing the expects($this->once()) constraints...

weierophinney · 2017-08-22T13:16:23Z

test/Response/SerializerTest.php

-            ->will($this->returnValue(true));
+        $stream = $this->createMock(StreamInterface::class);
+        $stream->method('isReadable')->will($this->returnValue(true));
+        $stream->method('isSeekable')->will($this->returnValue(false));


Same comment here about the removal of expects().

weierophinney · 2017-08-22T13:17:01Z

test/ServerIntegrationTest.php

@@ -43,5 +42,7 @@ public function testPassesBufferLevelToSapiStreamEmitter()
        );
        $server->setEmitter($emitter->reveal());
        $server->listen();
+
+        ob_end_clean();


Why was this added? Was it needed?

Discovered why when removing locally; PHPUnit flags it as a risky test due to the fact it did not close its own output buffers.

weierophinney · 2017-08-22T13:19:29Z

test/ServerRequestFactoryTest.php

+    {
+        $_SERVER = $this->globalServer;
+    }
+


Why is this functionality needed? The @backupGlobals and @preserveGlobalState annotations are enabled by default, which should make this unnecessary. What did you observe that led to their addition?

The test testFromGlobalsUsesCookieSuperGlobalWhenCookieHeaderIsNotSet() was causing inconsistent results due to superglobal state otherwise. This is better alleviated by the combination of @runInSeparateProcess and @preserveGlobalState disabled annotations.

weierophinney · 2017-08-22T13:21:01Z

test/ServerTest.php

            ->method('getBody')
            ->willReturn($responseBody);
        $this->response
-            ->expects($this->any())


The expects() calls need to be re-instated; these were there for a reason.

Drop php 5.4 and 5.5 support

weierophinney · 2017-08-22T17:33:33Z

@snapshotpl I've pushed the changes I've requested to your branch at this time, and will rebase to incorporate them into your own commits once I can verify the changes pass continuous integration.

Close #205

weierophinney · 2017-08-22T18:56:44Z

Thanks, @snapshotpl

weierophinney added this to the 2.0.0 milestone Oct 11, 2016

samsonasik reviewed Dec 17, 2016

View reviewed changes

snapshotpl changed the base branch from master to develop December 28, 2016 20:17

Ocramius assigned weierophinney Jan 23, 2017

Ocramius added enhancement question labels Jan 23, 2017

asgrim reviewed Mar 11, 2017

View reviewed changes

kokspflanze mentioned this pull request Jun 4, 2017

PHPunit 6 update #253

Closed

weierophinney modified the milestones: 1.5.0, 2.0.0 Aug 18, 2017

weierophinney suggested changes Aug 22, 2017

View reviewed changes

weierophinney added a commit to snapshotpl/zend-diactoros that referenced this pull request Aug 22, 2017

Merge pull request zendframework#205 from snapshotpl/drop-php-54-support

d53f46b

Drop php 5.4 and 5.5 support

snapshotpl added 11 commits August 22, 2017 12:52

Remove php support for lower than 5.6

cc95e5d

Remove skip test for old php versions

7cbb7ce

Use ::class in setExpectedException

1bc1e9f

Improve readability of tests

b13fe59

Remove expected exceptions from annotations

89fea99

Remove all class names as string

c08f2af

Replace deprecated method in tests

302dce7

Update composer.lock

0c614ab

Fix cs issues

c112f4e

Add support for phpunit ^6.0

a1a9591

Fix cs issues

5c3280d

weierophinney force-pushed the drop-php-54-support branch from 3148b37 to a0b4008 Compare August 22, 2017 18:40

snapshotpl added 3 commits August 22, 2017 13:50

Fix platform

2c8aa04

Use phpunit6 features

08b546d

Add support for php 7.2

adb5925

weierophinney force-pushed the drop-php-54-support branch from a0b4008 to adb5925 Compare August 22, 2017 18:50

weierophinney merged commit adb5925 into zendframework:develop Aug 22, 2017

weierophinney added a commit that referenced this pull request Aug 22, 2017

Adds CHANGELOG for #205

b7191c3

weierophinney added a commit that referenced this pull request Aug 22, 2017

Merge branch 'feature/205' into develop

5c86735

Close #205

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop php 5.4 and 5.5 support #205

Drop php 5.4 and 5.5 support #205

snapshotpl commented Sep 7, 2016

weierophinney commented Sep 8, 2016

weierophinney commented Oct 11, 2016

samsonasik Dec 17, 2016

snapshotpl commented Dec 28, 2016

weierophinney commented Dec 28, 2016

asgrim Mar 11, 2017

asgrim Mar 11, 2017

asgrim Mar 11, 2017

weierophinney Aug 22, 2017

weierophinney commented Aug 18, 2017

weierophinney left a comment

weierophinney Aug 22, 2017

weierophinney Aug 22, 2017

weierophinney Aug 22, 2017

weierophinney Aug 22, 2017

snapshotpl Aug 22, 2017

weierophinney Aug 22, 2017

weierophinney Aug 22, 2017

weierophinney Aug 22, 2017

weierophinney Aug 22, 2017

weierophinney Aug 22, 2017

weierophinney Aug 22, 2017

weierophinney Aug 22, 2017

weierophinney Aug 22, 2017

weierophinney commented Aug 22, 2017

weierophinney commented Aug 22, 2017

		->getMock();

		$stream->expects($this->once())->method('isReadable')


		$stream->expects($this->once())->method('isSeekable')
		->will($this->returnValue(false));

Drop php 5.4 and 5.5 support #205

Drop php 5.4 and 5.5 support #205

Conversation

snapshotpl commented Sep 7, 2016

weierophinney commented Sep 8, 2016

weierophinney commented Oct 11, 2016

Choose a reason for hiding this comment

snapshotpl commented Dec 28, 2016

weierophinney commented Dec 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Aug 18, 2017

weierophinney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Aug 22, 2017

weierophinney commented Aug 22, 2017