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

Fix Mapper::query() method to pass param types. #208

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

andrejs
Copy link
Contributor

@andrejs andrejs commented Dec 8, 2016

When using custom query, it's not possible to use custom types for query parameters. For example:

$mapper->query(
    'SELECT * FROM user WHERE state IN (?)',
    [
        ['pending', 'active']
    ],
    [
        \Doctrine\DBAL\Connection::PARAM_STR_ARRAY
    ]
);

This pull request fixes the problem by passing custom param types to \Doctrine\DBAL\Connection::executeQuery() via Mapper::query().

@FlipEverything
Copy link
Contributor

Off: The custom query above is just an example, right? Because you don't need to use a custom query for that. You can execute it like this: $mapper->where(['state IN' => ['pending', 'active']]);

On: I can't say anything about the usefullness of passing through the custom param type.

@andrejs
Copy link
Contributor Author

andrejs commented Dec 12, 2016

Yes, it's just an example for the sake of simplicity. Intended usage of \Spot\Mapper::query() is for custom, complex queries, where you might also have typed parameters. Currently using query() you can't specify them, however it's supported directly by DBAL (see \Doctrine\DBAL\Connection::executeQuery). The method just doesn't proxy them. IMHO this is unnecessary limitation of Spot.

@FlipEverything
Copy link
Contributor

I agree!

@tuupola
Copy link
Contributor

tuupola commented Apr 15, 2017

Needs tests, otherwise looks good to me. Valitron bug was already fixed with #217.

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

Successfully merging this pull request may close these issues.

4 participants