Skip to content

[HttpFoundation] changed MERGE queries #18501

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 19, 2016
Merged

[HttpFoundation] changed MERGE queries #18501

merged 1 commit into from
Jun 19, 2016

Conversation

hjkl
Copy link
Contributor

@hjkl hjkl commented Apr 10, 2016

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #17284
License MIT
Doc PR N/A

Changed the MERGE queries for Oracle and SQL Server to use question mark parameter markers so they work with emulation disabled or enabled - fixes #17284

@xabbuh
Copy link
Member

xabbuh commented Apr 11, 2016

Imo a proper fix would rather change the queries to not use a named parameter more than once.

@slootjes
Copy link

I can confirm this fix is working for me.

@fabpot
Copy link
Member

fabpot commented Apr 28, 2016

@Tobion Can you have a look at this one?

@fabpot
Copy link
Member

fabpot commented Jun 16, 2016

@hjkl Have you tested @xabbuh's suggestion instead?

@Tobion
Copy link
Contributor

Tobion commented Jun 16, 2016 •
edited
Loading

@hjkl please change getMergeSql (probably rename it as well to getMergeStatement) to return the PDO statement directly with the params already bound. Since the params and the sql are now dependent on each other, it should be done in one place.
Apart from that it looks correct to me.

@Tobion
Copy link
Contributor

Tobion commented Jun 16, 2016

This PR needs a rebase.

@hjkl
Copy link
Contributor Author

hjkl commented Jun 18, 2016

@fabpot I haven't attempted @xabbuh's suggestion - I don't have the TSQL expertise to refactor the merge query to only use each parameter once.

@Tobion Done, thanks for the advice.

@Tobion
Copy link
Contributor

Tobion commented Jun 18, 2016 •
edited
Loading

Imo a proper fix would rather change the queries to not use a named parameter more than once.

This is exactly what this PR does. NAMED params cannot be used more than once but it's not possible to change the MERGE queries to only use each param once as they need to be repeated. It only works for the MySQL for example because mysql supports the VALUES($this->dataCol) syntax to reuse values. Otherwise we would also be forced into using numeric values several times for mysql. For reference, if you disable emulated prepares in MySQL and use the same named param more than once you get a SQLSTATE[HY093]: Invalid parameter number error as I just tried.

👍 good to merge
Status: Reviewed

@fabpot
Copy link
Member

fabpot commented Jun 19, 2016

Thank you @hjkl.

@fabpot fabpot merged commit ebf3a2f into symfony:2.7 Jun 19, 2016
fabpot added a commit that referenced this pull request Jun 19, 2016
@fabpot
bug #18501 [HttpFoundation] changed MERGE queries (hjkl)
6a8a113 
This PR was merged into the 2.7 branch.

Discussion
----------

[HttpFoundation] changed MERGE queries

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #17284
| License | MIT
| Doc PR | N/A

Changed the MERGE queries for Oracle and SQL Server to use question mark parameter markers so they work with emulation disabled or enabled - fixes #17284

Commits
-------

ebf3a2f Fixed oci and sqlsrv merge queries when emulation is disabled - fixes #17284
phansys
phansys reviewed Jun 19, 2016
View reviewed changes
src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php
$mergeStmt->bindParam(6, $data, \PDO::PARAM_LOB);
$mergeStmt->bindParam(7, $maxlifetime, \PDO::PARAM_INT);
$mergeStmt->bindValue(8, time(), \PDO::PARAM_INT);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@hjkl, could you please submit another PR fixing this minor CS issue? Thank you in advance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@phansys sure, I can't see the coding standards issue though, could you please clarify for me?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure @hjkl, I refer to PSR-2 5.1:

} else {

Thank you.

@phansys phansys mentioned this pull request Jun 20, 2016
Closed
hjkl added a commit to hjkl/symfony that referenced this pull request Jun 20, 2016
@hjkl
fix coding standards for symfony#18501
b327352
This was referenced Jun 30, 2016
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug HttpFoundation Status: Reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@hjkl @xabbuh @slootjes @fabpot @Tobion @phansys @javiereguiluz @carsonbot