This trait is causing an N+1 query issue

[denjaland]

This trait is causing an N+1 query issue

[aocneanu]

Next week this package will have a second trait for less generic cases. It will be also a lot more performant. Stay tuned :D

[aocneanu]

causes for an N+1 query issue for each model we apply this to.

Good catch!

Will look into it.

[aocneanu]

@denjaland Please peak into the code and see if you find anything wrong.

Based on the new findings and on our experience using versioning I'm finding the new approach more straightforward, without the need to keep a "virtual property".
Of course it can be added as an accessor as you specified above.

I also addressed the migration name.

PS This is a breaking change for Enso.

[denjaland]

Hi @aocneanu,

What a fast turnaround!
I just checked your code, and I'm not sure whether you've tested it (I didn't test it myself, so my apologies for giving some randome feedback), but I don't think this will work to be honest.

If you check the code for Eloquent, you'll notice that the framework always throws the event retrieved prior to eager load the relations defined in $with or added to the Builder through the ->with() method.

This means that your code in the static::retrieved() even listener will always execute before relations have eager loaded.

Since the first thing you do in that listener is checking whether $model->versioning is set, that call alone will already query the database to get the versioning record for the model.

So in a way your changes make it worse, because you have now added the eager loading of versioning in the boot method, meaning you have now gone from an N+1 to an N+2 queries issues as I explained in laravel/framework#29630

The main issue here is that you want to support the ability to override the name of the attribute. If it is an option to not wanting to do that, I think an accessor is possible, combined with eager loading so that you can get rid of the retrieved listener alltogether. I don't think you need to do anything there already apart from the fact that you now want to set the configured attribute...

[aocneanu]

If you check the code for Eloquent, you'll notice that the framework always throws the event retrieved prior to eager load the relations defined in $with or added to the Builder through the ->with() method.
This means that your code in the static::retrieved() even listener will always execute before relations have eager loaded.

I did the check before doing the refactor.

Until we hear from someone from the Laravel team I'm not sure what we can do regarding the way the framework works now. I did the refactor assuming they are willing to work on this.

BTW, you should use the issue template

Also the new version is less complicated, removing the need for a "virtual property" and in fact saves one or two queries in comparison with the former one, depending on the use case.

I've also tested it.

[denjaland]

Okay - sounds great.
I think we might be going for our own implementation based on what you set up though. We don't need the ability to override property name, so we will probably be able to make it a bit simpler.

Yours works really great to be honest - very trustworthy. I just noticed that one of our pages went from an average of 45 queries to the database to 600+ queries, and that is something we want / need to avoid.

I just added feedback to that issue. that template didn't really ask me for much more unless I'm not clicking the right buttons :-)

[denjaland]

I haven't tested if it is an issue, but might be better to change this line to

    if(!in_array('versioning', $this->with)) $this->with[] = 'versioning';

I don't know how Laravel reacts when the same relation is added in that array twice (for instance when a developer added it on the model himself already...) - it might very well just querying them twice... I haven't checked deeper into the code, but there is definitely no check here.

[aocneanu]

@denjaland you can play with the new optimized version. We have now a custom event ;)

[denjaland]

Will definitely have a look into it!

[aocneanu]

Will close because the latest version fixes this.

Take a look at laravel-enso/versions too.

[denjaland]

Hiya - what is the difference between versionings and versions? Am I right in thinking tht the only difference is that versions saves the attribute on the actual model whereas versioning has its own model to keep track of model versions? Or is there anything else?