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

Updated dependencies - PHPUnit 5.7 and 6.0, dropped PHP5.5 support, QA Tools #115

Merged
merged 37 commits into from
Mar 7, 2017
Merged

Updated dependencies - PHPUnit 5.7 and 6.0, dropped PHP5.5 support, QA Tools #115

merged 37 commits into from
Mar 7, 2017

Conversation

michalbundyra
Copy link
Member

@michalbundyra michalbundyra commented Mar 3, 2017

  • PHPUnit ^6.0 || ^5.7 - Updated all tests to support both versions.

  • Added docheader tool to checks all license headers and update headers in all files.

  • Updated composer scripts and CONTRIBUTING to refer these scripts.

  • Updated travis configuration to run tests with lowest/locked/latest dependencies on PHP 5.6/7.0/7.1.

  • Dropped PHP 5.5 support.

  • Many CS fixes (fixed indents, array trailing commas, double quotes, formatting)

@froschdesign
Copy link
Member

@webimpress

Thanks for your work, but:

  • PHPUnit ^6.0 || ^5.7
  • Updated all tests to support both versions.
  • Added docheader tool to checks all license headers and update headers in all files.
  • Updated composer scripts and CONTRIBUTING to refer these scripts.
  • Updated travis configuration to run tests with lowest/locked/latest dependencies on PHP 5.6/7.0/7.1.
  • Dropped PHP 5.5 support.
  • changes in 214 files
  • 3.228 new lines
  • 1.377 removed lines
  • completely different topics

All this in one commit?!

Btw. This is more than a ugly job for a reviewer.

@michalbundyra
Copy link
Member Author

michalbundyra commented Mar 3, 2017

@froschdesign Yeah... I know it's a lot for one commit. I can split it for just update PHPUnit and then license headers.

The problem is also with docheader that we can't exclude some files. Maybe we should exclude the whole test directory? Do you want me to update the PR and split it into smaller commits?

@michalbundyra
Copy link
Member Author

Ok, I'll split it later into smaller commits and push force. Sorry for huge commit ...

@froschdesign
Copy link
Member

The problem is also with docheader that we can't exclude some files.

I mean this: malukenho/docheader#13

Changed domain from zendframework.github.io to docs.zendframework.com
- changed `PHPUnit_Framework_TestCase` to `PHPUnit\Framework\TestCase`
- changed `setExpectedException` to `expectException` and
  `expectExceptionMessage`
- changed `getMock` to `createMock`
and updated license headers in files
Dependencies updated to the latest versions.
Always prefered version is used first.

Updated travis configuration to run tests with lowest/locked/latest
dependencies and run all checks (license headers, cs, tests).
Added proper check to install legacy dependencies on PHP5.6.
Changed to method call instead. Then we can check if the exception
is thrown on a correct part of the test.
Optimized imports - alphabetical order and removed unused.
- aligned key value pairs
- removed redundant empty lines
- added missing trailing comma
@michalbundyra
Copy link
Member Author

@froschdesign PR updated. Please have a look and let me know what you think. Thanks!

src/Client.php Outdated
"Cannot handle content type '{$this->encType}' automatically"
);
throw new Client\Exception\RuntimeException(sprintf(
'Cannot handle content type "%s" automatically',
Copy link
Member

Choose a reason for hiding this comment

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

Those swapped quotes broken tests. Is this change required by CS tool?

Copy link
Member

@Xerkus Xerkus left a comment

Choose a reason for hiding this comment

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

Please, revert changes to quotes that are part of the literal.
Eg "some message with 'quotes' in it" should be reverted to its original form, or to 'some message with \'quotes\' in it'

Someone might depend on the exact message and experience subtle breakages

@michalbundyra
Copy link
Member Author

@Xerkus reverted. All green now. Thanks!

Copy link
Member

@Xerkus Xerkus left a comment

Choose a reason for hiding this comment

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

Just couple more remarks, rest of it looks good

"zendframework/zend-uri": "^2.5",
"zendframework/zend-validator": "^2.5"
"php": "^5.6 || ^7.0",
"zendframework/zend-loader": "^2.5.1",
Copy link
Member

Choose a reason for hiding this comment

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

please revert dependencies bump, it is outside the scope of this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Why you think it should be outside the scope of this PR? IMHO it should be in this scope, because I've also added testing with lowest/locked/latest dependencies. Usually we bump to the latest version (which contain all updates - and previous version was not LTS) to force users to use versions with all fixes. I think we should then have these dependencies bump here. Otherwise we can find many things to split this PR into multiple smaller PRs, but what is the point?

Copy link
Member

Choose a reason for hiding this comment

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

Updating minimum versions for those packages is unrelated to either of 1) phpunit 2) php version bump 3) coding style fixes 4) travis, unless they were failing with new test setup.

Hence it is outside of the scope of this PR.

Anyway, disregard that.

@@ -47,17 +49,17 @@
/**
* Common HTTP client
*
* @var \Zend\Http\Client
* @var Client
Copy link
Member

Choose a reason for hiding this comment

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

it is already imported as HTTPClient, no need for another use

@Xerkus Xerkus merged commit c8e3397 into zendframework:develop Mar 7, 2017
@Xerkus Xerkus added this to the 2.7.0 milestone Mar 7, 2017
@michalbundyra michalbundyra deleted the feature/dependencies branch June 22, 2017 20:56
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