Inconsistent column name recognition with php namespaces #627

Open
Ventzy opened this Issue Mar 10, 2013 · 4 comments

Comments

Projects
None yet
3 participants
@Ventzy

Ventzy commented Mar 10, 2013

I have query like this

$a = ActionQuery::create()
  ->innerJoinActionStatus()
  ->withColumn('Action.Id', 'id')
  ->withColumn('Action.StatusId', 'status_id')
  ->withColumn('ActionStatus.Code', 'code')
  ->select(array('id', 'status_id', 'code'))
  ->find();

This works when Propel generated classes does not use PHP namespaces. With namespaces however it does not work, because "Action.Id" and "Action.StatusId" are not recognized and converted to their column names - "action.id" and "action.status_id". The reason is in ModelCriteria->getColumnFromName method. The condition
if ($prefix == $this->getModelAliasOrName() || $prefix == $this->getTableMap()->getName())
is not met with namespaces, because $this->getModelAliasOrName() returns full namespace of the class and instead "Action" without namespaces, it returns Namespace\Namespace\Action.

If namespaces are added for the main model in the query it works

$a = ActionQuery::create()
  ->innerJoinActionStatus()
  ->withColumn('Namespace\Namespace\Action.Id', 'id')
  ->withColumn('Namespace\Namespace\Action.StatusId', 'status_id')
  ->withColumn('ActionStatus.Code', 'code')
  ->select(array('id', 'status_id', 'code'))
  ->find();

But there are few problems:

  1. Its verbose, its unnecessary (I believe) and it breaks old code if classes are distributed to namespaces.
  2. Bigger problem is that it is not consistent with joins. Note that joined table ActionStatus does not have namespace added. If its added - query does not work anymore, like this:
$a = ActionQuery::create()
  ->innerJoinActionStatus()
  ->withColumn('Namespace\Namespace\Action.Id', 'id')
  ->withColumn('Namespace\Namespace\Action.StatusId', 'status_id')
  ->withColumn('Namespace\Namespace\ActionStatus.Code', 'code')
  ->select(array('id', 'status_id', 'code'))
  ->find();

It does not work with namespace, because of second condition in ModelCriteria->getColumnFromName method: elseif (isset($this->joins[$prefix])). In $this->joins keys are class names like "ActionStatus" and not namespaced class names like "Namespace\Namespace\ActionStatus".

There is simple solution for the first variant - without added namespaces in the query. In the first condition
if ($prefix == $this->getModelAliasOrName() || $prefix == $this->getTableMap()->getName())
can be added " || $prefix == $this->getTableMap()->getPhpName()" to become
if ($prefix == $this->getModelAliasOrName() || $prefix == $this->getTableMap()->getName() || $prefix == $this->getTableMap()->getPhpName())
This way it will detect "Action" because $this->getTableMap()->getPhpName() will return class name without namespaces.

@jakerella

This comment has been minimized.

Show comment Hide comment
@jakerella

jakerella Mar 10, 2013

I think the main problem is that ->withColumn('Action.Id', 'id') is ambiguous if PHP 5.3 namespacing is used. What if there was a \Namespace\SubOne\Action class and a \Namespace\SubTwo\Action class? How would Propel know which one to reference? I suppose if it were to only look at the joined tables for withColumn calls then that would work, but I don't know that you can/want to put that logic in ModelCriteria->getColumnFromName().

I think the main problem is that ->withColumn('Action.Id', 'id') is ambiguous if PHP 5.3 namespacing is used. What if there was a \Namespace\SubOne\Action class and a \Namespace\SubTwo\Action class? How would Propel know which one to reference? I suppose if it were to only look at the joined tables for withColumn calls then that would work, but I don't know that you can/want to put that logic in ModelCriteria->getColumnFromName().

@jaugustin

This comment has been minimized.

Show comment Hide comment
@jaugustin

jaugustin Mar 10, 2013

Member

@jakerella that's an issue we talked with @willdurand but for now we didn't found the good solution there is also PR open on Propel2 for this

propelorm/Propel2#172

Member

jaugustin commented Mar 10, 2013

@jakerella that's an issue we talked with @willdurand but for now we didn't found the good solution there is also PR open on Propel2 for this

propelorm/Propel2#172

@jakerella

This comment has been minimized.

Show comment Hide comment
@jakerella

jakerella Mar 10, 2013

Well, looking at that PR (and this issue) I can see how simplicity would be nice, but we also should not break things for devs that DO have two of the same class in different namespaces. Perhaps Propel should be smart enough to say: "oh, you're asking for the Book class/table... I know that is actually \Libray\Book, so here you go." But if you ask for "User" and there are two different classes: \Library\User and \Computer\User Propel may need to throw an exception (a new AmbiguousClassException perhaps).

Well, looking at that PR (and this issue) I can see how simplicity would be nice, but we also should not break things for devs that DO have two of the same class in different namespaces. Perhaps Propel should be smart enough to say: "oh, you're asking for the Book class/table... I know that is actually \Libray\Book, so here you go." But if you ask for "User" and there are two different classes: \Library\User and \Computer\User Propel may need to throw an exception (a new AmbiguousClassException perhaps).

@jaugustin

This comment has been minimized.

Show comment Hide comment
@jaugustin

jaugustin Mar 11, 2013

Member

@jakerella that exactly what I had in mind, :)

Member

jaugustin commented Mar 11, 2013

@jakerella that exactly what I had in mind, :)

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