Ticket #1360 (closed defect: fixed)

Opened 11 months ago

Last modified 10 months ago

[PATCH] Doctrine_Import_Schema fails on importing a mix of concrete and simple/column-aggregation inheritance

Reported by: bschussek Owned by: jwage
Priority: major Milestone:
Component: Schema Files Version: 1.0.0
Severity: Keywords:
Cc: Has Test: no
Status: Pending Core Response Has Patch: yes

Description (last modified by bschussek) (diff)

Imagine the following setup:

abstract class BaseRecord extends Doctrine_Record
abstract class Category extends BaseRecord
class ConcreteCategory extends Category

schema.yml:

Category:
  inheritance:
    extends: BaseRecord
  columns:
    ...

ConcreteCategory:
  inheritance:
    extends: Category
    type: column_aggregation
    keyField: ...
    keyValue: ...

BaseRecord? contains some methods that are shared across all my models. It does not contain any columns and thus is not specified in schema.yml. Category is a base class for all my category types. It is abstract as well, several concrete categories inherit it.

The problem is that Doctrine tries to merge all columns from the children (ConcreteCategory?) to the most upper parent class (here BaseRecord?), since the type is specified as column_aggregation. It does not check though, whether the inheritance type in the children's parents (Category extends BaseRecord?) is of type column_aggregation as well. Merging everything to BaseRecord? fails though, as BaseRecord? is nowhere defined in the schema.yml.

There is a very easy fix for this problem:

Doctrine_Import_Schema, line 475:

            $parent = $this->_findBaseSuperClass($array, $definition['className']);
            // Move any definitions on the schema to the parent
            if (isset($definition['inheritance']['extends']) && isset($definition['inheritance']['type']) && ($definition['inheritance']['type'] == 'simple' || $definition['inheritance']['type'] == 'column_aggregation')) {
                foreach ($moves as $move => $resetValue) {
                    $array[$parent][$move] = Doctrine_Lib::arrayDeepMerge($array[$parent][$move], $definition[$move]);

In case the inheritance of the current class (ConcreteCategory?) is column_aggregation, the base parent (BaseRecord?) is found out and initialized ($array[$parent]...). _findBaseSuperClass should not loop to the most upper parent though, but only as long as the condition in line 477 is met. Thus we should change the code in line 500 from

    protected function _findBaseSuperClass($array, $class)
    {
        if (isset($array[$class]['inheritance']['extends'])) {
            return $this->_findBaseSuperClass($array, $array[$class]['inheritance']['extends']);

to

    protected function _findBaseSuperClass($array, $class)
    {
        if (isset($array[$class]['inheritance']['extends']) && isset($array[$class]['inheritance']['type']) && ($array[$class]['inheritance']['type'] == 'simple' || $array[$class]['inheritance']['type'] == 'column_aggregation')) {
            return $this->_findBaseSuperClass($array, $array[$class]['inheritance']['extends']);

You can also refactor the initialization of the variable $parent inside the if-statement (line 475 again):

            // Move any definitions on the schema to the parent
            if (isset($definition['inheritance']['extends']) && isset($definition['inheritance']['type']) && ($definition['inheritance']['type'] == 'simple' || $definition['inheritance']['type'] == 'column_aggregation')) {
                $parent = $this->_findBaseSuperClass($array, $definition['className']);
                foreach ($moves as $move => $resetValue) {
                    $array[$parent][$move] = Doctrine_Lib::arrayDeepMerge($array[$parent][$move], $definition[$move]);

Change History

  Changed 11 months ago by bschussek

Oh my. I just noticed a stupid copy-paste error in my patch. The fix for _findBaseSuperClass() should of course be

    protected function _findBaseSuperClass($array, $class)
    {
        if (isset($array[$class]['inheritance']['extends']) && isset($array[$class]['inheritance']['type']) && ($array[$class]['inheritance']['type'] == 'simple' || $array[$class]['inheritance']['type'] == 'column_aggregation')) {
            return $this->_findBaseSuperClass($array, $array[$class]['inheritance']['extends']);

Note the $array[$class] instead of $definition.

follow-up: ↓ 3   Changed 11 months ago by eXtreme

Man, do you know you can edit your task's details? :P It's just under Comment textbox ;)

in reply to: ↑ 2   Changed 11 months ago by bschussek

  • has_patch set
  • description modified (diff)

Replying to eXtreme:

Man, do you know you can edit your task's details? :P It's just under Comment textbox ;)

Duh, actually yes. I just didn't think of it yesterday! :-O

The task details are fixed now.

  Changed 10 months ago by jwage

  • status changed from new to closed
  • resolution set to fixed

(In [4832]) fixes #1360

  Changed 10 months ago by jwage

  • version changed from 0.11 to 1.0
  • milestone changed from Unknown to 1.0.0-RC2

  Changed 10 months ago by anonymous

  • milestone New deleted

Milestone New deleted

Note: See TracTickets for help on using tickets.