Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Composite pk break test #1114

Closed
wants to merge 5 commits into from
Closed

Conversation

goetas
Copy link
Member

@goetas goetas commented Aug 19, 2014

No description provided.

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3260

We use Jira to track the state of pull requests and the versions they got
included in.

if (isset($this->class->fieldMappings[$field]['requireSQLConversion'])) {
$placeholder = Type::getType($this->class->getTypeOfField($field))->convertToDatabaseValueSQL($placeholder, $this->platform);
if (count($columns)>1 && $comparison === Comparison::IN){
throw ORMException::cantUseInOperatorOnCompositeKeys();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In reality, you can... =)

SELECT ... FROM ... WHERE (id1, id2) IN (('cpk1.1', 'cpk1.2'), ('cpk2.1', 'cpk2.2'), ...);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this SQL standard? Sqlite seems that does not support this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhm... is that cross-db compatible?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is... in SQLite, you can follow these rules:

select-stmt: ... WHERE expr
expr: "(" expr ")" | in-expr
in-expr: expr [ "NOT" ] "IN" ( [ database-name "." ] table-name | "(" ( select-stmt | { expr "," }* ) ")" )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see where the nested parenthesis are in there. See for example http://stackoverflow.com/a/11171387/347063

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it's either a bug in their grammar or their support. Based on their SQL grammar, it is supported.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guilhermeblanco given the amount of effort that we put in fixing SQLServer's shit (pardon the terminology: comes from frustration) I don't think this is a valid argument anymore :P
We support what is supported by all storage layers, not what the standard supports.

@goetas
Copy link
Member Author

goetas commented Aug 19, 2014

@guilhermeblanco I'm sorry, but I have created this PR only to clarify http://www.doctrine-project.org/jira/browse/DDC-3259

@goetas goetas closed this Aug 19, 2014
$className = (isset($this->class->associationMappings[$field]['inherited']))
? $this->class->associationMappings[$field]['inherited']
: $this->class->name;

return $this->getSQLTableAlias($className) . '.' . $this->quoteStrategy->getJoinColumnName($joinColumn, $this->class, $this->platform);
$columns = array();
foreach ($this->class->associationMappings[$field]['joinColumns'] as $joinColumn) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing line break

@goetas
Copy link
Member Author

goetas commented Aug 19, 2014

This is the real PR #1113

@@ -1741,79 +1757,86 @@ public function expandParameters($criteria)
continue; // skip null values.
}

$types[] = $this->getType($field, $value);
$params[] = $this->getValue($value);
$types = array_merge($types, $this->getTypes($field, $value));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align = signs

$class = $this->em->getClassMetadata(ClassUtils::getClass($value));
if ($class->isIdentifierComposite) {
$newValue = array();
foreach ($class->getIdentifierValues($value) as $innerValue) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing line break

@guilhermeblanco
Copy link
Member

@goetas AAHH... sorry then! =)

*
* @throws \Doctrine\ORM\ORMException
*/
protected function getSelectConditionStatementColumnSQL($field, $assoc = null)
private function getSelectConditionStatementColumnSQL($field, $assoc = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a minor bc break

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants