doctrine/archived-orm
1
0
Fork 0
mirror of https://github.com/doctrine/orm.git synced 2026-03-23 22:42:18 +01:00
Code Issues 5.3k Packages Projects Releases 100 Wiki Activity

GetSingleScalarResult/GetScalarResult should always return an int when the field is typed as integer #6470

New Issue
Open
opened 2026-01-22 15:33:47 +01:00 by admin · 7 comments
admin commented 2026-01-22 15:33:47 +01:00
Owner
Copy Link

Originally created by @VincentLanglet on GitHub (May 22, 2020).

Feature Request

Q A
New Feature yes (or Bugfix depending on the point of view)
BC Break no

Summary

The start of the discussion can be found here: https://github.com/doctrine/orm/pull/8149

Let's take an integer field

    /**
     * @var int|null
     *
     * @ORM\Column(type="integer")
     * @ORM\Id
     * @ORM\GeneratedValue(strategy="AUTO")
     */
    private $id;

And a query

$query = $this->createQueryBuilder('contractOperation')
            ->select('contractOperation.id')
            ->setMaxResults(1)
            ->getQuery();

With a postgres database,

$query->getSingleScalarResult()

Is returning an int, 1.

But with mariadb it returns a string, 1.
I think it should be better to always return an int.

Moreover,

$query->getResult()

is returning an array of int with MariaDB.

So I see no reason to returns a string with getSingleScalarResult and an (array of) int with getResult.

Originally created by @VincentLanglet on GitHub (May 22, 2020). ### Feature Request <!-- Fill in the relevant information below to help triage your issue. --> | Q | A |------------ | ------ | New Feature | yes (or Bugfix depending on the point of view) | BC Break | no #### Summary The start of the discussion can be found here: https://github.com/doctrine/orm/pull/8149 Let's take an integer field ``` /** * @var int|null * * @ORM\Column(type="integer") * @ORM\Id * @ORM\GeneratedValue(strategy="AUTO") */ private $id; ``` And a query ``` $query = $this->createQueryBuilder('contractOperation') ->select('contractOperation.id') ->setMaxResults(1) ->getQuery(); ``` With a postgres database, ``` $query->getSingleScalarResult() ``` Is returning an `int`, 1. But with mariadb it returns a `string`, `1`. I think it should be better to always return an `int`. Moreover, ``` $query->getResult() ``` is returning an array of `int` with MariaDB. So I see no reason to returns a `string` with `getSingleScalarResult` and an (array of) `int` with `getResult`.
admin commented 2026-01-22 15:33:47 +01:00
Author
Owner
Copy Link

@VincentLanglet commented on GitHub (May 22, 2020):

This is not restricted to getSingleScalarResult.
getScalarResult has the same issue and returns an array of string with MariaDB for the given query.

This also occurs for query like ->select('COUNT(contractOperation.id)'), returning int with pdosql and string with MariaDB. But It's maybe harder to improve the behaviour since it's not related to a field...

@VincentLanglet commented on GitHub (May 22, 2020): This is not restricted to `getSingleScalarResult`. `getScalarResult` has the same issue and returns an array of `string` with MariaDB for the given query. This also occurs for query like `->select('COUNT(contractOperation.id)')`, returning `int` with pdosql and `string` with MariaDB. But It's maybe harder to improve the behaviour since it's not related to a field...
admin commented 2026-01-22 15:33:48 +01:00
Author
Owner
Copy Link

@VincentLanglet commented on GitHub (May 22, 2020):

The issue seems should be fixable in https://github.com/doctrine/orm/blob/master/lib/Doctrine/ORM/Internal/Hydration/AbstractHydrator.php#L318

$cacheKeyInfo are

array:3 [▼
  "isScalar" => true
  "fieldName" => "id"
  "type" => Doctrine\DBAL\Types\IntegerType {#558}
]

So

$type  = $cacheKeyInfo['type'];
$value = $type ? $type->convertToPHPValue($value, $this->_platform) : $value;

Fix the issue

There is already something similar

// WARNING: BC break! We know this is the desired behavior to type convert values, but this
// erroneous behavior exists since 2.0 and we're forced to keep compatibility.
if (! isset($cacheKeyInfo['isScalar'])) {
    $dqlAlias  = $cacheKeyInfo['dqlAlias'];
    $type      = $cacheKeyInfo['type'];
    $fieldName = $dqlAlias . '_' . $fieldName;
    $value     = $type
        ? $type->convertToPHPValue($value, $this->platform)
        : $value;
}

But in my case there is a isScalar value and no dqlAlias one.
Maybe the fieldName modification should stay in the isScalar check but the type update shouldn't ?

For the other issue (when using 'COUNT(contractOperation.id)') the type is currently Doctrine\StringType.

This is because of this function: https://github.com/doctrine/orm/blob/master/lib/Doctrine/ORM/Query/SqlWalker.php#L1317
According to the comment, the type cannot be resolved so string is set by default.
https://github.com/doctrine/orm/blob/master/lib/Doctrine/ORM/Query/SqlWalker.php#L1378

But maybe the type should be resolved sometimes...

@VincentLanglet commented on GitHub (May 22, 2020): The issue seems should be fixable in https://github.com/doctrine/orm/blob/master/lib/Doctrine/ORM/Internal/Hydration/AbstractHydrator.php#L318 $cacheKeyInfo are ``` array:3 [▼ "isScalar" => true "fieldName" => "id" "type" => Doctrine\DBAL\Types\IntegerType {#558} ] ``` So ``` $type = $cacheKeyInfo['type']; $value = $type ? $type->convertToPHPValue($value, $this->_platform) : $value; ``` Fix the issue There is already something similar ``` // WARNING: BC break! We know this is the desired behavior to type convert values, but this // erroneous behavior exists since 2.0 and we're forced to keep compatibility. if (! isset($cacheKeyInfo['isScalar'])) { $dqlAlias = $cacheKeyInfo['dqlAlias']; $type = $cacheKeyInfo['type']; $fieldName = $dqlAlias . '_' . $fieldName; $value = $type ? $type->convertToPHPValue($value, $this->platform) : $value; } ``` But in my case there is a `isScalar` value and no `dqlAlias` one. Maybe the `fieldName` modification should stay in the `isScalar` check but the type update shouldn't ? For the other issue (when using 'COUNT(contractOperation.id)') the type is currently `Doctrine\StringType`. This is because of this function: https://github.com/doctrine/orm/blob/master/lib/Doctrine/ORM/Query/SqlWalker.php#L1317 According to the comment, the type cannot be resolved so `string` is set by default. https://github.com/doctrine/orm/blob/master/lib/Doctrine/ORM/Query/SqlWalker.php#L1378 But maybe the type should be resolved sometimes...
admin commented 2026-01-22 15:33:48 +01:00
Author
Owner
Copy Link

@greg0ire commented on GitHub (May 23, 2020):

Any thoughts about this comment: 72bc09926d/lib/Doctrine/ORM/Internal/Hydration/AbstractHydrator.php (L330-L331) ? It was first introduced in a4dac7a292 and then reworked in 48172f3a53 .

@greg0ire commented on GitHub (May 23, 2020): Any thoughts about this comment: https://github.com/doctrine/orm/blob/72bc09926df1ff71697f4cc2e478cf52f0aa30d8/lib/Doctrine/ORM/Internal/Hydration/AbstractHydrator.php#L330-L331 ? It was first introduced in a4dac7a292cffcd48e9ac2c7680d506cd5e8ead8 and then reworked in 48172f3a53bd2bd003fbe019ba0d72cfd5d669f8 .
admin commented 2026-01-22 15:33:48 +01:00
Author
Owner
Copy Link

@VincentLanglet commented on GitHub (May 23, 2020):

Thanks for the help @greg0ire

// WARNING: BC break! We know this is the desired behavior to type convert values, but this
// erroneous behavior exists since 2.0 and we're forced to keep compatibility. For 3.0 release,
// uncomment these 2 lines of code.
//$type  = $cache[$key]['type'];
//$value = $type->convertToPHPValue($value, $this->_platform);

Is pretty clear and go on my side. We do wont to convert the value with the type. But it was a BC-break so should be made in 3.0.

The rework

// WARNING: BC break! We know this is the desired behavior to type convert values, but this
// erroneous behavior exists since 2.0 and we're forced to keep compatibility.
if (! isset($cacheKeyInfo['isScalar'])) {
    $dqlAlias  = $cacheKeyInfo['dqlAlias'];
    $type      = $cacheKeyInfo['type'];
    $fieldName = $dqlAlias . '_' . $fieldName;
    $value     = $type
        ? $type->convertToPHPValue($value, $this->platform)
        : $value;
}

Wasn't the best rework, it should have been

// WARNING: BC break! We know this is the desired behavior to type convert values, but this
// erroneous behavior exists since 2.0 and we're forced to keep compatibility.
if (! isset($cacheKeyInfo['isScalar'])) {
    $type      = $cacheKeyInfo['type'];
    $value     = $type
        ? $type->convertToPHPValue($value, $this->platform)
        : $value;
}

if (isset($cacheKeyInfo['dqlAlias'])) {
    $dqlAlias  = $cacheKeyInfo['dqlAlias'];
    $fieldName = $dqlAlias . '_' . $fieldName;
}

Because the manipulation are not made for the same reason.

We could make a PR on master then.

@VincentLanglet commented on GitHub (May 23, 2020): Thanks for the help @greg0ire ``` // WARNING: BC break! We know this is the desired behavior to type convert values, but this // erroneous behavior exists since 2.0 and we're forced to keep compatibility. For 3.0 release, // uncomment these 2 lines of code. //$type = $cache[$key]['type']; //$value = $type->convertToPHPValue($value, $this->_platform); ``` Is pretty clear and go on my side. We do wont to convert the value with the type. But it was a BC-break so should be made in 3.0. The rework ``` // WARNING: BC break! We know this is the desired behavior to type convert values, but this // erroneous behavior exists since 2.0 and we're forced to keep compatibility. if (! isset($cacheKeyInfo['isScalar'])) { $dqlAlias = $cacheKeyInfo['dqlAlias']; $type = $cacheKeyInfo['type']; $fieldName = $dqlAlias . '_' . $fieldName; $value = $type ? $type->convertToPHPValue($value, $this->platform) : $value; } ``` Wasn't the best rework, it should have been ``` // WARNING: BC break! We know this is the desired behavior to type convert values, but this // erroneous behavior exists since 2.0 and we're forced to keep compatibility. if (! isset($cacheKeyInfo['isScalar'])) { $type = $cacheKeyInfo['type']; $value = $type ? $type->convertToPHPValue($value, $this->platform) : $value; } if (isset($cacheKeyInfo['dqlAlias'])) { $dqlAlias = $cacheKeyInfo['dqlAlias']; $fieldName = $dqlAlias . '_' . $fieldName; } ``` Because the manipulation are not made for the same reason. We could make a PR on master then.
admin commented 2026-01-22 15:33:48 +01:00
Author
Owner
Copy Link

@beberlei commented on GitHub (May 27, 2020):

@greg0ire the problem is not integer columns, but lets say you have a "datetime" column though getSingleScalarResult. changing to use type conversion would suddenly created a DateTime instead of a string, potentially breaking lots of code.

Its indeed something that can be changed on 3.0 already, but it should be done in a BC / upgradable way.

Idea: Introduce a new query hint that is checked in hydrator at the point, and call it when using a new method getSingleTypedScalarResult. Is that technically feasible?

@beberlei commented on GitHub (May 27, 2020): @greg0ire the problem is not integer columns, but lets say you have a "datetime" column though getSingleScalarResult. changing to use type conversion would suddenly created a DateTime instead of a string, potentially breaking lots of code. Its indeed something that can be changed on 3.0 already, but it should be done in a BC / upgradable way. Idea: Introduce a new query hint that is checked in hydrator at the point, and call it when using a new method `getSingleTypedScalarResult`. Is that technically feasible?
admin commented 2026-01-22 15:33:48 +01:00
Author
Owner
Copy Link

@greg0ire commented on GitHub (May 27, 2020):

Idea: Introduce a new query hint that is checked in hydrator at the point, and call it when using a new method getSingleTypedScalarResult. Is that technically feasible?

I have no idea 😅 it does sound feasible but I'm don't know much about the ORM, what's for sure is that I know less than you do 😛
Also, I don't understand what "it" refers to in "call it when using a new method", you can't call a query hint, can you? And finally, a new method sounds a bit overkill, I think it's acceptable to change the behavior of existing methods iff the query hint is provided.

EDIT: or do you mean the new method would add the hint right before executing the query?

@greg0ire commented on GitHub (May 27, 2020): > Idea: Introduce a new query hint that is checked in hydrator at the point, and call it when using a new method getSingleTypedScalarResult. Is that technically feasible? I have no idea :sweat_smile: it does sound feasible but I'm don't know much about the ORM, what's for sure is that I know less than you do :stuck_out_tongue: Also, I don't understand what "it" refers to in "call it when using a new method", you can't call a query hint, can you? And finally, a new method sounds a bit overkill, I think it's acceptable to change the behavior of existing methods iff the query hint is provided. EDIT: or do you mean the new method would add the hint right before executing the query?
admin commented 2026-01-22 15:33:49 +01:00
Author
Owner
Copy Link

@SenseException commented on GitHub (May 28, 2020):

And does this new method replace the old one? Is there a plan for deprecation?

@SenseException commented on GitHub (May 28, 2020): And does this new method replace the old one? Is there a plan for deprecation?
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
BC Break
Bug
Can't Fix
Deprecation
Documentation
DQL
Duplicate
Failing Test
Hacktoberfest
Improvement
Improvement
Incomplete
Invalid
Missing Tests
New Feature
PR Created
pull-request
Mirrored from GitHub Pull Request
 Question
Regression
Status: Waiting feedback
Waiting feedback
WIP
Won't Fix
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
admin
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: doctrine/archived-orm#6470
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?