Improve Includes panel #598
Conversation
src/Panel/IncludePanel.php
Outdated
| { | ||
| foreach ($this->_composerPaths as $package => $path) { | ||
| if (strstr($file, $path)) { | ||
| return $package; |
There was a problem hiding this comment.
strstr returns string, but I just copy/pasted code from _isPluginFile .
There was a problem hiding this comment.
You return $package, but doc block states bool.
src/Panel/IncludePanel.php
Outdated
| * @return bool | ||
| */ | ||
| protected function _niceFileName($file, $type) | ||
| protected function _niceFileName($file, $type, $name = null) |
There was a problem hiding this comment.
no need for name to be nullable
There was a problem hiding this comment.
Function gets 2 arguments when $type is app/cake/root, $name is used when type is plugin/vendor.
There was a problem hiding this comment.
Then you should fix the doc block.
|
Nice work! |
src/Panel/IncludePanel.php
Outdated
| /** | ||
| * The list of Composer packages | ||
| * | ||
| * @var <type> |
Codecov Report
@@ Coverage Diff @@
## master #598 +/- ##
============================================
+ Coverage 51.13% 51.14% +0.01%
- Complexity 472 482 +10
============================================
Files 47 47
Lines 1412 1437 +25
============================================
+ Hits 722 735 +13
- Misses 690 702 +12
Continue to review full report at Codecov.
|
src/Panel/IncludePanel.php
Outdated
| @@ -156,7 +156,7 @@ protected function _isAppFile($file) | |||
| * Check if a path is from a plugin | |||
| * | |||
| * @param string $file File to check | |||
| * @return bool | |||
| * @return string|bool plugin name, or false if not plugin | |||
There was a problem hiding this comment.
I dont think an is...() method should ever return anything but bool. Why do you need to return the plugin name?
Then it should be called pluginFile() and return null otherwise.
src/Panel/IncludePanel.php
Outdated
| @@ -173,7 +173,7 @@ protected function _isPluginFile($file) | |||
| * Check if a path is from a Composer package | |||
| * | |||
| * @param string $file File to check | |||
| * @return bool | |||
| * @return string|bool package name, or false if not Composer package | |||
|
Once the tests are passing this looks pretty good to me. |
|
Tests are failing because CakePHP 3.6 deprecations. Tests on 3.4.0 ( Fix tests, then I could rebase to re-test on fixed version |
|
My mistake @garas. We have another branch that has no remaining deprecation errors, but it might be some time before that lands on master. |
|
I've rebased yesterday, tests passed. You can merge. |
I've tried to add test like
$this->assertArrayHasKey('cakephp/chronos', $data['vendor']);, but during test everything (cake/vendor/etc) is inside DebugKit directory, so everything goes into 'plugins -> DebugKit' group.But in normal app setup, everything goes into correct categories.