Skip to content
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

Fix logger for case Yii::$app lost on process exiting #6549

Closed

Conversation

tanakahisateru
Copy link
Contributor

Logger::flush() might be called by register_shutdown_function() when application instance has gone.

In fact, Codeception extension clears Yii::$app on every test cases.

Yii::$app = null;

Logger::flush() might be called by register_shutdown_function() when application instance has gone.
@samdark samdark added the status:to be verified Needs to be reproduced and validated. label Dec 17, 2014
@samdark samdark added this to the 2.0.x milestone Dec 17, 2014
@cebe
Copy link
Member

cebe commented Dec 17, 2014

Why do you have the logger configured in your Tests? Should be skipped IMO because it will result in n shutdown functions to be registered for n tests...

@tanakahisateru
Copy link
Contributor Author

Which does "Tests" point (a) my own app's tests using Codeception or (b) test added while this PR?

(a) I'm using single logger instance stored in 'Yii::$_logger' commonly.
(b) There are 3 new Logger in other tests. Already 3 shutdown function are registered before my modification. (I think that it's better if they are shared)

My problem is that actually PHP crashed at the end of codecept run.

FAILURES!
Tests: 4, Assertions: 11, Failures: 2.
PHP Fatal error:  Call to a member function getRequest() on null in /.../vendor/yiisoft/yii2/log/Target.php on line 269
PHP Stack trace:
PHP   1. {main}() /.../vendor/codeception/codeception/codecept:0
PHP   2. Symfony\Component\Console\Application->run() /.../vendor/codeception/codeception/codecept:27
PHP   3. yii\log\Logger->flush() /.../vendor/yiisoft/yii2/log/Logger.php:0
PHP   4. yii\log\Dispatcher->dispatch() /.../vendor/yiisoft/yii2/log/Logger.php:170
PHP   5. yii\log\Target->collect() /.../vendor/yiisoft/yii2/log/Dispatcher.php:183
PHP   6. yii\log\FileTarget->export() /.../vendor/yiisoft/yii2/log/Target.php:111
PHP   7. array_map() /.../vendor/yiisoft/yii2/log/FileTarget.php:99
PHP   8. yii\log\Target->formatMessage() /.../vendor/yiisoft/yii2/log/FileTarget.php:99
PHP   9. yii\log\Target->getMessagePrefix() /.../vendor/yiisoft/yii2/log/Target.php:250

My app has no special config.

[
    'components' => [
        'log' => [
            'traceLevel' => YII_DEBUG ? 3 : 0,
            'targets' => [
                [
                    'class' => 'yii\log\FileTarget',
                    'levels' => ['error', 'warning'],
                ],
            ],
        ],

Perhaps some of my functions called Yii::error() or Yii::warning() then the test runner terminated while those messages are remaining in the buffer.

@tanakahisateru
Copy link
Contributor Author

Similar result would be displayed by below code.

Yii::error("test");
Yii::$app = null;
exit;

@Ragazzo
Copy link
Contributor

Ragazzo commented Dec 17, 2014

@cebe to be true i already faced with 2 - 5 such examples here in issues about testing you can search it if needed )

@cebe
Copy link
Member

cebe commented Dec 17, 2014

I am asking why you need to enable the log dispatcher in the test application? If you create a new instance for each test case you will spam the logfiles which you do not need and it will also make your tests slower. To avoid this problem, just do not configure a log component in application.

In normal envirnoment there is no reason to unset Yii::$app and it is not only the logger that relies on an application instance being available in Yii::$app.

@cebe
Copy link
Member

cebe commented Dec 17, 2014

related to #6102

@Ragazzo
Copy link
Contributor

Ragazzo commented Dec 17, 2014

I am asking why you need to enable the log dispatcher in the test application?

It is the problem that developers dont throw exceptions (fail fast , let it fail) and only use Yii::log and so on , in their applications everywhere . That is the reason as you can see )

@cebe
Copy link
Member

cebe commented Dec 17, 2014

but are you using these logs when running tests to determine the state of the tests or evaluate them?
I do not see any value in this when each test has its own log router with default config...

@Ragazzo
Copy link
Contributor

Ragazzo commented Dec 17, 2014

i dont use this logs , i just throw an exception if something goes not according contracts )

@cebe
Copy link
Member

cebe commented Dec 17, 2014

yeah, this questions was more directed to the issue starter :)
so, @tanakahisateru can you please explain why you need this (#6549 (comment))?

@tanakahisateru
Copy link
Contributor Author

@cebe Because current both application template basic and advanced are reusing app config and do not redefine log dispatcher for tests.
Though any logs are not useful while running tests. But it's a different issue from such as null pointer exception, I think.
Not for only tests, null check is better practice against unexpected situation.

@klimov-paul
Copy link
Member

It is already resolved by b338240

@cebe cebe removed this from the 2.0.x milestone Apr 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:to be verified Needs to be reproduced and validated.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants