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

Added identiferEquals method for expressions to use fields with quoti… #8242

Merged
merged 6 commits into from
Feb 15, 2016

Conversation

skie
Copy link
Member

@skie skie commented Feb 11, 2016

…ng in join conditions. Added identifier parameter for expressions. Request created against master to replace #8238

…ng in join conditions. Added identifier parameter for expressions.
*/
public function equalFields($left, $right)
{
return $this->eq(new IdentifierExpression($left), new IdentifierExpression($right));
Copy link
Member

Choose a reason for hiding this comment

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

I think we we should test for $left and $right not being ExpressionInterface before wrapping them in an identifier expression

@markstory markstory added this to the 3.2.2 milestone Feb 12, 2016
{
$wrapIdentifier = function ($field) {
if ($field instanceof ExpressionInterface) {
return $field;
Copy link
Member

Choose a reason for hiding this comment

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

Do any of the tests get in here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, Query tests cover this method.

@markstory markstory modified the milestones: 3.2.2, 3.2.3 Feb 12, 2016
@markstory
Copy link
Member

This looks good to me. Any thoughts @lorenzo ?

@lorenzo
Copy link
Member

lorenzo commented Feb 15, 2016

Woops, I thought it was merged already

lorenzo added a commit that referenced this pull request Feb 15, 2016
Added identiferEquals method for expressions to use fields with quoti…
@lorenzo lorenzo merged commit 08e43f7 into cakephp:master Feb 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants