Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Using traits over extends for BaseObjects? #523

Open
gossi opened this Issue · 32 comments

9 participants

@gossi

I think I read about this idea for propel somewhere (however can't find the reference right now) to use traits over extends for BaseObjects.

Old Behavior:

# Book.php
class Book extends BaseBook {...}

# Base/Book.php
class Book extends implements ActiveRecordInterface {...}

New Behavior:

# Book.php
class Book implements ActiveRecordInterface {
    use BookTrait;
    ...
}

# Trait/Book.php
trait BookTrait {
    use ActiveRecordTrait;
    ...
}

That way the extends is opened up for the domain model and propel is "plugged-in" as trait.

@ivosabev

I am two hands up for Propel being injected as a Trait.

@staabm
Collaborator

:+1:

@jaugustin
Collaborator

@gossi Could you do fast pros / cons comparison about the change ?

For example with trait actual implementation of inheritance won't work :

A extends BaseA extends B extends BaseB

With traits we can't do that.

@ivosabev

The same effect is achievable through composition. The problem is not about technical possibility, rather than design and the overall philosophy of propel. A good example of the composition approach would be what AR in Rails look like.

@gossi

I totally agree with @ivosabev here. It is about the design of the generated propel code. Because something is technically possible, doesn't mean it must be done that way. That is also the reason why A extends BaseA extends B extends BaseB feels wrong in so many ways.

What looks right to me, is the separation of the application model and the propel generated classes (let's call them propel stubs) that somehow should get plugged into the application model. Composition is one way to do that, however I have no idea how AR in rails look like but I can guess some kind of wrapper is needed to pass through the functionality from the application model to the propel stubs (Adapter Pattern). While this can be realised with either interfaces or multiple inheritance, neither of them looks like a good solution to me. Given that most design patterns have their origin in java, neither multiple inheritance nor traits could be thought of, when designing design patterns (I also think, traits are the better solution than multiple inheritance).
The extends also should exclusively belong to one model, unless one model extends another. I don't see that neither my application model extends the propel stubs in any nor the other way round. So, the propel stubs will get plugged into my application model and that's why I think traits are the best solution here.

PS. I hope I didn't start a war about design patterns now *duck*

@jaugustin
Collaborator

@gossi I am ok with a new design, but we should look at all covered cases before switching ;)

@staabm
Collaborator

In my opinionen it would be a big benefit when the propel-user can decide which classes in his model extend which other class.

ATM the user cannot use inheritance for his domain logic, because propel "needs it for technical reasons".

I would love to the "propel-plug" implemented on another way, but of course everything must work as before. I think Traits could do this things for us very well... not sure if there is a even better one though.

Doctrine uses Annotations to achieve the wiring of technical stuff into the domain model (which has the benefit that annotations can also be used for several things on on severall levels; e.g. class-level, method-level, property-level... while Traits work only on the class level but could be faster because they are implemented on a language level...)

I am open for discussion ;)

@gossi

As far as I know, Annotations are done in docblock and must be parsed somehow. That doesn't sound fast. Can somebody with the more inner propel knowledge give some reasons for modifications on class-, method- and property-level?

Another idea for using traits (but that might be totally independent of that) is behaviors. Behaviors can plug themselves into generated classes/traits as traits.

@jaugustin
Collaborator

@staabm if you are talking about doctrine 2 I think the annotation could be compare with propel schema.xml

It will only describe field/column and relations.

But in doctrine 2 which is a Data mapper the all the logic is out of the object, in the Entity manager.
Where in propel the logic is mostly in the object.

@jaugustin
Collaborator

@fzaninotto @willdurand could you give your point of view on this ?

@willdurand
Owner

Traits are not silver bullets.

@staabm
Collaborator

@willdurand and whats your opinion regarding the topic? ;-)

Would you support a change away from a BaseModel class to trait usage?

@willdurand
Owner

I'd say no.

@staabm
Collaborator

Any arguments?

@marcj
Owner

That's actually exactly what Propel2 should do. I'd give here huge +1.

1.
Propel generates code not because we need the inheritance but because we want to be fast and IDE friendly.
We had in Propel1 to use the classical vertical inheritance because PHP < 5.4 does not support horizontal inheritance/code reuse. Now, in Propel2 we could do it the right way and let the actual model be stupid and just inject the AR via a Trait. This gives users the ability to do whatever they want with their model and when they need the AR "mixins" they can just inject it.

2.
Another huge benefit is that traits allow us to rename methods being used in the actual model.
We have at the moment sometimes the case that some behavior are not compatible between each other because they create the same method name. That is difficult to track inside a particular behavior. The better way would be that each behavior creates its traits for the model. Example:

   NodeModel with behavior: sortable, nestedset (assume its legit ;)

   Trait NodeModelNestedSet {
        ...
        public function getScopeValue();
    }

    Trait NodeModelSortable {
        ...
        public function getScopeValue();
    }

    Trait NodeModelTrait {
        use NodeModelEncapsulation,  // getters&setters
            NodeModelActiveRecord,     // save/delete etc
            NodeModelNestedSet,         // nested-set behavior stuff
            NodeModelSortable {           // sortable behavior stuff
                NodeModelSortable::getScopeValue as getSortableScopeValue;
        }
     }

     Class NodeModel {
         use NodeModelTrait;
     }

The NodeModelTrait use statement can then be generated by examining of all methods of the trait via the reflection api and generate a rename as map (maybe based on some rules defined by the behavior).
With the possibility to use traits it can even be up to the user itself. He can for example rewrite his model:

     Class NodeModel {
        use NodeModelEncapsulation,
            NodeModelActiveRecord,
            NodeModelNestedSet,
            NodeModelSortable {
               NodeModelSortable::getScopeValue as getScopeSortableValue;
        }
     }

Handling name collisions is then a ease.

3.
Next thing is that the builder itself would be more readable and maintainable because each behavior creates its own traits and that's it. No rewriting of a huge base model anymore.

4.
I believe it's then also easier to integrate ODM/NoSQL stuff into a model via traits. But not sure here.

@marcj
Owner

5.
It would be possible to create model classes without AR methods but only with the encapsulation stuff, which is sometimes pretty handy (performance/memory)

@willdurand
Owner

As far as you use traits, you introduce coupling to your code. It is not a silver bullet. It may be better to provide "sane" model classes, but I am not sure about that.

@marcj
Owner

@willdurand, what are your concerns exactly?

@willdurand
Owner

We should not abuse of traits, just because it looks cooler. By the way, traits in PHP are actually not traits but mixins with the ability to solve conflicts explicitly.

@marcj
Owner

We should not abuse of traits, just because it looks cooler.

I'm not sure why you're thinking we want to introduce Trais because it looks cooler? My points above do not contain such a 'cooler' argument. Are my actual points not valid to you?

By the way, traits in PHP are actually not traits but mixins with the ability to solve conflicts explicitly.

Yes, exactly what I've said and what the advantage is over classical inheritance.

@staabm
Collaborator

I am with @marcj here. Traits seem like to fit perfectly for our use case.

@willdurand have a look at other ORM Frameworks. They worked very hard to remove the technical requirement so users can build their domain model like they wan't without loosing the inheritance capabities.

@gossi

Maybe this metaphor helps in some way. I sometimes think of traits as lego bricks, while classes are finished buildings, like a house or a train. So, one property of traits is the multi usage. They can be used in multiple classes, whereas a class in that context has more like a single usage.

Will propel deliver finished products (classes) or bricks (traits) the end-user will finally use? I'm with bricks here.

On behaviors. I dunno wether the example from @marcj is a bit over-engineered here (I have no final opinion yet). Wether behaviors are distributed as trait or not, my opinion is: If the trait only delivers a single functionality to the final propel stub, then a trait won't make that much sense. I also see some use-cases for traits in behaviors:

  1. Some behaviors may distribute traits as their runtime (not generated), which can be mixed in to that final propel stub.
  2. Also I see a use-case where a (database) behavior might generate a trait, based on the database-related data that got mixed in to the final propel stub
  3. (Optional) Behaviors can deliver wrapper methods to the final stub, that call methods from traits with parameters by the calling model

I think traits are a very valuable functionality, especially for behavior authors, which also deserves a good API, so behaviors can make the best usage out of it and reliably modify desired objects.

@marcj
Owner

@gossi, yes, I think we do all agree that traits are mixins (which are "lego bricks" if you want).
Most behavior do not provide only one method, but even they'd, I don't see why traits with only one method make no sense (for example Mootools has a mixin called Options which does provide only one method setOptions : )

However, I think Propel's "Behavior" are actually exactly what mixins are: They inject new functionalities into a object, so using traits here makes totally sense to me. The ActiveRecord stuff is actual also something we do inject and is not necessarily required (maybe for the feature, when we allow to separate those things optionally to have mass-insertion and stuff like that). I agree, extracting the encapsulation methods into a trait is indeed over-engineered.

@gossi gossi referenced this issue
Open

Satisfy Behavior Authors #533

1 of 6 tasks complete
@gharlan

:+1: for traits

@niels-nijens

I've had a lot of discussions about traits with my development team.
The only valid use case for traits we have come up with is injecting code into an existing class inheritance model. Though, even this is a questionable use case as you might have problems in your class model already. Other than that we believe that traits provide bad coding practices.

I believe the generated model classes are one of the main unique selling points for using Propel. If I wanted something else I would have gone with Doctrine.

So, a :thumbsdown: from me and the rest of my team.

@willdurand
Owner

:-1:

@willdurand willdurand closed this
@marcj
Owner

I don't think we should just close this issue @willdurand. We showed a lot cases where traits are useful and better than the current inheritance approach. The guys are against it haven't showed yet a concrete argument.

So please, lets be objective and try to find and weigh out all pro and const.

@marcj marcj reopened this
@marcj marcj added the Enhancement label
@gossi

Another use-case I observed lately is updating.
Given the following a behavior or propel code is beeing updated and a new version is out. So far the end-user needs to recompile its model to get the code changes into his code. While this may be a common practice for propel itself, it is not for behaviors. You would just run with composer update and you think you are done while actually you would have needed another call to update your propel models while the composer update actually updated one of the behaviors.
The best notification here would be during the composer installation itself, with a line inserted into the composer output from the behavior itself, that reminds the user to update the propel model (similarly to how brew handles installation, where some software gives you a hint to run a special command, brew couldn't do itself, typically some sudo commands).
This would also mean a special Propel/Behavior installer, that intersects into the composer installer to receive the notifications calls from propel and either pass it on the the behavior or outputs themselves (Just to make proper notifications).

@willdurand
Owner

Two good reads that will, hopefully, allow you to understand why I am against traits:

@fzaninotto

I'm quite in favor of traits. They have drawbacks against aggregation, but they are much easier to deal with (no need for a DIC, faster as they don't require instances). Also, they have advantages over inheritance, which is the reason why they are considered here.

Behaviors is a perfect use case, in fact even Doctrine has behaviors (!) with traits. I'd argue that the ActiveRecord and the ActiveQuery functionality could also be traits, but generated traits. The code we generate into the BaseObject and BaseQuery should be moved to traits, to let the model use its own inheritance pattern.

@willdurand Bejamin's example is flawed, it is not a proof at all. Besides, Propel2 still allows some static method calls, am I right?

@gharlan

I like the idea a lot, but there are some problems. For example it would be more tricky to overwrite a base method..

class Book
{
    use BookTrait;

    public function setTitle($title)
    {
        // parent::setTitle($title) does not work here
    }
}

// So it is necessary to use an alias for the base method:

class Book
{
    use BookTrait {
        setTitle as private baseSetTitle;
    }

    public function setTitle($title)
    {
        $this->baseSetTitle($title);

        // ...
    }
}
@marcj
Owner

2012/07/09 has been announced already that Propel2 will use traits, http://propelorm.org/blog/2012/07/09/propel2-what-why-when.html

The last great improvement in Propel2 is the support of traits. Propel2 will be able to generate traits, to get rid of the classic inheritance and the base class, so that you will be able to use POPO (or domain objects) with Propel2!

We should stay with this decision and introduce glory traits. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.