[WIP] Refactor SchemaReader

[gossi]

PR for #36

I just started working on refactoring the SchemaReader. My idea behind was to make it as independent as possible, means the only reference to propel is the passed generator config. One SchemaReader is also possible to read in as many schemas as wanted, so it's no further bound to the one schema it is reading in.

I've tested it with the bookstore/schema.xml file and it was able to read it in, no further tests so far. Also no features from the referenced issue are yet included.

Models

However, I had to look into the model and .. hell yeah, they drive me nuts! They are somewhat unreliable in their API it's hard to get it right and not fuck up right from the start. A lot of that still stems from the old days and the path to now is still visible, yet I think it's about time to refactor them along with the rest of the code. Here is some of my review:

1) Model.addSomeModel(), in this case (Database.addDomain())

bad:

    public function addDomain($data)
    {
        if ($data instanceof Domain) {
            $domain = $data; // alias
            $domain->setDatabase($this);
            $this->domainMap[$domain->getName()] = $domain;

            return $domain;
        }

        $domain = new Domain();
        $domain->setDatabase($this);
        $domain->loadMapping($data);

        return $this->addDomain($domain); // call self w/ different param
    }

good:

    public function addDomain(Domain $domain)
    {
        $domain->setDatabase($this);
        $this->domainMap[$domain->getName()] = $domain;

        return $this; // for fluent interface
    }

2) The order of calling is important:

Bad (it fails)

$field = new Field();
$field->loadMapping($attributes);
$field->setEntity($entity);
$entity->addField($field);

// `setEntity()` must be called before `loadMapping()` - this one here breaks

Good:

$field = new Field();
$field->loadMapping($attributes);

// either
$field->setEntity($entity);
// -- OR --
$entity->addField($field);
// both should have the same effect

3) Instantiations:

Many models do set some internal values of which public properties are exposed, so they need some internal setup for this, which basically is done with loadMapping(). However, different models have different requirements to this. A simple model->loadMapping($attributes) won't help here (see example above).

Suggested solution: Use constructor to set initial values and pass in their requirements, set loadMapping() to protected and call that from constructor.

Second, is the mapping of attributes (e.g. from schema.xml) to internal values. I thought instead of pulling that off to some kind of factory, where the attributes from schema.xml are transformed to the respective model values (introducing a layer between to keep models as POJO). I further though I had a good throw at this with my introduced ModelFactory... yet that was before I realized all that order strictness. A drawback: internal, non public visible values can't be set easily, unless exposed through a public API, which sometimes isn't the best thing.

Either way, there is room to improve.

[gossi]

I would alter the models accordingly with that PR, yet this needs some feedback and input from you guys.

[marcj]

Good job, I like it 👍

[gossi]

cool! My plan would be as follows:

1. Rework models and php7ize them
2. Use updated models on new SchemaReader
3. Include features from referenced issue into SchemaReader and provide test for it
4. Use new SchemaReader in situations where propel reads in schemas at the moment (in various ways as I found out)

Regardings models: I guess reworking models will break a lot of existing code, need to be adapted as well then. However, this will improve code quality of the project as whole. I still need to look through the code, what's the best way to instantiate them and load them with initial mapping. Could be a ModelFactory, could be constructors, still need to find them out. I don't know much about propel internals in area, where models are used outside SchemaReader. Your experience and feedback will be heavily appreciated for this as what should be the best solution here.

PS: Once the code is ready to merge, I will let run code formatter to apply required code styling, until then my IDE will apply to what is best for me.

[gossi]

I'll keep commenting on this thread, while I work through the code and I see things I don't quite understand or need feedback for.

finalization of models This is a weird parameter in the getDatabases() method of e.g. Schema and is passed down. There are two solutions I could think of, both of which will remove the parameter in the forementioned method (that's just wrong there):

1. Use listeners. Put listeners on behaviors, databases, entities and whenever they fire, do update accordingly. This requires high logic and is very computing-intense, but would always be a consistent model.
2. Run it manually. Not everything requires to apply finalization on the model (especially if you only work with the model as a data source). It basically affects the generator part. Essentially, the generator should be aware of that fact and run finalization on its own.

I'd go with solution 2 here unless you have something to throw in.

[gossi]

The more I work through the models, the more I understand them. I put some rules together which have helped me so far to refactor the code:

1) Models are POJOs and composition works with the help of traits 2) Specialization over generalization. This introduces a superordinate chain, e.g. Schema < Database < Entity < Field. You can set a value at Schema and override it at any lower level. The opposite of this is, that requesting a value from a Field may go up that chain until a value is found. 3) Grouping code chunks. This is for models with many lines (e.g. Entity). I grouped code blocks (bunch of similar methods) together to make them easier findable within the code which greatly increased refactorability (and later maintainability I hope).

[cristianoc72]

@gossi any news here? Could I help you in some way?

[gossi]

Everything is fine, though had to pause during summer. Will and can continue soon.

[cristianoc72]

Sorry mate, summer vacations are sacred! I just can't wait to see this beauty finished! :smile:

[gossi]

Guys, I thought I find time to finish this job in the last quartal of last years. Turns out, I don't have capacities for this at the moment and probably for the next year. Can anybody take over this task?

[cristianoc72]

I can try. Could you write me, please, a sort of ToDo list? To better understand what is missing to do. Thank you!

[gossi]

This is a recap from my memory, though might have leaks already, but:

• Make the model namespace independent of other parts of propel (e.g. $db = new Database('path/to/schema.xml'); should be enough code). A PlatformInterface is optional and will be created on demand internally if necessary.
• Implement the superordinate chain into all models, e.g. Entity < Database < Schema
  • e.g. Entity.getNamespace() will ask on entity first, then bubble up to database or schema.
• Realize models with composition, see my *Parts traits for this
• Getters will return what setters did set (this was problematic in propel2)
• Clear naming conventions on *Name properties and methods, see Propelorm/Propel2#1121
• Implement the good parts I explained in my first parts and drop the bad parts
• Drop loadMapping() completely, use getters and setters, provide appropriate defaults or put them on top of the superordinate chain
• Externalize finalize() methods from the model into somewhere else => $whatever->finalize($schema).
• Rewrite my SchemaReader to make use of the new APIs. Maybe also use something different like symfony config component to support multiple schema formats #69

I think these are the main parts I can recap now. There are more and I already have solutions for problems in my head I didn't mention here or are in the code, so feel free to ask.

[cristianoc72]

I'm on it since a couple of days and what I can say is that it's a awesome refactor! The new code is clean, no duplications, no "strange" inheritance, less code to do the same things! Many thanks and compliments to @gossi for the excellent work. I hope to be able to continue on this road 👍

[marcj]

oh cool! Cant wait to see it :)

[cristianoc72]

Move forward in #83