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

Fixes Issue #318 - SelectDecorator - subselect limit parameter not prefixed #329

Merged
merged 2 commits into from
Aug 7, 2018
Merged

Conversation

fuel-odrais
Copy link
Contributor

@fuel-odrais fuel-odrais commented Jul 23, 2018

Provide a narrative description of what you are trying to accomplish:

  • Are you fixing a bug?
    • Detail how the bug is invoked currently.
      Call prepareStatementForSqlObject on a Select with a sub-Select that has limit and/or offset set
    • Detail the original, incorrect behavior.
      A PDO Exception is thrown: PDOException: SQLSTATE[HY093]: Invalid parameter number: parameter was not defined
    • Detail the new, expected behavior.
      The statement is properly generated.
    • Base your feature on the master branch, and submit against that branch.
    • Add a regression test that demonstrates the bug, and proves the fix.
    • Add a CHANGELOG.md entry for the fix.

@tptrixtop
Copy link
Contributor

Looks like that was fixed by that #300

@fuel-odrais
Copy link
Contributor Author

@tptrixtop:

In PR #300, src/Sql/Platform/Mysql/SelectDecorator.php line 54 is this:

$parameterContainer->offsetSet($paramPrefix . 'limit', $this->limit, ParameterContainer::TYPE_INTEGER);

That code associates the Select's limit property with a SQL parameter named $paramPrefix . 'limit'.

Then we return the name of the SQL parameter that will actually be returned from the generated query. That string value is missing the $paramPrefix value.

I've included unit tests to demonstrate the bug and would be happy to make a commit with the tests only so you can verify.

@tptrixtop
Copy link
Contributor

@fuel-odrais

Ah, got it sir)

Copy link
Contributor

@tptrixtop tptrixtop left a comment

Choose a reason for hiding this comment

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

Fix makes sense.

@tptrixtop
Copy link
Contributor

ping @webimpress i think)

Copy link
Member

@michalbundyra michalbundyra left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@michalbundyra
Copy link
Member

@tptrixtop Sorry, I am not a maintainer... Ping @ezimuel :)

@ezimuel ezimuel merged commit 81168db into zendframework:master Aug 7, 2018
@ezimuel
Copy link
Contributor

ezimuel commented Aug 7, 2018

@fuel-odrais Thanks!

@MatyCZ
Copy link

MatyCZ commented Dec 3, 2018

When will be this fix released as new zend-db version?

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