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

Fix for issue ZF2-503#2244

Closed
SGI495 wants to merge 4 commits into
zendframework:masterfrom
SGI495:master
Closed

Fix for issue ZF2-503#2244
SGI495 wants to merge 4 commits into
zendframework:masterfrom
SGI495:master

Conversation

@SGI495

@SGI495 SGI495 commented Aug 26, 2012

Copy link
Copy Markdown
Contributor

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.

Comment thread library/Zend/Db/Sql/Select.php Outdated

Copy link
Copy Markdown

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
Copy Markdown

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

@weierophinney

Copy link
Copy Markdown
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
Copy Markdown

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

@travisbot

Copy link
Copy Markdown

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

@travisbot

Copy link
Copy Markdown

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

@ralphschindler

Copy link
Copy Markdown
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.

5 participants