fix YII_ENV constant check and use dirname #39
Conversation
allyraza
commented
Jun 26, 2018
allyraza
commented
| Q | A |
|---|---|
| Is bugfix? | yes |
| New feature? | no |
| Breaks BC? | no |
| Tests pass? | yes |
| Fixed issues | comma-separated list of tickets # fixed by the PR, if any |
| require __DIR__ . '/../vendor/autoload.php'; | ||
| require __DIR__ . '/../config/env.php'; | ||
| require __DIR__ . '/../vendor/yiisoft/yii2/Yii.php'; | ||
| require dirname(__DIR__) . '/vendor/autoload.php'; |
There was a problem hiding this comment.
I find __DIR__ . '/../' more readable than the usage of dirname().
There was a problem hiding this comment.
I could also say the same about dirname(__DIR__) being more readable to me, why I sent a PR is more about being consistent dirname(__DIR__) is being used everywhere other than index.php take a look at config/web.php and config/console.php
There was a problem hiding this comment.
I'd prefer to change those config/web.php and config/console.php
I.e. if there would be something like __DIR__ . '/../../' you'd need to write dirname(dirname(__DIR__)).
Or we introduce a variable like $rootPath.
There was a problem hiding this comment.
Since PHP 7.0 dirname() has a second parameter for levels, so you can use dirname(__DIR__, 2).
I'm for creating APP_ROOT constant and use it as a root to build paths. DRY.
| @@ -4,6 +4,7 @@ | |||
|
|
|||
| use Yii; | |||
| use yii\base\Model; | |||
| use app\models\User; | |||
There was a problem hiding this comment.
While this is currently required for the template to work I'd prefer to remove the User model and login completely and rather have something in the docs which describes how to install a full-featured module such as yii2-usuario.
There was a problem hiding this comment.
I don't think it is very wise to have a demo app which does not work or throws an error, it should work out of the box and docs should indicate how to use a different user module.
There was a problem hiding this comment.
Details added in yiisoft/app#40
There was a problem hiding this comment.
Not a question of the current PR for sure.
config/console.php
Outdated
| @@ -57,11 +57,11 @@ | |||
| */ | |||
| ]; | |||
|
|
|||
| if (YII_ENV_DEV) { | |||
| if (YII_ENV == 'dev') { | |||
config/web.php
Outdated
| @@ -73,18 +73,18 @@ | |||
| 'params' => $params, | |||
| ]; | |||
|
|
|||
| if (YII_ENV_DEV) { | |||
| if (YII_ENV == 'dev') { | |||
| @@ -4,6 +4,7 @@ | |||
|
|
|||
| use Yii; | |||
| use yii\base\Model; | |||
| use app\models\User; | |||
There was a problem hiding this comment.
Not a question of the current PR for sure.
|
Missing |
hiqsol
force-pushed
the
master
branch
2 times, most recently
from
458de03
to
e0c99f4
Compare
|
Sorry, closing this PR. |
