Add new methods to satisfy Application contract for Laravel 5.8 #1625
Conversation
This adds a `shouldReport` method as required by the updated contract. Currently it just always returns true - more sophisticated methods of determining if it should report could be implemented though. Fixes dingo#1624
|
This only fails testing because of API changes in Laravel 5.7 vs 5.8. I would make an attempt to resolve the errors, however there are too many ways this could be addressed for me to attempt it. I need some direction from maintainers on how this should be addressed. The common option is to tag a specific version of the package for the new laravel version. So V2 of dingo is 5.5-5.7 and V3 of Dingo would be 5.8+ (at least until another breaking change). Or since it's literally just a test stub file causing it to fail, we could adjust how tests are configured to detect which stub to use. Then tests could just switch between the pre-5.8 and post-5.8 stubs. |
|
Update: This PR will not fix all the issues - it merely fixes the first error, but unfolds many many more. The Application contract was updated in Laravel 5.8, even if we make changes to address these changes it won't fix the issue. Details here: https://laravel.com/docs/5.8/upgrade#the-application-contract Even after making the necessary changes to the Stub file, we'll run into a new error due to Lumen not being fully updated for 5.8 yet. Essentially changes in The offending line. |
…that implements the Laravel 5.8 contract - this should fix the main issues we’re seeing.
This creates a trait that contains logic needed to check the version of Laravel/lumen that is installed. Using said version it will provide the proper application stub for testing.
shouldReport method to satisfy contract|
@mallardduck Thanks for your contributions here. In terms of the application stub, why not just update the existing one? If I try you 58 stub as replacing the normal one, all the tests still pass? So far as Lumen, I actually think we should just drop it's support going forward, as it doesn't seem to be that it is being used frequently for new projects within the community nor supported well by the Laravel team themselves. Edit: After taking a closer look, I can see they are not directly compatible. I guess the question is does it really matter if tests pass, given it's just a testing stub. |
|
Excluding the Lumen routing tests has corrected the testing not working. I will merge this PR for now because it's great, but let me know your thoughts on the two application stubs vs one. |
PR Not Final
Until Lumen publishes a tag for 5.8 the testing framework used for this package is broken. So the main option is to wait until that happens, with an alternaitive of simply modifying the dependency for lumen-framework to not use
dev-master.Intent of PR
This PR adds all the new methods for Laravel 5.8 as required by the updated contract.
Fixes #1624
Notes
This change also includes some necessary modifications to how testing works, so as to allow the tests to support Laravel 5.5-5.7 and 5.8. The package itself seems unaffected, just the tests were having issues.