j4mie / paris

A lightweight Active Record implementation for PHP5, built on top of Idiorm.
http://j4mie.github.com/idiormandparis/
996 stars 131 forks source link

Single Table Inheritance Support #77

Closed nbonamy closed 10 years ago

nbonamy commented 10 years ago

Hello,

just considering migrating a small code base to Paris but for this I need Single Table Inheritance Support (http://martinfowler.com/eaaCatalog/singleTableInheritance.html). How hard would this be to implement ?

Probably have to overwrite ORMWrapper::_create_model_instance. Am I right ?

Thanks, Nicolas

treffynnon commented 10 years ago

Never really given this any thought. It is definitely outside the remit of Paris and Idiorm. Perhaps someone else has so I'll leave the ticket open for now.

tag commented 10 years ago

I simply use two models, one for the parent table, and one for the child table.

If you want it a bit more transparent at your model layer, you could override __get() and __set() in both the parent and the child to automatically load and check the other. May be a good use of traits.

nbonamy commented 10 years ago

Did a quick modification. Please tell me if you think this is worth it: I can do a fork/pull request.

So I modified ORMWrapper::_create_model_instance this way

       protected function _create_model_instance($orm) {
            if ($orm === false) {
                return false;
            }

            $class_name = $this->_class_name;
            if (property_exists($this->_class_name, '_subclasses')) {
                $properties = get_class_vars($this->_class_name);
                $subclasses = $properties['_subclasses'];
                foreach ($subclasses as $subclass_name => $selectors) {
                    $match = true;
                    foreach ($selectors as $attr => $value) {
                        if ($orm->$attr != $value) {
                            $match = false;
                        }
                    }
                    if ($match === true) {
                        $class_name = $subclass_name;
                        break;
                    }
                }
            }

            $model = new $class_name();
            $model->set_orm($orm);
            return $model;
        }

To activate it just add a static variable to your model class (the same way $_table works):

class Competition extends Model
    public static $_subclasses = array(
        'CompetitionScore' => array('type' => 1),
        'CompetitionDiff' => array('type' => 2),
        'CompetitionSet' => array('type' => 3)
    );
}
tag commented 10 years ago

I can't recommend this approach.

  1. It's not clear what the $_subclasses array means, or how it's defined.
  2. __get and set__ behaviors not defined.
  3. save() functionality not clear.
  4. create() functionality not clear.
  5. delete() behavior not defined. (Deletes both? Deleting parent cascade to child? Deleting child cascades to parent?)
  6. Is your Model representing the unified view table (from your example)?

Answers to these questions would need to be agreed in design, IMNSHO, before adding something like this to core

Lastly, type is an hackish way of differentiating, as an entity might be of multiple types (Person <- Employee | Customer | Student). It seems either strategy or decorating patterns might be of better use on the model side. Or, as I suggested earlier, create different Model classes for each subclass and the parent. It's a bit more work to manage in the controllers, but doable for a small project.

I don't see a convincing case as to why this belongs in Paris. (Although, you've intrigued me, and I will try to hack together a different way of handling this.)

nbonamy commented 10 years ago

I understand your concern and I am going to give a bit more details...

The model classes behind this would be:

class Competition extends Model {
  public static $_subclasses = array(
    'CompetitionScore' => array('type' => 1),
    'CompetitionDiff' => array('type' => 2),
    'CompetitionSet' => array('type' => 3)
  );
}

class CompetitionScore extends Competition {
}

class CompetitionDiff extends Competition {
}

class CompetitionSet extends Competition {
}

The explanation for $_subclasses would be something like: an array of key/value pairs. Each pair being:

One thing that I have not check I agree is: how do we make sure that instances of say CompetitionDiff are stored in the competition table and not the competition_diff table. This needs to be worked out.

So now trying to answer some of your concerns:

  1. I was proposing this and of course this needs to be more documented. $_subclasses will not be no more or no less magical than the already existing $_table.
  2. I am most probably missing something here as I thought there was no getter/setter in Paris contrary than Doctrine 2.x for instance
  3. Competition subclasses inherit from Model (via Competition) so they get that for free don't they?
  4. Same than 3
  5. This is not the way Single Table Inheritance works. An entity is either a CompetitionScore, a CompetitionDiff or a CompetitionSet. All of those instances are stored in the same database table (defined by Competition). So there is no parent/child deleting stuff. The advantage is that you can have different business logic that you leverage through OOP polymorphism because the ORM layer automatically instantiates the right subclass based on data.
  6. Yes please refer to http://martinfowler.com/eaaCatalog/singleTableInheritance.html : "Single Table Inheritance maps all fields of all classes of an inheritance structure into a single table."

Lastly, type is just an example. It it is an attribute that allows you to differentiate between the different subclasses. That's the way all ORM I have worked with implement Single Table Inheritance (Hibernate, Doctrine...). You can name this column the way you want of course... The implementation I suggested allows to do this.

Though I perfectly understand that there is work left to be done I think this perfectly fits into Paris: this would be an easy and straightforward manner to increase the scope of Paris and support inheritance in the data model.

I am currently evaluating Paris to replace Doctrine 0.10 (yes that old) in my application. Inheritance is a must-have in my case and I think that this is a lightweight extension to Paris.

Regards, Nicolas

PS: you can also check http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/inheritance-mapping.html#single-table-inheritance

tag commented 10 years ago

I was treating SingleTable as a (read-only) view of the normalized tables, thinking data would need to be pulled from (and saved to) multiple tables for a single model object (a la "Class Table Inheritance"). I see now that you're actively de-normalizing tables.

It seems, then, what you're doing is attempting to leverage ORMWrapper->_create_model_instance() into a custom factory method (somewhat poorly said, as it's already something of a factory method). Thus, couldn't you work around the absence of this feature in Paris by building a factory in your model, and using it in your controller?

(Aside: It seems a more robust way of implementing such things in Paris might be the introduction of hooks on certain events, such as instantiate, create, hydrate, save, etc. I don't know what Simon's plans are in this regard, but it hasn't been mentioned.)

Something like (untested):

class Competition extends Model {
  public static function typedFactory(Competition $c) {
    switch($c->type) {
      case 1:
        $obj =  Model::factory('CompetitionScore');
        break;
      case 2:
        $obj =  Model::factory('CompetitionScore');
        break;
      // etc.
      default:
        return $c;  //or, return null, throw exception, etc.
    }

    // Take advantage of the fact ORM->hydrate doesn't flag as dirty, although Model->hydrate does.
    $obj->orm->hydrate( $c->asArray() );
    return $obj;
  }
}

class CompetitionScore extends Competition {} // Assumes `::_table` explicitly set in superclass
class CompetitionDiff extends Competition {}
class CompetitionSet extends Competition {}

//////////////////
// Examples:

// This would just work, but would not have type testing
$obj = Model::factory('CompetitionScore')->findOne($id); 

$obj2 = Competition::typedFactory( Model::factory('Competition')->findOne($id) ); 

Yeah, this has the downside of additional object creation, but this solution could be used immediately, without an update to Paris.

Pragmatically, is Paris were to implement "Single Table Inheritance", should it also then support "Class Table Inheritance" (sometimes called "Joined Table Inheritance") and "Concrete Table Inheritance"? Or, should it leave these details of each of these specific model/class mapping outside of the library?

EDIT: Fix code typo.

nbonamy commented 10 years ago

Sure I do not know what direction is meant to be given to Paris.

I am doing some benchmarks to compare the speed of different data access layers (namely mysqli vs pdo vs paris vs doctrine) and the paris overhead is so low compared to Doctrine that I am really looking into it right now. As previously said, the one thing I am missing is inheritance support. Considering the lightweight choices done on Paris I would find it acceptable to say: we support inheritance as we recognize that OOP polymorphism is useful (...) but we do it only the simplest way which is Single Table Inheritance. This choice is made to keep Paris 1) simple 2) efficient.

I know I can go with an outside Paris solution as you suggested or even derive ORMWrapper and inject into a derived Model class and have all this stuff processed "outside" Paris but I was just thinking this would be a nice addition to the library without sacrificing the lightweight nature of it which is was attracted me to it.

Of course, final implemention of this should be documented, unit tests added and all that stuff. If final decision is to go for it I am willing to fork, do the job as properly as I can and issue a pull request.

Regards

nbonamy commented 10 years ago

About making sure CompetitionDiff are saved in the same table than Competition. A rewrite of Model::table_name is needed. Pseudo code is:

Get parent class of current model class
If it is not Model
  Get its static vars
  Check for $_subclasses
  If found then
    Return table name of parent class
Return "standard" table name

Once again probably 10 lines of code in the end...

tag commented 10 years ago

Thanks for the discussion. After a better understanding of what you're looking for, I think yes, it could be accomplished with a small change to ORMWrapper::_create_model_instance(), as you suggest. (But, to your more recent comment, I would suggest simply requiring ::_table be set instead of rewriting Model::_get_table_name().)

Personally, however, it's not a feature I would ever use so long as I'm in control of the schema (preferring "Class Table Inheritance" or "Concrete Table Inheritance", as I believe SingleTable violates both "good" ER and class design, even while admitting that sometimes "good" is the enemy of "practical"). There are also, as you agree, many different ways to work around lack of direct support for the Single Table approach.

I'm not sure it's in keeping with the vision of Paris, but should @treffynnon wish to add it, I'm happy to help you with it if I can, and won't object further to its inclusion.

nbonamy commented 10 years ago

I am a bit biased here because while I have previously used STI with specific data for each subclass, in my current case subclasses do not have specific data and I am only looking at leveraging polymorphism.

About requiring ::_table instead of "hacking" Model::_get_table_name() I thought about that. This would of course be the easy way though it would be a little less elegant from a user perspective.

Finally my initial rewrite of ORMWrapper::_create_model_instance() is a bit overkill in the sense that you do not need multiple "selectors": in most (if not all) cases, you have a single discrimator column...

So now it's up to @treffynnon to decide I guess :-)

treffynnon commented 10 years ago

Firstly, I am sorry it has taken so long to get back to you on this. I have been really busy and working plenty of overtime lately.

@nbonamy and @tag I am not so sure that this is something that I would necessarily see as having a place in Paris. It feels much more like something that should either be written in the application domain or as an additional layer (separate project) that can be used on top of Paris.

I am also in a position where further features make it more difficult to maintain the project as we move forward.