search

kirkbushell / eloquence

A drop-in library for certain database functionality in Laravel, that allows for extra features that may never make it into the main project.
MIT License
537 stars 58 forks source link

fix UUIDModel boot method to laravel way of booting traits #25

Closed epic-kaso closed 8 years ago

kirkbushell commented 9 years ago

Hi!

Could you elaborate as to why this is the "laravel way"?

epic-kaso commented 9 years ago

if not done like this then when a user overrides the boot method in there own model, this done doesnt get called. doing it like this ensures that the boot method is called regardless of whether the user overrides the boot method or not in there eloquent model

kirkbushell commented 9 years ago

@kasoprecede47 what makes it the laravel way though?

The reason why it's boot is intentional - to ensure default behaviour. If users want to overload boot, then they should also include any other methods they would like to include.

Laravel won't automatically boot the method you're suggesting, as far as I'm aware, and even if it did, users still need to include parent::boot anyway.

epic-kaso commented 9 years ago

@kirkbushell Laravel automatically boots it. see http://www.archybold.com/blog/post/booting-eloquent-model-traits

epic-kaso commented 9 years ago

I tried to use the lib as it currently is and because I also have a boot override on my model, It wasnt calling the boot method in the trait. I called parent::boot

kirkbushell commented 9 years ago

The easiest way to get around that for now, is to simply alias the trait's boot method to something else, write your own boot method and then include the code required in UUIDmodel.

I can't merge this in just yet, as the change really should be without the boot() call in the new method you've highlighted. If you can make that change then I can merge in. I won't release until probably 1.5 along with some other changes, as it's also an API change and is separtae from the other traits, so I'll need to look at resolving those, as well.

Thanks for the heads up :)

kirkbushell commented 8 years ago

@kasoprecede47

epic-kaso commented 8 years ago

@kirkbushell yeah what's up?

kirkbushell commented 8 years ago

@kasoprecede47 never got a response from you regarding previous comment.

epic-kaso commented 8 years ago

@kirkbushell About that, I really didn't understand that. Did you mean i should remove the "parent::boot()" call ?

kirkbushell commented 8 years ago

@kasoprecede47 calling the boot method isn't necessary within the changes you've made.

epic-kaso commented 8 years ago

@kirkbushell Okay I understand now. I will make the changes and push asap

kirkbushell commented 8 years ago

@kasoprecede47 are you still looking at updating this?

epic-kaso commented 8 years ago

@kirkbushell I've made the update, I really don't know what else I should do

kirkbushell commented 8 years ago

@kasoprecede47 ah, thanks. I'll check it out today and merge. Thanks! :)

kirkbushell commented 8 years ago

Merged in, thanks for this - I like it.

epic-kaso commented 8 years ago

@kirkbushell you are welcome, I'm glad you like it.