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

Parameter parsing fixes #309

Merged
merged 1 commit into from
May 9, 2013
Merged

Parameter parsing fixes #309

merged 1 commit into from
May 9, 2013

Conversation

lstrojny
Copy link
Contributor

Fix for #301 has been reverted to address http://www.doctrine-project.org/jira/browse/DBAL-496. This fix addresses both issues in a consistent manner and ads a few more tests.

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DBAL-501

We use Jira to track the state of pull requests and the versions they got
included in.


if ($isParam) {
throw SQLParserUtilsException::missingParam($paramName);
} else {
Copy link
Member

Choose a reason for hiding this comment

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

no need for else as the if throws an exception

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mutual exclusiveness is better communicated that way. That's why there is an else.

Copy link
Member

Choose a reason for hiding this comment

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

but it does not follow our coding standards

Copy link
Member

Choose a reason for hiding this comment

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

@lsmith77
Copy link
Member

using this branch I was able to successfully run the Jackalope Doctrine DBAL test suite with MySQL. Note for SQLite I wasnt able to run the test suite since it seems like drop FK statements are no longer ignored which makes the DB rest (https://github.com/jackalope/jackalope-doctrine-dbal/blob/master/src/Jackalope/Transport/DoctrineDBAL/RepositorySchema.php#L120) in our test suite fail:

PHP Fatal error:  Uncaught exception 'Doctrine\DBAL\DBALException' with message 'Sqlite platform does not support alter foreign key.' in /Users/lsmith/htdocs/cmf-sandbox/vendor/jackalope/jackalope-doctrine-dbal/vendor/doctrine/dbal/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php:646

but that is another story :-/

}

// Hash keys can be prefixed with a colon for compatibility
if (isset($paramsOrTypes[':' . $paramName])) {
Copy link
Member

Choose a reason for hiding this comment

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

wait, so PDO supports $stmt->bindParam(':foo', $val); instead of $stmt->bindParam('foo', $val);

Copy link
Member

Choose a reason for hiding this comment

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

yes .. it supports both

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that was the root cause of the compatibility break with the Jackalope DBAL component.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't know that, hence that code was never tested. :)

@lstrojny
Copy link
Contributor Author

@beberlei could you advise on where to go from here?

@beberlei
Copy link
Member

@lstrojny thinking about the SRP violation, a solution would be three methods,l but not sure if this is necessary here. Pondering with the problem... :)

@lstrojny
Copy link
Contributor Author

Sure, three methods would work extractType, extractParam, and doExtract. But this code is complex as hell as it is, no need to add more. I was even thinking about inlining the whole conditions to spare a few method calls (as n can get pretty large potentially).

@beberlei beberlei merged commit 38f0d51 into doctrine:master May 9, 2013
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 29, 2022
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.

7 participants