Feature/generate providers #12
Conversation
| } | ||
|
|
||
| [$alias, $facade] = explode('@', $package->getFacade()); | ||
|
|
There was a problem hiding this comment.
Because of the array destructuring, Travis is failing.
Can i update the project to PHP 7.1? @kielabokkie @titopixelfusion
There was a problem hiding this comment.
@thijsbouwes Is it possible to rewrite this bit so it would also work on 7.0? If we want everyone to use it it might hold people back if they have to upgrade their local PHP version to 7.1 if they haven't already.
There was a problem hiding this comment.
Nice one @thijsbouwes. Just a comment on your question regarding PHP 7.1
|
@kielabokkie updated the compatible issue, runs fine on php 7 now 😃 |
| @@ -86,11 +87,11 @@ public function setVersion(string $version) | |||
| */ | |||
| public function getPath(string $path) | |||
| { | |||
| if(array_key_exists($path, $this->paths)){ | |||
| return $this->paths[$path]; | |||
| if(array_key_exists($path, $this->paths) === false){ | |||
There was a problem hiding this comment.
I'd simplify with !array_key_exists(...)
There was a problem hiding this comment.
@titopixelfusion I disagree, I find the ! is easily missed when skimming through code. I prefer the === false for readability.
There was a problem hiding this comment.
@kielabokkie I find it can be tricky, cos if you have the value on the right hand side, that means a silly mistake like $var = false (just one equal) is valid syntax and hard to debug. Either way, we should be consistent, in other place we have if(!class_exists($packageClass)) {
There was a problem hiding this comment.
Hmm, the both work @kielabokkie @titopixelfusion what do we implement?
There was a problem hiding this comment.
IMHO being consistent is the key, it comes down to a personal preference
There was a problem hiding this comment.
@thijsbouwes @titopixelfusion is right, we we do it in other places already so then it's better to stick to that. So I'm happy for it to be changed to !array_key_exists(...)
| */ | ||
| public function configure(Package $package, Framework $activeFramework) | ||
| { | ||
| if ($activeFramework->getName() !== 'Laravel') { |
There was a problem hiding this comment.
Instead of hardcode by name, Framework should have a method like configurable() that returns a boolean. In the Laravel one it would be true, all the others false, this way we would have more flexibility in our code
| */ | ||
| public function getNewConfig(string $configFile, Package $package) | ||
| { | ||
| $state = null; |
There was a problem hiding this comment.
I think this has to be part of the foreach body or we can have side effects depending on how the lines are ordered
| public function getNewConfig(string $configFile, Package $package) | ||
| { | ||
| $state = null; | ||
| $appConfig = file($configFile); |
There was a problem hiding this comment.
To make this test easier to test, I recommend to pass an array to the method. This reminds me the Symfony Yaml component, initially it accepted a filename, that it changed to only accept a string (the contents of the file). If we accept an array, we don't care about the source anymore, easier to test.
| { | ||
| $facade = $package->getFacade(); | ||
| if (isset($facade) === false) { | ||
| return; |
There was a problem hiding this comment.
doc block stays we always return a string
There was a problem hiding this comment.
Updated the doc blocks
| { | ||
| $provider = $package->getProvider(); | ||
| if (isset($provider) === false) { | ||
| return; |
There was a problem hiding this comment.
wrong return value
| @@ -86,11 +87,11 @@ public function setVersion(string $version) | |||
| */ | |||
| public function getPath(string $path) | |||
| { | |||
| if(array_key_exists($path, $this->paths)){ | |||
| return $this->paths[$path]; | |||
| if(array_key_exists($path, $this->paths) === false){ | |||
There was a problem hiding this comment.
@thijsbouwes @titopixelfusion is right, we we do it in other places already so then it's better to stick to that. So I'm happy for it to be changed to !array_key_exists(...)
Description 📖
barryvdh/laravel-debugbar, here you can see to registering for the providers/facades in action.Steps to test/reproduce 🚶
php skeletor project:create test123-> build a Laravel 5.4 project with thebarryvdh/laravel-debugbarpackageconfig/app.phpthe package reference should be in here../vendor/bin/codecept runall the test should pass.Todos ✅
Deployment notes
At the moment the registering of packages only works for
Laravel. You get a notification, when try to setup the packages onLumen.