kodeine / laravel-meta

Fluent Meta Data for Eloquent Models, as if it is a property on your model
MIT License
400 stars 90 forks source link

Testing issue #92

Closed Temepest74 closed 2 years ago

Temepest74 commented 2 years ago

During some tests, the saveMeta() function won't be called.

I think it is due to self::$_isOververRegistered being always true, and I really think this should not be the case because, during some tests, the function getListener (from vendor/laravel/framework/src/Illuminate/Events/Dispatcher.php line 248) won't return any listeners, but sometimes will.

If you need a demo project of this bug, I could create one, I just hope that this package is still maintained. I solved this error for myself by treating $_isObserverRegistered as a non-static variable. I could create a pull request with this change, but I don't understand why it was static in the first place.

kodeine commented 2 years ago

@Temepest74 can you please submit the PR ?

Temepest74 commented 2 years ago

Yeah, but it is a good idea to make the variable nonstatic? Whybit was static in the first place?

siamak2 commented 2 years ago

Yeah, but it is a good idea to make the variable nonstatic? Whybit was static in the first place?

static variable are declared only once in current session. we need to add only one event listener for saved event so static variable is the best way. if you make it non static the event listener will be registered for every model class you instantiate. for example if you get 1000 model from database and access to their metas then you have 1000 event listener for saved event and every one of them will be called after saving the model and that's not ideal. can you share your unit test so I can reproduce the problem?

siamak2 commented 2 years ago

@kodeine according to laravel documentation it is best to register event listener in boot method of the model. for traits you should add trait name at end of boot so in your case method name should be bootMetable. if I read laravel's setUp method in Illuminate\Foundation\Testing\TestCase class correctly, laravel reboots entire application after each test this will remove all event listeners but won't affect static variable I think it's better to do something like this: protected static function bootMetable(){ self::saved(function ($model) { $model->saveMeta(); }); }

kodeine commented 2 years ago

@siamak2 would you mind doing a PR, im not setup at the moment to make changes. And i agree with you of keeping it static.

Temepest74 commented 2 years ago

Sorry guys, but my timezone didn't match with yours. Thank you @siamak2 , it solved my problem