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

[Yii2] Reset global event handlers #4621

Merged
merged 1 commit into from Nov 13, 2017

Conversation

Projects
None yet
4 participants
@SamMousa
Collaborator

SamMousa commented Nov 13, 2017

Global event handlers should not persist between tests.
The if statement is needed since the offAll() function exists only since 2.0.10 of the framework.

Ping @samdark

Reset global event handlers
Global event handlers should not persist between tests.
The `if` statement is needed since the `offAll()` function exists only since 2.0.10 of the framework.

@Naktibalda Naktibalda changed the title from Reset global event handlers to [Yii2] Reset global event handlers Nov 13, 2017

@samdark samdark added the Yii label Nov 13, 2017

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Nov 13, 2017

Collaborator

Looks right to me but I think it may change behavior people are relying to...

Collaborator

samdark commented Nov 13, 2017

Looks right to me but I think it may change behavior people are relying to...

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Nov 13, 2017

Member

Makes sense to me

Member

DavertMik commented Nov 13, 2017

Makes sense to me

@DavertMik DavertMik merged commit b1d96da into Codeception:2.3 Nov 13, 2017

3 of 4 checks passed

wercker/build Wercker pipeline failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
semaphoreci The build passed on Semaphore.
Details
@lubobill1990

This comment has been minimized.

Show comment
Hide comment
@lubobill1990

lubobill1990 Dec 17, 2017

@samdark @DavertMik @SamMousa
Global event being removed in this brutal way may cause problems:

For example, I add events to a class after the class definition in the same file. Such event is business logic of models and should not been removed after running a test case.

Thanks to reading the change log before updating codeception, or it will take me a lot of time to figure out why my test cases fail.

Class level event should be considered as maintaining class level relationship, it should be consistent between different test cases.

I guess the idea to remove class level event after running a test case is because the class level event added in test case A will break test case B.

But such event which is not compatible between different test cases should not be added as class level event, it's misusing class level event.

So I propose not to remove class level event after running a test case.

lubobill1990 commented Dec 17, 2017

@samdark @DavertMik @SamMousa
Global event being removed in this brutal way may cause problems:

For example, I add events to a class after the class definition in the same file. Such event is business logic of models and should not been removed after running a test case.

Thanks to reading the change log before updating codeception, or it will take me a lot of time to figure out why my test cases fail.

Class level event should be considered as maintaining class level relationship, it should be consistent between different test cases.

I guess the idea to remove class level event after running a test case is because the class level event added in test case A will break test case B.

But such event which is not compatible between different test cases should not be added as class level event, it's misusing class level event.

So I propose not to remove class level event after running a test case.

lubobill1990 added a commit to lubobill1990/Codeception that referenced this pull request Dec 17, 2017

@SamMousa

This comment has been minimized.

Show comment
Hide comment
@SamMousa

SamMousa Dec 17, 2017

Collaborator

@lubobill1990 it sounds like you are violating a lot of standard practices...
Including a file with a class shouldn't have side effects like registering events. I think this is even strictly forbidden by the autoloading PSR.

Collaborator

SamMousa commented Dec 17, 2017

@lubobill1990 it sounds like you are violating a lot of standard practices...
Including a file with a class shouldn't have side effects like registering events. I think this is even strictly forbidden by the autoloading PSR.

@lubobill1990

This comment has been minimized.

Show comment
Hide comment
@lubobill1990

lubobill1990 Dec 18, 2017

@SamMousa
Hi Sam,
Because class file can't be included twice which will emit fatal error, so I think it's OK to add class level event below class definition.

I also use class level event in other scenarios:

For example, in configuration file

'on beforeRequest' => function () {
        \yii\base\Event::on(\yii\base\Controller::class, \yii\base\Controller::EVENT_BEFORE_ACTION, function () {

With listening class level event by this way, I don't have to create a parent class to be extended by each controller.

But in this scenario, class level event still can't be all removed after running a test case.

lubobill1990 commented Dec 18, 2017

@SamMousa
Hi Sam,
Because class file can't be included twice which will emit fatal error, so I think it's OK to add class level event below class definition.

I also use class level event in other scenarios:

For example, in configuration file

'on beforeRequest' => function () {
        \yii\base\Event::on(\yii\base\Controller::class, \yii\base\Controller::EVENT_BEFORE_ACTION, function () {

With listening class level event by this way, I don't have to create a parent class to be extended by each controller.

But in this scenario, class level event still can't be all removed after running a test case.

@SamMousa

This comment has been minimized.

Show comment
Hide comment
@SamMousa

SamMousa Dec 18, 2017

Collaborator

Actually it can, request / application is recreated thus the event is registered again.

Even worse, not removing it will make the events fire twice.

Collaborator

SamMousa commented Dec 18, 2017

Actually it can, request / application is recreated thus the event is registered again.

Even worse, not removing it will make the events fire twice.

@SamMousa SamMousa deleted the SamMousa:patch-2 branch Dec 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment