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

[5.7] Add AsPivot trait #25851

Merged
merged 2 commits into from
Oct 2, 2018
Merged

Conversation

mathieutu
Copy link
Contributor

@mathieutu mathieutu commented Oct 1, 2018

Hi,

TL;DR:

  • Just moved the core of the Pivot class to a trait.
  • No breaking change.
  • All tests green.

Why:

In fact, like a lot of developers, I have my own abstract Model class which is extended by all my eloquent models. It allows me to add directly some methods that all can share.

Extending the pivot class prevent this classic way of doing.

It's why I've added this trait: to allow devs to implement the pivot methods to any of their Model.

Thanks.
Matt'

@driesvints driesvints changed the title Add IsPivot trait. [5.7] Add IsPivot trait. Oct 1, 2018
@deleugpn
Copy link
Contributor

deleugpn commented Oct 2, 2018

The naming isn't that great, though, it makes my mind go to Boolean checks.

Here's two suggestions:

use AsPivot;
use Pivot;

@mathieutu
Copy link
Contributor Author

@deleugpn no problem with changing that. I like the "use as pivot" way.

@taylorotwell
Copy link
Member

I'm fine with AsPivot.

@mathieutu mathieutu changed the title [5.7] Add IsPivot trait. [5.7] Add AsPivot trait. Oct 2, 2018
@taylorotwell taylorotwell merged commit 27ab200 into laravel:5.7 Oct 2, 2018
@LukeTowers
Copy link
Contributor

@mathieutu @idsulik any interest in adding a matching AsMorphPivot trait?

To respond to @taylorotwell's original request for justification in #34488 (see comment) you would need to use AsMorphPivot as a trait for the same reason that you would need to use AsPivot as a trait: you are extending the base Model class with a customized Model class and need the Pivot & MorphPivot models to extend your custom Model class instead of the base Laravel Model class.

You can't just extend the base MorphPivot model in that case because the base MorphPivot class extends the base Pivot class which extends the base Model class. Instead, you would need to create your own MorphPivot class that extends your own Pivot class that extends your own Model class that extends the base Laravel Model class. One such example of the need for custom base Model classes is the Winter CMS base Model class that adds our dynamic class extension system and custom events to all models within Winter CMS (see https://github.com/wintercms/storm/blob/develop/src/Database/Model.php).

Currently, we have to extend our custom Pivot model and reimplement all of the Laravel logic (see https://github.com/wintercms/storm/pull/64/files#diff-44330773d7e77b50c2bfd6a051c1ff1c0b538dd7d03caf65062649d7798bfee9), adding AsMorphPivot as a trait would make it so that we didn't have to do that.

One difference that I would suggest from the original #34488 is that the AsMorphPivot trait shouldn't itself include the AsPivot trait since I think the only valid use case for this change is the one I described above and that use case does not benefit from having the AsMorphPivot trait automatically include the AsPivot trait.

@GrahamCampbell GrahamCampbell changed the title [5.7] Add AsPivot trait. [5.7] Add AsPivot trait May 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants