[RFC] Renaming 'Om' to 'Base'

[fzaninotto]

The generated Model classes are currently stored in files under the 'Om' namespace (and subdirectory). 'om' used to mean 'Object Model' (although I'm not really sure), but the stub files are also part of the Object Model, so ot doesn't really make sense.

I suggest to rename the 'Om' folder to 'Base', and rename the generated classes from 'BaseBook' to 'Base\Book', which would make more sense.

[willdurand]

Agreed.

[jaugustin]

+1, but that's a big naming change ;)

[hhamon]

+1

[jaugustin]

I start the renaming in a om-rename branch.

The Object are generated in the good folder 'Base' without prefix.
Book (base one ;) ) is in Base\Book

Book :

<?php
use Base\Book as BaseBook
class Book extends BaseBook {}

There is still some issue with Base class that have use on child classe (conflic) I don't now if I should use self/static inenstead of using use Book as ChildBook

[willdurand]

static is fine if it works. Otherwise ChildObject will be enough.

William

Le 25 nov. 2011 à 00:57, Jérémie Augustinreply@reply.github.com a écrit :

I start the renaming in a om-rename branch.

The Object are generated in the good folder 'Base' without prefix.
Book (base one ;) ) is in Base\Book

Book :

<?php
use Base\Book as BaseBook
class Book extends BaseBook {}

There is still some issue with Base class that have use on child classe (conflic) I don't now if I should use self/static inenstead of using use Book as ChildBook

---

Reply to this email directly or view it on GitHub:
#35 (comment)

[fzaninotto]

I think you should update the declareClassNamespace() method to let it
accept aliases, instead of concatenating the classname and the alias into a
string.

2011/11/25 William DURAND <
reply@reply.github.com

static is fine if it works. Otherwise ChildObject will be enough.

William

Le 25 nov. 2011 00:57, Jrmie Augustinreply@reply.github.com a crit
:

I start the renaming in a om-rename branch.

The Object are generated in the good folder 'Base' without prefix.
Book (base one ;) ) is in Base\Book

Book :

<?php
use Base\Book as BaseBook
class Book extends BaseBook {}

There is still some issue with Base class that have use on child classe
(conflic) I don't now if I should use self/static inenstead of using use Book as ChildBook

---

Reply to this email directly or view it on GitHub:
#35 (comment)

---

Reply to this email directly or view it on GitHub:
#35 (comment)

[jaugustin]

Yes that's what I was thinking at 1am ;)

I will refactor the method to handle alias and to trow exception when there is conflic (adding a classname that already exist or is the current class)

[willdurand]

Will we remove some properties?

Envoyé de mon iPhone

Le 25 nov. 2011 à 11:40, Jérémie Augustinreply@reply.github.com a écrit :

Yes that what I was thinking at 1am ;)

I will refactor the method to handle alias and to trow exception when there is conflic (adding a classname that already exist or is the current class)

---

Reply to this email directly or view it on GitHub:
#35 (comment)

[jaugustin]

that's what I asked in the code the other way around ;)
Some part is hard coded do we want more or less flexibility.

[willdurand]

Who will rename these stuffs ?
Is there a real usecase since we have namespaces ?

Le 25 nov. 2011 à 11:52, Jérémie Augustinreply@reply.github.com a écrit :

that's what I asked in the code the other way around ;)
Some part is hard coded do we want more or less flexibility.

---

Reply to this email directly or view it on GitHub:
#35 (comment)

[willdurand]

We should keep these things hardcoded and remove useless properties.

Le 25 nov. 2011 à 11:52, Jérémie Augustinreply@reply.github.com a écrit :

that's what I asked in the code the other way around ;)
Some part is hard coded do we want more or less flexibility.

---

Reply to this email directly or view it on GitHub:
#35 (comment)

[jaugustin]

I refactor declareClassNamespace to handle Alias.

But now I have a big Probleme with non prefixed base class, we will have to give alias to all child classes.

if we have a Model\Base\Book like :

<?php
namespace Model\Base;

use Model\Book as ChildBook;
use Model\Author;

class Book extends BaseObject  implements Persistent
{
}

This won't work and we have to do this :

<?php
namespace Model\Base;

use Model\Book as ChildBook;
use Model\Author as ChildAuthor;

class Book extends BaseObject  implements Persistent
{
}

What do you think about this, should we continue on this way ?

[willdurand]

Why ?

Envoyé de mon iPhone

Le 26 nov. 2011 à 01:32, Jérémie Augustinreply@reply.github.com a écrit :

I refactor declareClassNamespace to handle Alias.

But now I have a big Probleme with non prefixed base class, we will have to give alias to all child classes.

if we have a Model\Base\Book like :

<?php
namespace Model\Base;

use Model\Book as ChildBook;
use Model\Author;

class Book extends BaseObject  implements Persistent
{
}

This won't work and we have to do this :

<?php
namespace Model\Base;

use Model\Book as ChildBook;
use Model\Author as ChildAuthor;

class Book extends BaseObject  implements Persistent
{
}

What do you think about this, should we continue on this way ?

---

Reply to this email directly or view it on GitHub:
#35 (comment)

[jaugustin]

take a look at the fatal I got when running the test:

Fatal error: Cannot use Propel\Tests\Bookstore\Behavior\AggregateComment as AggregateComment because the name is already in use in D:\Dev\propel\Propel2\tests\Fixtures\bookstore\build\classes\Propel\Tests\Bookstore\Behavior\Base\AggregateCommentQuery.php on line 13

and the class look like this :

<?php

namespace Propel\Tests\Bookstore\Behavior\Base;

use \PDO;
use Propel\Runtime\Propel;
use Propel\Runtime\Collection\Collection;
use Propel\Runtime\Connection\ConnectionInterface;
use Propel\Runtime\Exception\PropelException;
use Propel\Runtime\Query\Criteria;
use Propel\Runtime\Query\ModelCriteria;
use Propel\Runtime\Query\ModelJoin;
use Propel\Tests\Bookstore\Behavior\AggregateComment;
use Propel\Tests\Bookstore\Behavior\AggregateCommentPeer;
use Propel\Tests\Bookstore\Behavior\AggregatePost;
use Propel\Tests\Bookstore\Behavior\AggregatePostQuery;
use Propel\Tests\Bookstore\Behavior\AggregateCommentQuery as ChildAggregateCommentQuery;

/**
 * Base class that represents a query for the 'aggregate_comment' table.
 *
 *
 * @package    propel.generator.Propel.Tests.Bookstore.Behavior.Base
 */
abstract class AggregateCommentQuery extends ModelCriteria
{

My point of view is that we Cannot use Propel\Tests\Bookstore\Behavior\AggregateComment as AggregateComment because there is already a class in the Propel\Tests\Bookstore\Behavior\Base namespace that is named AggregateComment (the base object)
But I am not a php 5.3 namespace Expert ;)

[jaugustin]

I found this, that prouve the opposite, but in our case there is a conflict.

http://www.php.net/manual/fr/language.namespaces.faq.php#language.namespaces.faq.conflict [french]
http://www.php.net/manual/en/language.namespaces.faq.php#language.namespaces.faq.conflict [english]

I think it could be because of Symfony 2 autoload.

I test to remove the Propel\Tests\Bookstore\Behavior\Base\AggrecateColumn, then it passed the first test and faill the next one for the same reason a conflict with a base class

[jaugustin]

I made a new commit, But I still get lot's of issues :(

first the qucikbuilder, and schema without namespace.
Do we continue to support Model without namespace ? like the one in tests files ? loaded with the quick builder.

I force the renaming of child class but this mess up generated phpdoc and some other things :(

[willdurand]

I guess we should be able to generate non-namespaced classes as PHP 5.3 doesn't force anyone to use namespaces.

[jaugustin]

I am waiting answer in #82 to fix some issues with Classname / FQCN

[jaugustin]

I push new commits.

I still get 10 failures on Runtime tests (caused by Classname / FQCN keys issues #82)

[jaugustin]

@willdurand has discuss, I will drop support for non-namespaced classes because child classes will be in conflict with base classes

[fzaninotto]

@jaugustin Can you explain us again why we have these conflicts? Seems like a good use Base\Book as BaseBook should suffice, I'm missing something here.

[willdurand]

No problem

Envoyé de mon iPhone

Le 12 déc. 2011 à 13:42, Jérémie Augustinreply@reply.github.com a écrit :

@willdurand has discuss, I will drop support for non-namespaced classes because child classes will be in conflict with base classes

---

Reply to this email directly or view it on GitHub:
#35 (comment)

[jaugustin]

@fzaninotto that work with namespaced model, my issue is with model without namespace:

Class Book extends Book

where the second Book is the base class

and use won't work

<?php
use Book as BaseBook //this will throw a fatal exception

class Book extends BaseBook
{
}

the other way to support non-namespaced model is to set Base prefix on base class, but I don't think it's a good idea.

[fzaninotto]

Why don't you use Base\Book? it's not because the model has no namespace that its base version should not have one...

[jaugustin]

ok, I will try that solution.

<?php
use Base\Book as BaseBook

class Book extends BaseBook
{
}

[jaugustin]

Work in progress :

I fixed runtime tests (all green)
with correct key for toArray fromArray in ObjectCollection

Still lots of work's to do on builder,

I will implements methods mentioned in #82, and use them in the right place in all builder file

• getUnqualifiedClassName() => Book
• getQualifiedClassName() => Model\Base\Book
• getFullyQualifiedClassName() => \Model\Base\Book

I have done some work to automaticaly declare "use statement" but it's not realy good, to much hack needed to fix simple case.

[jaugustin]

TODO:

• use new getClassname method's for old get(Peer|Query|Object)Classname (maybe need 3 methods by type)
• use new getClassname method's for getClassnameFromBuilder($builder) (maybe need 3 methods)
• fix behavior
• fix Base, Child class conflict (I started using static )
• fix auto declaration of use statments
• add tests for new method's (getClassname, declarationClasses)
• fix hundred's tests ;)

[fzaninotto]

Wow, that's a lot left to do. I suggest you rebase now, or you'll have more work afterwards.

Le 22 déc. 2011 à 23:39, Jérémie Augustinreply@reply.github.com a écrit :

TODO:

• use new getClassname method's for old get(Peer|Query|Object)Classname (maybe need 3 methods by type)
• use new getClassname method's for getClassnameFromBuilder($builder) (maybe need 3 methods)
• fix behavior
• fix Base, Child class conflict (I started using static )
• fix auto declaration of use statments
• add tests for new method's (getClassname, declarationClasses)
• fix hundred's tests ;)

---

Reply to this email directly or view it on GitHub:
#35 (comment)

[willdurand]

Rebased :)

[jaugustin]

WIP only 266 errors left :(

https://github.com/jaugustin/Propel2/tree/om-rename-rework

http://travis-ci.org/#!/jaugustin/Propel2/jobs/892254

[jaugustin]

Tests that Fails:

• Runtime : Tests: 1004, Assertions: 2194, Failures: 6, Errors: 15, Skipped: 22
• Generator\Behavior : Fatal big issue with Quickbuilder and class/namespace conflict
• Generator\Builder : Tests: 371, Assertions: 964, Failures: 9
• Generator* : Green
• Other : Green

[willdurand]

Nice, keep going!

[willdurand]

@jaugustin I can see few failures due to the model name. What's the plan? Should we use the FQCN or just QCN here?

Some tests expect the QCN (Foo\Bar) but the generated code sets the FQCN (\Foo\Bar).

[jaugustin]

Some news :

Tests that Fails:

• Generator\Behavior : Fatal big issue with Quickbuilder and class/namespace conflict

Tests Ok:

• Runtime : Tests: 1008, Assertions: 2258, Skipped: 22
• Generator* : Green
• Other : Green

But some work need to be done on the use of FQCN / QCN / CN in the Query Object because it's not realy consistent

[willdurand]

@jaugustin can you ask few questions to decide what we have to do about FQCN/QCN/CN ?

This refactoring is really important, and almost finished (the behaviors part is not a big deal), so we have to be sure we provide clean changes.

[jaugustin]

Tests that still failling :

• Behavior :
  • Archivable
  • Delegate
  • I18n
  • Versionnable

The end of this issue has never been so close ;)

[jaugustin]

only one behavior failing :

• Versionnable

[fzaninotto]

Putting the champagne in the fridge

[hhamon]

Huge piece of work! Congratulations!

[jaugustin]

issue closed