search

laravel-ardent / ardent

Self-validating, secure and smart models for Laravel's Eloquent ORM
BSD 3-Clause "New" or "Revised" License
1.39k stars 211 forks source link

Use traits instead! #32

Closed atrauzzi closed 11 years ago

atrauzzi commented 11 years ago

Instead of using PHP's limited single-inheritance, ardent should use PHP's traits to accomplish all of it's magic.

http://php.net/manual/en/language.oop5.traits.php

rhukster commented 11 years ago

Traits are PHP 5.4 only. Laravel 4 should support PHP 5.3+

http://four.laravel.com/docs/installation

atrauzzi commented 11 years ago

Hmm, that's within Laravel itself. Although you can imagine how destructive it would be if every package required you to extend some class that extended Eloquent.

I almost think that all packages going forward ought to be using traits so that they can coexist. The fact that Ardent ties me to extending it's base class ensures that some day I may stop using it, or won't start using it at all.

laravelbook commented 11 years ago

@rhukster @atrauzzi I agree, traits are a wonderful addition to PHP. Unfortunately, more often than not our apps need to be deployed on external environments not entirely in our control. You'll be surprised to learn that more than 60% of (shared hosting) environments are still running PHP 5.2! PHP 5.4 adoption is less than 3% (while not an accurate picture, these stats do portray the general trend)

Distros like CentOS and Debian are slow to adopt the latest version (for good reason). I guess it'll be a while before 5.4 achieves critical mass... (and by that time we'll probably be way into PHP 6... ;) )

Flynsarmy commented 11 years ago

Could you not provide both a class and a trait?

HellPat commented 11 years ago

Doctrine extensions do so for a while now. https://github.com/l3pp4rd/DoctrineExtensions/tree/master/lib/Gedmo/SoftDeleteable/Traits

dwightwatson commented 11 years ago

Agreed, a class and trait would be nice in the meantime for people on 5.4.

atrauzzi commented 11 years ago

PHP 5.3 is EOL. I don't think the dinosaur excuse should prevent the addition of new features. Requiring that someone extend your class is an outdated and impractical practice in single inheritance languages.

thinksaydo commented 11 years ago

We're trying to use both Ardent and Cartalyst Sentry for User models, which isn't possible. So, we think Traits would work better for Ardent as it would allow mixing Ardent into a Sentry model.

igorsantos07 commented 11 years ago

Excuse me, I may have missed your point. Traits instead of...

-- Igor Santos Desenvolvedor web [enviado do celular] On Oct 3, 2013 1:02 PM, "Collin Schneider" notifications@github.com wrote:

We're trying to use both Ardent and Cartalyst Sentry - not an easy thing to do without some custom code. Traits would be a better fit for Ardent.

— Reply to this email directly or view it on GitHubhttps://github.com/laravelbook/ardent/issues/32#issuecomment-25633395 .

dwightwatson commented 11 years ago

You can copy the Sentry User model to your models folder, make it extend Ardent and then set that option in the Sentry package config.

This is a matter of traits vs. multiple inheritance, which PHP doesn't support.

Sent from my iPhone

On 4 Oct 2013, at 3:41 am, Igor Santos notifications@github.com wrote:

Excuse me, I may have missed your point. Traits instead of...

-- Igor Santos Desenvolvedor web [enviado do celular] On Oct 3, 2013 1:02 PM, "Collin Schneider" notifications@github.com wrote:

We're trying to use both Ardent and Cartalyst Sentry - not an easy thing to do without some custom code. Traits would be a better fit for Ardent.

— Reply to this email directly or view it on GitHubhttps://github.com/laravelbook/ardent/issues/32#issuecomment-25633395 .

— Reply to this email directly or view it on GitHub.

thinksaydo commented 11 years ago

One of the rare times when multiple inheritance might be nice. But, that's why we have traits, for things like Ardent. I may start converting Ardent into a trait-based solution and ship it back here as a pull request when ready.

thinksaydo commented 11 years ago

Just a note that we're moving off Cartalyst Sentry and switching to Confide instead, which uses Ardent out of the box. Problem solved for now.

onetdev commented 10 years ago

I know that this is a old topic but traits would be nice since Laravel 4.2 minimum PHP requirement is now 5.4

atrauzzi commented 10 years ago

All the more reason to ensure you're always up to date with the language. It's 2014, lazy hosting providers is not an excuse like it used to be in the 90s/00s. You just switch provider, not cripple your options.

igorsantos07 commented 10 years ago

Alexander, you sound like a hater. Stop it. No one ever said here traits are bad or they won't happen. Go with your rant to other older or conservative projects, please :)

I've never stopped to think on how traits would work here and I guess we would have much more important work to do... But that sounds like a great idea and, as usual, PRs are welcomed :)

Igor Santos -- Desenvolvedor Web [enviado do meu celular] On Aug 28, 2014 11:58 AM, "Alexander Trauzzi" notifications@github.com wrote:

All the more reason to ensure you're always up to date with the language. It's 2014, lazy hosting providers is not an excuse like it used to be in the 90s/00s. You just switch provider, not cripple your options.

— Reply to this email directly or view it on GitHub https://github.com/laravelbook/ardent/issues/32#issuecomment-53732017.

dwightwatson commented 10 years ago

I hope I'm not out of line by posting this here, but if you're looking for something like Ardent in a trait form you may want to check out my watson/validating package.

It doesn't support autohydration or simple relationship modelling because I felt that was out of scope of the trait, but you might find it useful.

atrauzzi commented 10 years ago

@igorsantos07 Just because you disagree doesn't mean I'm ranting, don't turn this into a beauty contest. More than enough people posted with valid explanations as to why traits should be a priority. Going after me like that is scapegoating. And moreover doesn't make the feature any less important.

This all said, I've since learned that libraries like this one are an antipattern and would discourage people from putting validation in their models. Amongst other things. So I'm not really so much interested in doing any trait conversions, sorry.

igorsantos07 commented 10 years ago

I don't disagree with traits, I disagree with your way of talking about it :P If you read again what I replied you'll see I'm not complaining on the idea, but rather about your way of expressing your thoughts, that were rather aggressive.

I restate it's a good idea and PRs are welcomed :)

Igor Santos -- Desenvolvedor Web [enviado do meu celular] On Aug 29, 2014 4:03 PM, "Alexander Trauzzi" notifications@github.com wrote:

@igorsantos07 https://github.com/igorsantos07 Just because you disagree doesn't mean I'm ranting, don't turn this into a beauty contest. More than enough people posted with valid explanations as to why traits should be a priority. Going after me like that is scapegoating. And moreover doesn't make the feature any less important.

This all said, I've since learned that libraries like this one are an antipattern and would discourage people from putting validation in their models. Amongst other things.

— Reply to this email directly or view it on GitHub https://github.com/laravelbook/ardent/issues/32#issuecomment-53917426.