Skip to content
This repository has been archived by the owner. It is now read-only.

Update to Laravel 5.8 #550

Merged
merged 3 commits into from Feb 27, 2019

Conversation

@jhm-ciberman
Copy link
Contributor

commented Feb 27, 2019

  • Removes the direct dependency on phpunit
    (since is required by orchestra/testbench)
  • Update tests to support phpunit 8.0.0 (some methods were deprecated)

jhm-ciberman added some commits Feb 27, 2019

Update to Laravel 5.8
-Removes the direct dependency on phpunit
(since is required by orchestra/testbench)
- Update tests to support phpunit 8.0.0 (some methods were deprecated)
@jhm-ciberman

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

@dimsav Can you please review this, merge and release a new version? It's not urgent but I want to update my client's server to laravel 5.8 as soon as posible because it has a lot of bugfixes that I need.
Thanks 😄

Edit: Also, I think this is an awesome package. Thanks for the great work!

@drbyte

This comment has been minimized.

Copy link

commented Feb 27, 2019

@jhm-ciberman do you have a solution for making all these tests still work with older Laravel versions? The :void you added to setUp() in TestCase.php breaks everything prior to Laravel 5.8

@jhm-ciberman

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

Can I ask why? I am not into Laravel package development, but as far as I understand orchestra/testbench is independent from Laravel. Or am I missing something?

@drbyte

This comment has been minimized.

Copy link

commented Feb 27, 2019

Meh. Ya, I guess it's independent from your own actual app. But if the package maintainer wants to run the tests against older versions (for compatibility with the users using those older versions) then they'll have to go and remove all the incompatibilities before they can run the tests.

My point is: you may not get the "quick merge" you asked for.

@jhm-ciberman

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

Still, I don't understand what it breaks the :void at the end. The test suite is not required in any app so it should not break anything.

The setUp() method I am overriding is from orchestra/testbench, not from Laravel, so it shouldn't break anything.

Can you provide me some example of what it breaks?

@rhwilr

This comment has been minimized.

Copy link

commented Feb 27, 2019

The :void declaration for the setUp() method is not Laravels fault but a result of Laravel upgrading to PHPUnit 8.
Instead of removing "phpunit/phpunit": "~7.0" from the dev-dependencies we could pin it to version 8. I think this should resolve this problem.

@rhwilr

This comment has been minimized.

Copy link

commented Feb 27, 2019

Correction:
Laravel did not upgrade to PHPUnit 8 but it added support for it. Therefore the method have to declare the void return type.

But I have to agree with @jhm-ciberman, this does not break anything. It still works with Laravel 5.7.

@dimsav

This comment has been minimized.

Copy link
Owner

commented Feb 27, 2019

Thank you all for making sure this package is properly maintained. 🙏

@dimsav dimsav merged commit 0c1f96d into dimsav:master Feb 27, 2019

1 check passed

continuous-integration/styleci/pr The analysis has passed
Details
@Gummibeer

This comment has been minimized.

Copy link
Collaborator

commented Feb 27, 2019

To all here: all our build combinations pass, even with the :void.
https://circleci.com/workflow-run/d34883e1-b949-49a2-ab40-4af0318870be
If someone is interested in his special combination: we have all 9 prepared, so everyone can run them with the CircleCI local CLI https://circleci.com/docs/2.0/local-cli/#run-a-job-in-a-container-on-your-machine

After #556 is approved and merged we will release a new version.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.