Open mrubelmann opened 6 years ago
Reviewed 2 of 2 files at r1. Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.
composer.json, line 27 at r1 (raw file):
"require": { "php": ">=5.3.9", "phpunit/phpunit": "~5.0",
I still have to support phpunit 4.x. Please revert this change.
src/Concise/Mock/PrototypeBuilder.php, line 57 at r1 (raw file):
} protected function getReturnType(ReflectionMethod $method)
Your IDE will probably complain about this, but we still need to support < PHP 7:
if (method_exists($method, 'hasReturnType') && $method->hasReturnType()) {
}
This is also why the Travis build is failing.
Comments from Reviewable
Reviewed 3 of 3 files at r2. Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks broke.
composer.json, line 27 at r1 (raw file):
I still have to support phpunit 4.x. Please revert this change.
Why is v6+ not supported?
src/Concise/Core/Version.php, line 86 at r2 (raw file):
else { return ''; }
What is the reason for this change? It should always be able to produce a version name.
Comments from Reviewable
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks broke.
composer.json, line 27 at r1 (raw file):
Why is v6+ not supported?
The namespaces have changed in v6. For example, instead of having a class named PHPUnit_Framework_Test in the global namespace, it's now \PHPUnit\Framework\Test. There may be other breaking changes, but I'm not sure.
src/Concise/Core/Version.php, line 86 at r2 (raw file):
What is the reason for this change? It should always be able to produce a version name.
Oops, I didn't realize the pull request was going to be updated with this change. (This is my first pull request on GitHub. How exciting!) I'm not 100% sure if it's necessary yet. The reason for it is because I'm specifying "dev-master" as the version requirement for my project that's using Concise since I'm not tagging any releases. It winds up looking for $names['d'], which clearly doesn't exist. My local dev environment doesn't complain for some reason, but it throws an exception on the build machine. I don't understand how it could possibly work anywhere, but I'm pretty new to PHP.
Once I get everything working, I'll try removing this and see what happens. If it still doesn't work, I'll at least put in a comment about it.
src/Concise/Mock/PrototypeBuilder.php, line 57 at r1 (raw file):
Your IDE will probably complain about this, but we still need to support < PHP 7: ```php if (method_exists($method, 'hasReturnType') && $method->hasReturnType()) { } ``` This is also why the Travis build is failing.
I thought it would be okay since I'm checking to see if "hasReturnType" exists before calling it. Is there something else I need to do to support both versions?
Comments from Reviewable
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks broke.
composer.json, line 27 at r1 (raw file):
The namespaces have changed in v6. For example, instead of having a class named PHPUnit_Framework_Test in the global namespace, it's now \PHPUnit\Framework\Test. There may be other breaking changes, but I'm not sure.
Ah, I see. Thanks.
composer.json, line 40 at r2 (raw file):
"": [ "tests/", "src/" ] } }
Since you are running a much newer version of PHP when you update the composer dependencies it is picking the latest versions that are compatible with your newer PHP versions, but not necessarily compatible with older PHP versions.
Perhaps the best course of action is to remove the compose.lock
file in this PR.
src/Concise/Core/Version.php, line 86 at r2 (raw file):
Oops, I didn't realize the pull request was going to be updated with this change. (This is my first pull request on GitHub. How exciting!) I'm not 100% sure if it's necessary yet. The reason for it is because I'm specifying "dev-master" as the version requirement for my project that's using Concise since I'm not tagging any releases. It winds up looking for $names['d'], which clearly doesn't exist. My local dev environment doesn't complain for some reason, but it throws an exception on the build machine. I don't understand how it could possibly work anywhere, but I'm pretty new to PHP. Once I get everything working, I'll try removing this and see what happens. If it still doesn't work, I'll at least put in a comment about it.
You will not need to tag release. That is done by me on the repository once the PR is merged in. As long as it doesn't cause an error on your end you should not need to change anything. It will just work when the new version is released.
Please remove this change.
src/Concise/Mock/PrototypeBuilder.php, line 57 at r1 (raw file):
I thought it would be okay since I'm checking to see if "hasReturnType" exists before calling it. Is there something else I need to do to support both versions?
You are only checking the return value of hasReturnType
not if the method exists - which is required to support less than PHP 7. See https://travis-ci.org/elliotchance/concise/jobs/258273737#L296
Comments from Reviewable
Concise isn't able to mock functions in PHP 7 that have a return type declared. This change supports that by using reflection to find the return type and appending it to the function prototype. It supports both primitives and classes. I'm kind of a PHP newbie, so I might be missing some edge case, but it seems to work so far.
This change is