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

Support true VARCHAR binding to prevent implicit conversions? #4263

Closed
gjdanis opened this issue Sep 9, 2020 · 4 comments · Fixed by #4274
Closed

Support true VARCHAR binding to prevent implicit conversions? #4263

gjdanis opened this issue Sep 9, 2020 · 4 comments · Fixed by #4274

Comments

@gjdanis
Copy link
Contributor

gjdanis commented Sep 9, 2020

Feature Request

Q A
New Feature yes
RFC no
BC Break no

Summary

PHP 7.2 added support for specifying different bind types for unicode and non-unicode string data: https://wiki.php.net/rfc/extended-string-types-for-pdo

This is a major improvement over the default (PDO::PARAM_STR) which binds strings as unicode data. In the past, the inability for PHP to handle this has led to performance problems in certain cases. For example if we have something like:

$statement = $connection->prepare(
   'SELECT * FROM ... WHERE myVarcharColumn = :value' -- will bind as N'...'
);

$statement->bindValue(':value', $myVarcharValue);
$statement->execute();

If there's an index on myVarcharColumn SQL won't be able to use the index effectively since the binding will be an NVARCHAR string and force an implicit conversation.

Does the team have a plan to support this binding in the future? We actually implemented this internally in a wrapper we have DBAL. I'm happy to open a PR if the team supports this feature:

use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Types\StringType;
use PDO;

class VarcharType extends StringType
{
    public const NAME = 'varchar';

    public function getSQLDeclaration(array $field, AbstractPlatform $platform)
    {
        $length = $field['length'] ?? $platform->getVarcharDefaultLength();
        return 'VARCHAR(' . $length . ')';
    }

    public function getBindingType()
    {
        return PDO::PARAM_STR | PDO::PARAM_STR_CHAR;
    }

    public function getName()
    {
        return static::NAME;
    }
}
@gjdanis gjdanis changed the title Support true VARCHAR binding? Support true VARCHAR binding to prevent implicit conversions? Sep 9, 2020
@morozov
Copy link
Member

morozov commented Sep 11, 2020

Does the team have a plan to support this binding in the future?

I believe it would be a valuable addition. Note that the direct usage of PDO with DBAL is deprecated (see ParameterType). The solution will have to also work with sqlsrv, support PARAM_STR_ARRAY (which will probably warrant some refactoring) and will require some integration testing to prove that with the properly bound parameters no conversion is happening.

This was referenced Sep 11, 2020
@gjdanis
Copy link
Contributor Author

gjdanis commented Sep 11, 2020

Started working on this here @morozov: https://github.com/doctrine/dbal/pull/4270/files#diff-0abfe812e47f01473ac9b172da2d2c3cR87

Please let me know if this is close to what you had in mind.

@morozov
Copy link
Member

morozov commented Sep 12, 2020

Please let me know if this is close to what you had in mind.

I didn't know it could be tested like this but this way looks good.

@morozov morozov linked a pull request Sep 12, 2020 that will close this issue
gjdanis added a commit to gjdanis/dbal that referenced this issue Sep 16, 2020
gjdanis added a commit to gjdanis/dbal that referenced this issue Sep 17, 2020
greg0ire pushed a commit to gjdanis/dbal that referenced this issue Sep 18, 2020
@morozov morozov added this to the 2.11.0 milestone Sep 18, 2020
@morozov morozov linked a pull request Sep 18, 2020 that will close this issue
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants