New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove the use of deprecated dispatcher factory #67
Remove the use of deprecated dispatcher factory #67
Conversation
Removed the calls to the deprecated dispatcher factory
Codecov Report
@@ Coverage Diff @@
## dev #67 +/- ##
============================================
+ Coverage 81.23% 81.74% +0.51%
- Complexity 402 423 +21
============================================
Files 13 14 +1
Lines 1007 1063 +56
============================================
+ Hits 818 869 +51
- Misses 189 194 +5
Continue to review full report at Codecov.
|
tests/bootstrap.php
Outdated
@@ -14,8 +14,8 @@ | |||
use Cake\Core\Configure; | |||
use Cake\Core\Plugin; | |||
use Cake\Datasource\ConnectionManager; | |||
use Cake\Datasource\FactoryLocator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this class used in the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't think so. I'll need to check properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then why was the "use" statement added? 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good question 🤔
@@ -115,7 +115,4 @@ | |||
|
|||
Plugin::load('DebugKit', ['path' => ROOT, 'bootstrap' => true]); | |||
|
|||
DispatcherFactory::add('Routing'); | |||
DispatcherFactory::add('ControllerFactory'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have the replacement middlewares ready for these? If so we should add the new Plugin class introduced in 3.6 and add them to the middleware queue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've no idea if there are replacement middlewares. Do we really need middleware setup for unit tests? That sounds more like an integration test thing, as the test suite doesn't use any routing or controllers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right nvm, both are core filters so we don't need to bother.
Tests failing. |
Is the TypeMap deprecation new in Seems to be, 3.6.1 <- 3.6.2 Please let me know if you'd like to include commits to fix the TypeMap deprecation in this pull request, and I can add that. |
Yes please. |
Hey @ADmad can this be merged now? Are there any other changes you would like? |
Thanks! |
References #51
The aim of this pull request is to remove the usage of the deprecated Dispatcher Factory called in the Test bootstrap.
I have removed the lines and the test suite still passes. I believe this is because the plugin tests instantiate the classes directly, and so do not rely on the locator class. The
BootstrapTest
class does load the plugin, using it's own bootstrap file. So the class isn't required in the test suites bootstrap.