Skip to content

Om rename : the big change #175

Merged
merged 38 commits into from Apr 15, 2012

5 participants

@jaugustin
Propel member

Hi,

Work on om-rename #35 is finished, now it's time to merge it back.

a little PR with 60 changed files with 817 additions and 598 deletions ;)

and all tests are GREEN

Build Status

@jaugustin
Propel member

grrrr it need a rebase

jaugustin and others added some commits Nov 25, 2011
@jaugustin jaugustin [Generator] start renaming Base object from Om dir to base dir withou…
…t Base prefix
5ad5781
@jaugustin jaugustin [Generator] refactor declareClass to handle Alias, manual or automati…
…caly
14452d8
@jaugustin jaugustin [Generator] continue to rename Base classes b7c4c46
@jaugustin jaugustin [Generator] fix use of FQCN, add parameter to getClassname 2ae0712
@jaugustin jaugustin [Runtime][Collection] fix setModel to handle FQCN correctly, fix call…
… to getModel
32111e7
@jaugustin jaugustin [Runtime][ObjectCollection] fix FQCN feys for from/to Array e2ea88f
@jaugustin jaugustin [Generator][QuickBuilder] fix Base prefix for non-namespaced class, f…
…ix auto namespace in quickbuilder
7c751f4
@jaugustin jaugustin [Generator] add 4 new getClassname methods, use it in the right place 44a7593
@jaugustin jaugustin [Generator] fix willdurand rebase ac88602
@jaugustin jaugustin [Runtime] fix collection tests 42e7177
@jaugustin jaugustin [Runtime] fix some tests (work in progress) ad82988
@jaugustin jaugustin [WIP]fedex day: om rename to Base a7c874c
@jaugustin jaugustin [Generator][Builder] fix Object builder tests and use lcfirst 254e8df
@jaugustin jaugustin [Generator][Util] fix QuickBuilder tests 0bd0aab
@willdurand willdurand [Generator] Fixed some unit tests, added a LogicException d1101c4
@jaugustin jaugustin [Generator][TableMapBuild] change FQCN to CN for extends TableMap ed1130f
@jaugustin jaugustin [Runtime][ModelCriteria] fix test, handle FQCN or QCN has Model input
[Runtime][ModelWith] fix test, handle FQCN or QCN has Model input
1cbda7a
@jaugustin jaugustin [Runtime][Map] fix getTableMap when class doesn't have namespace
[Runtime][Query] fix join to use FQCN or QCN => force QCN
c67fea3
@jaugustin jaugustin [Generator][Builder] fix tests of declareClass whith duclicate exception 1897efa
@jaugustin jaugustin [Generator][Behavior] fix AggregateColumn, wrong classname with names…
…apce, remove lcfirst hack for php 5.2
67a0d79
@jaugustin jaugustin [Generator][Behavior] fix build ConcreteInheritanceBehavior, setParen…
…tClass set QCN instead of FQCN
2a508a6
@jaugustin jaugustin [Generator][Behavior] fix getPeerClassNameWithNamespace for NestedSet c1e3869
@willdurand willdurand Updated gitignore
Removed composer.lock
36ed53f
@willdurand willdurand [Generator] Fixed CS 49398cc
@jaugustin jaugustin [Generator][I18n] fix generate wrong query filterBy
[Generator][Builder] fix generate class without namespace, add whitelist
class (PDO, Exception, DateTime
remove unecessary use statement DateTimeZone
6f2f1e7
@jaugustin jaugustin [Generator][QuickBuilder] fix rebase of #155 56485ab
@jaugustin jaugustin [Generator][Builder] fix issue with non namespaced Model, fix namespa…
…ce of QueryInheritance force Base when needed, the end of #35
1f5cf64
@jaugustin
Propel member

green and mergeable,

@jaugustin jaugustin commented on an outdated diff Apr 14, 2012
...sionable/VersionableBehaviorObjectBuilderModifier.php
@@ -487,7 +487,10 @@ public function getAllVersions(\$con = null)
protected function addComputeDiff(&$script)
{
$versionTable = $this->behavior->getVersionTable();
+ $versionARClassname = $this->builder->getClassnameFromBuilder($this->builder->getNewStubObjectBuilder($versionTable));
@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

seems unused

@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jaugustin jaugustin commented on an outdated diff Apr 14, 2012
...sionable/VersionableBehaviorObjectBuilderModifier.php
@@ -487,7 +487,10 @@ public function getAllVersions(\$con = null)
protected function addComputeDiff(&$script)
{
$versionTable = $this->behavior->getVersionTable();
+ $versionARClassname = $this->builder->getClassnameFromBuilder($this->builder->getNewStubObjectBuilder($versionTable));
+ $versionForeignColumn = $versionTable->getColumn($this->behavior->getParameter('version_column'));
@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

seems unused

@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jaugustin jaugustin and 1 other commented on an outdated diff Apr 14, 2012
src/Propel/Generator/Builder/Om/AbstractOMBuilder.php
@@ -33,6 +35,14 @@
protected $declaredClasses = array();
/**
+ * Mapping bettwen fully qualified classnames and their short classname or alias
+ * @var array
+ */
+ protected $declaredShortClassesOrAlias = array();
+
+ protected $whiteListOfDeclaratedClasses = array('PDO', 'Exception', 'DateTime');
@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

need a little comment

@hhamon
Propel member
hhamon added a note Apr 14, 2012

$whiteListOfDeclaredClasses instead of $whiteListOfDeclaratedClasses

@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fzaninotto
Propel member

🙌
🙏
💪
👏

nuff said

@jaugustin jaugustin commented on an outdated diff Apr 14, 2012
src/Propel/Generator/Builder/Om/AbstractOMBuilder.php
+ } elseif (false === array_search($class, $this->whiteListOfDeclaratedClasses, true)) { //force alias for model without namespace
+
+ return true;
+ }
+ }
+ if ('Base' == $namespace && '' == $this->getNamespace()) {
+ if (false === array_search($class, $this->whiteListOfDeclaratedClasses, true)) { //force alias for model without namespace
+
+ return true;
+ }
+ }
+
+ return false;
+ }
+
+ public function declareClassNamespacePrefix($class, $namespace = '', $aliasPrefix = false)
@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

missing phpDoc

@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@willdurand willdurand and 1 other commented on an outdated diff Apr 14, 2012
src/Propel/Generator/Command/ModelBuild.php
@@ -85,7 +85,7 @@ protected function configure()
->addOption('disable-namespace-auto-package', null, InputOption::VALUE_NONE,
'Disable namespace auto-packaging')
->addOption('base-prefix', null, InputOption::VALUE_REQUIRED,
- 'Prefix for base classes', 'Base')
+ 'Prefix for base classes', '') //should be removed
@willdurand
Propel member
willdurand added a note Apr 14, 2012

and so what?

@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

done ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hhamon hhamon and 1 other commented on an outdated diff Apr 14, 2012
src/Propel/Generator/Util/QuickBuilder.php
@@ -278,4 +278,17 @@ public function fixNamespaceDeclarations($source)
return $output;
}
+
+ /**
+ * prevent generated class without namespace to fail
+ * @param string $code
+ * @return string
+ */
+ protected function forceNamespace($code) {
@hhamon
Propel member
hhamon added a note Apr 14, 2012

parenthesis must be on the following line

@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hhamon hhamon and 1 other commented on an outdated diff Apr 14, 2012
src/Propel/Runtime/Collection/Collection.php
@@ -47,6 +47,12 @@ class Collection extends ArrayObject implements Serializable
protected $model = '';
/**
+ * the fully qualified classname of the model
@hhamon
Propel member
hhamon added a note Apr 14, 2012

the => The ^^

@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hhamon hhamon commented on the diff Apr 14, 2012
src/Propel/Runtime/Map/DatabaseMap.php
$this->addTableFromMapClass($tmClass);
-
- return $this->tablesByPhpName[$phpName];
- } else {
- throw new TableNotFoundException("Cannot fetch TableMap for undefined table phpName: $phpName");
+ if (array_key_exists($phpName, $this->tablesByPhpName)) {
+ return $this->tablesByPhpName[$phpName];
+ }
+ $phpName = '\\'.$phpName;
+ if (array_key_exists($phpName, $this->tablesByPhpName)) {
@hhamon
Propel member
hhamon added a note Apr 14, 2012

if (isset($this->tablesByPhpName[$phpName])) { would be a bit faster than array_key_exists

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hhamon hhamon commented on the diff Apr 14, 2012
src/Propel/Runtime/Query/ModelCriteria.php
@@ -99,7 +99,12 @@ public function __construct($dbName = null, $modelName = null, $modelAlias = nul
{
$this->setDbName($dbName);
$this->originalDbName = $dbName;
- $this->modelName = $modelName;
+ if (0 === strpos($modelName, '\\')) {
+ $this->modelName = substr($modelName, 1);
@hhamon
Propel member
hhamon added a note Apr 14, 2012

Maybe you may use a ternary operator here

@willdurand
Propel member
willdurand added a note Apr 14, 2012

no need here. readability over all :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hhamon hhamon commented on the diff Apr 14, 2012
src/Propel/Runtime/Query/ModelWith.php
@@ -71,7 +71,11 @@ public function init(ModelJoin $join)
public function setModelName($modelName)
{
- $this->modelName = $modelName;
+ if (0 === strpos($modelName, '\\')) {
+ $this->modelName = substr($modelName, 1);
@hhamon
Propel member
hhamon added a note Apr 14, 2012

ternary operator instead here?

@willdurand
Propel member
willdurand added a note Apr 14, 2012

no need here. readability over all :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jaugustin jaugustin commented on the diff Apr 14, 2012
src/Propel/Generator/Builder/Om/AbstractPeerBuilder.php
@@ -279,7 +279,7 @@ static public function getColumnName(Column $col, $phpName = null)
}
if (null !== $phpName) {
- return sprintf('%sPeer::$s', $phpName, $const);
+ return sprintf('static::$s', $const);
@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

I change this but I don't think the old code was good : $const could never be used
@fzaninotto @willdurand
maybe this code is dead ?

@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

this is definitely dead code, used for compatibility but the function is never called

@willdurand
Propel member
willdurand added a note Apr 14, 2012

remove it.

@jaugustin
Propel member
jaugustin added a note Apr 15, 2012

maybe it's only the phpName option never used ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@willdurand
Propel member

@hhamon if you can run your quality checkers, I'll be glad to merge your fixes :)

@jaugustin jaugustin and 1 other commented on an outdated diff Apr 14, 2012
...pel/Generator/Builder/Om/MultiExtendObjectBuilder.php
@@ -33,7 +33,7 @@ class MultiExtendObjectBuilder extends AbstractObjectBuilder
*/
public function getUnprefixedClassname()
{
- return $this->getChild()->getClassname();
+ return $this->getChild()->getClassName();
@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

maybe getClassName of Propel\Generator\Model\Inheritance could be renamed to getClassname ?

Do we want to normalize Classname vs ClassName (maybe another issue for that)

@willdurand
Propel member
willdurand added a note Apr 14, 2012

ClassName everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jaugustin jaugustin commented on an outdated diff Apr 14, 2012
src/Propel/Generator/Builder/Om/ObjectBuilder.php
@@ -3448,9 +3452,11 @@ protected function addRefFKAdd(&$script, ForeignKey $refFK)
$joinedTableObjectBuilder = $this->getNewObjectBuilder($refFK->getTable());
$className = $joinedTableObjectBuilder->getObjectClassname();
+ /* FIXME : use the right methode to get the base Classes
@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

Oups I don't remember this

@jaugustin
Propel member
jaugustin added a note Apr 15, 2012

fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jaugustin jaugustin commented on an outdated diff Apr 14, 2012
src/Propel/Generator/Builder/Om/ObjectBuilder.php
$tblFK = $refFK->getTable();
$foreignObjectName = '$' . $tblFK->getStudlyPhpName();
$script .= "
/**
+ * {$this->getRefFKPhpNameAffix($crossFK, false)} != {$this->getFKPhpNameAffix($crossFK, false)}
@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

oups! this seems to be a debug comment 😉

@jaugustin
Propel member
jaugustin added a note Apr 15, 2012

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jaugustin jaugustin and 1 other commented on an outdated diff Apr 14, 2012
src/Propel/Generator/Builder/Om/QueryBuilder.php
@@ -38,8 +38,10 @@ public function getNamespace()
if ($this->getGeneratorConfig() && $omns = $this->getGeneratorConfig()->getBuildProperty('namespaceOm')) {
@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

Maybe this could be remove because we force Base as a subdirectory the configuration is not necessary and I am not sure that it could work with a different parametter

@willdurand
Propel member
willdurand added a note Apr 14, 2012

good to remove

@jaugustin
Propel member
jaugustin added a note Apr 15, 2012

removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jaugustin jaugustin commented on an outdated diff Apr 14, 2012
src/Propel/Generator/Builder/Om/QueryBuilder.php
if ($namespace = $fkQueryBuilder->getNamespace()) {
- $queryClass = '\\' . $namespace . '\\' . $queryClass;
+ $queryClass = '\\' . $queryClass;
@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

I think the "\\" is not needed because $queryClass is already a FQCN

@jaugustin
Propel member
jaugustin added a note Apr 15, 2012

removed useless code (the if block) :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jaugustin jaugustin and 1 other commented on an outdated diff Apr 14, 2012
src/Propel/Generator/Builder/Om/QueryBuilder.php
if ($namespace = $fkQueryBuilder->getNamespace()) {
- $queryClass = '\\' . $namespace . '\\' . $queryClass;
+ $queryClass = '\\' . $queryClass;
@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

the same here :
I think the "\" is not needed because $queryClass is already a FQCN

@willdurand
Propel member
willdurand added a note Apr 14, 2012

you comment about your own stuffs, you're magic :p

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jaugustin jaugustin commented on an outdated diff Apr 14, 2012
src/Propel/Generator/Util/QuickBuilder.php
@@ -201,7 +198,9 @@ public function getClassesForTable(Table $table, array $classTargets = null)
if ($table->getInterface()) {
$interface = $this->getConfig()->getConfiguredBuilder('interface', $target)->build();
- $interface = "\nnamespace\n{\n" . $interface . "\n}\n";
+ if (false === strpos($class, 'namespace')) {
@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

is it necessary or is is it double with the fixNamespaceDeclarations() ?

@jaugustin
Propel member
jaugustin added a note Apr 15, 2012

removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jaugustin jaugustin and 1 other commented on an outdated diff Apr 14, 2012
tools/generator/default.properties
@@ -87,7 +87,7 @@ propel.addSaveMethod = true
propel.addTimeStamp = false
propel.addValidateMethod = true
propel.addHooks = true
-propel.basePrefix = Base
+propel.basePrefix =
@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

totally remove this ?

@willdurand
Propel member
willdurand added a note Apr 14, 2012

agreed

@willdurand
Propel member
willdurand added a note Apr 14, 2012

btw, I'm not sure this default.properties file is style needed.. I'll clean the project later. But yes, remove this parameter.

@jaugustin
Propel member
jaugustin added a note Apr 15, 2012

removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Apr 14, 2012
...r/ConcreteInheritance/ConcreteInheritanceBehavior.php
@@ -136,22 +136,13 @@ public function parentClass($builder)
$parentTable = $this->getParentTable();
switch (get_class($builder)) {
case 'Propel\Generator\Builder\Om\ObjectBuilder':
- $objectBuilder = $builder->getNewStubObjectBuilder($parentTable);
- $builder->declareClass($objectBuilder->getFullyQualifiedClassname());
-
- return $objectBuilder->getClassname();
+ return $builder->declareClassFromBuilder($builder->getNewStubObjectBuilder($parentTable), true);
break;
@stof
stof added a note Apr 14, 2012

break is useless as there is a return statement above

@jaugustin
Propel member
jaugustin added a note Apr 15, 2012

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Apr 14, 2012
src/Propel/Generator/Builder/Om/AbstractOMBuilder.php
* @var array
*/
protected $declaredClasses = array();
/**
+ * Mapping bettwen fully qualified classnames and their short classname or alias
@stof
stof added a note Apr 14, 2012

typo here

@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Apr 14, 2012
src/Propel/Generator/Builder/Om/AbstractOMBuilder.php
* @return string
*/
public function getFullyQualifiedClassname()
{
+ return '\\' . $this->getQualifiedClassname();
if ($namespace = $this->getNamespace()) {
@stof
stof added a note Apr 14, 2012

you can remove all this dead code as the method returns above

@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 2 others commented on an outdated diff Apr 14, 2012
src/Propel/Generator/Builder/Om/AbstractOMBuilder.php
}
/**
- * Returns the namespaced classname if there is a namespace, and the raw classname otherwise
+ * Returns the qualified classname (e.g. Model\Book)
+ *
+ * @return string
+ */
+ public function getQualifiedClassname()
+ {
+ if ($namespace = $this->getNamespace()) {
+ return $namespace . '\\' . $this->getUnqualifiedClassname();
+ } else {
@stof
stof added a note Apr 14, 2012

does Propel follow the same rule than Symfony2 regarding not using else when not needed ? I don't remember what the agreement was when we discussed the coding standards months ago

@stof
stof added a note Apr 14, 2012

Btw, I saw many other places where such a comment could have been applied

@jaugustin
Propel member
jaugustin added a note Apr 14, 2012

done for this file, but it should be done for all the lib

@willdurand
Propel member
willdurand added a note Apr 14, 2012

yep, I know..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jaugustin
Propel member

if everyone is ok with the code :) I think this could be merge

@willdurand willdurand merged commit 8a65452 into master Apr 15, 2012
@willdurand
Propel member

Merged!

@hhamon hhamon pushed a commit to hhamon/Propel2 that referenced this pull request May 15, 2012
@fzaninotto fzaninotto Fix archivable behavior when used with unique index.
If a table has unique columns, the resulting archive table has set
UNIQUE indices on these columns.

While this seems like a good idea at the first glance, it's a bug
because you can't delete two records which share the same value in this
colum.

This patch fixes the issue by copying unique indices to indices on the
archive table.

Closes #175.
9080b45
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.