Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Fix for issue ZF2-503 #2244

Closed
wants to merge 4 commits into from
Closed

Fix for issue ZF2-503 #2244

wants to merge 4 commits into from

Conversation

SGI495
Copy link
Contributor

@SGI495 SGI495 commented Aug 26, 2012

Fixed invalid quoting of LIMIT and OFFSET clauses in getSqlStringForSqlObject() as described in http://framework.zend.com/issues/browse/ZF2-503.

The solution works and I believe it does the same thing that was intended with quoteValue() (stopping injections), but it now returns a valid SQL statement. However I'm unsure if throwing an exception like that is expected behaviour so let me know if it's no good.

$sql = $platform->quoteValue($this->limit);
if (is_int($this->limit)) {
$sql = $this->limit;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

coding standards - put the else on the same line as the closing brackets and check indentations of all your lines please

@travisbot
Copy link

This pull request fails (merged 5cc7440 into 57c7d09).

@weierophinney
Copy link
Member

Check for "!is_int()" and throw an exception - that will simplify the logic and remove the need for an else block.

@travisbot
Copy link

This pull request fails (merged 8008a1e into 57c7d09).

@travisbot
Copy link

This pull request fails (merged 2f20f6f into 57c7d09).

@travisbot
Copy link

This pull request passes (merged 97aa908 into 57c7d09).

@ralphschindler
Copy link
Member

This not a good fix. You're probably experiencing this problem on MySQL, which is the only platform that does not accept quoted values for LIMIT and OFFSET. It is more correct for them to be quoted in terms of a valid abstraction.

A better fix for this is to create SQL abstraction for this situation, which can't be done in time for 2.0.

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

Successfully merging this pull request may close these issues.

None yet

5 participants