use the empty string instead of null as an array offset by xabbuh · Pull Request #12160 · doctrine/orm

xabbuh · 2025-09-09T19:27:55Z

No description provided.

greg0ire · 2025-09-09T22:13:28Z

What prompted you to contribute this? Was it static analysis or was it tests? If it's static analysis and there are no unit tests covering theses cases, maybe we should use assertions instead?

xabbuh · 2025-09-10T04:01:19Z

Sorry I forgot to mention. This is a deprecation that will be introduced with PHP 8.5. We caught this in the Symfony CI. So I enabled a PHP 8.5 job on my fork and these were the places that were caught by the tests.

greg0ire · 2025-09-10T06:08:25Z

Oh ok so there are actual situations where this can happen 🤯

greg0ire · 2025-09-10T06:11:40Z

src/Persisters/Entity/JoinedSubclassPersister.php

    {
-        if (isset($this->class->fieldMappings[$this->class->versionField]->inherited)) {
-            $definingClassName = $this->class->fieldMappings[$this->class->versionField]->inherited;
+        if (isset($this->class->fieldMappings[$this->class->versionField ?? '']->inherited)) {


If we take this one as an example, does it happen in a realistic scenario, or would it make sense to trigger a deprecation ourselves in the next major release?

I have no idea. 🤷 But it's actually triggered a lot of times in the tests (see https://github.com/xabbuh/orm/actions/runs/17612071318/job/50036129395#step:6:84).

stof · 2025-09-10T16:51:53Z

src/Query/SqlWalker.php

            $sqlParts[] = $col . ' AS ' . $columnAlias;

-            $this->scalarResultAliasMap[$resultAlias][] = $columnAlias;
+            $this->scalarResultAliasMap[$resultAlias ?? ''][] = $columnAlias;


should it even be added in scalarResultAliasMap if there is no result alias ?

I am not sure here about the implications if we don't add it. In other places we use the $scalarResultCounter property but I have no idea if that would work as expected here.

after re-reading how the map is accessed in walkResultVariable() it's probably safe to assume that adding an entry here in case of $resultAlias being null doesn't make much sense

stof · 2025-09-10T16:52:58Z

src/Tools/Pagination/LimitSubqueryOutputWalker.php

        // Get any aliases that are available for select expressions.
        foreach ($AST->selectClause->selectExpressions as $selectExpression) {
-            $selectAliasToExpressionMap[$selectExpression->fieldIdentificationVariable] = $selectExpression->expression;
+            $selectAliasToExpressionMap[$selectExpression->fieldIdentificationVariable ?? ''] = $selectExpression->expression;


shouldn't we rather skip expressions that have no identification variable (instead of keeping only the last one as they will anyway override each other in that map) ?

That's true, it didn't make a difference before as we never read from the map below when $orderByItem->expression was not a string. So we can just abstain from adding it.

stof · 2025-09-10T16:53:39Z

src/Tools/Pagination/LimitSubqueryOutputWalker.php

        foreach ($orderByClause->orderByItems as $orderByItem) {
-            if (is_string($orderByItem->expression) && isset($selectAliasToExpressionMap[$orderByItem->expression])) {
-                $orderByItem->expression = $selectAliasToExpressionMap[$orderByItem->expression];
+            if (is_string($orderByItem->expression) && isset($selectAliasToExpressionMap[$orderByItem->expression ?? ''])) {


this is useless, as is_string($orderByItem->expression) already ensures it is not null (same on next line)

indeed, reverted

stof · 2025-09-10T16:56:07Z

src/Persisters/Entity/JoinedSubclassPersister.php

    {
-        if (isset($this->class->fieldMappings[$this->class->versionField]->inherited)) {
-            $definingClassName = $this->class->fieldMappings[$this->class->versionField]->inherited;
+        if (isset($this->class->fieldMappings[$this->class->versionField ?? '']->inherited)) {


I think it will be clearer to handle the (common) case of classes without a version field explicitly instead of attempting an array check with the empty string.

Suggested change

if (isset($this->class->fieldMappings[$this->class->versionField ?? '']->inherited)) {

if ($this->class->versionField !== null && isset($this->class->fieldMappings[$this->class->versionField]->inherited)) {

This PR was merged into the 6.4 branch. Discussion ---------- Fix ord()-related PHP 8.5 deprecations | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | - | License | MIT The remaining deprecations will be fixed by doctrine/orm#12160 Commits ------- 5314fd5 Fix ord()-related PHP 8.5 deprecations

xabbuh · 2025-09-20T08:27:09Z

thanks for the reviews, comments addressed, this is ready from my side

derrabus · 2025-09-26T09:45:35Z

I thought it might be great if PHPStan could help us in that regard. phpstan/phpstan#13592

derrabus

LGTM, just a nit-pick.

derrabus · 2025-09-26T09:50:19Z

src/Query/SqlWalker.php

            $sqlParts[] = $col . ' AS ' . $columnAlias;

-            $this->scalarResultAliasMap[$resultAlias][] = $columnAlias;
+            if (is_string($resultAlias)) {


Let's make it explicit that this is actually a null-check.

Suggested change

if (is_string($resultAlias)) {

if ($resultAlias !== null) {

derrabus · 2025-09-26T09:50:26Z

src/Query/SqlWalker.php

                    $sqlParts[] = $col . ' AS ' . $columnAlias;

-                    $this->scalarResultAliasMap[$resultAlias][] = $columnAlias;
+                    if (is_string($resultAlias)) {


xabbuh · 2025-09-29T08:47:09Z

@derrabus made me aware that 2.20 is still maintained so here is another PR targeting the 2.20.x branch: #12181 (merging up will later then lead to conflicts so I keep this PR open as a reference on how to resolve them)

xabbuh · 2025-10-07T13:51:08Z

#12191 handles this

xabbuh force-pushed the php-8.5 branch 2 times, most recently from 0e2692f to 3aa7791 Compare September 9, 2025 19:54

greg0ire reviewed Sep 10, 2025

View reviewed changes

greg0ire previously approved these changes Sep 10, 2025

View reviewed changes

greg0ire requested review from SenseException and derrabus September 10, 2025 15:40

stof reviewed Sep 10, 2025

View reviewed changes

nicolas-grekas mentioned this pull request Sep 16, 2025

Fix ord()-related PHP 8.5 deprecations symfony/symfony#61766

Merged

xabbuh dismissed greg0ire’s stale review via 483392f September 20, 2025 07:53

xabbuh force-pushed the php-8.5 branch 3 times, most recently from 530099d to 3bb9931 Compare September 20, 2025 08:12

SenseException previously approved these changes Sep 21, 2025

View reviewed changes

derrabus previously approved these changes Sep 26, 2025

View reviewed changes

use the empty string instead of null as an array offset

cb1637c

xabbuh dismissed stale reviews from derrabus and SenseException via cb1637c September 26, 2025 10:42

xabbuh force-pushed the php-8.5 branch from 3bb9931 to cb1637c Compare September 26, 2025 10:42

SenseException approved these changes Sep 28, 2025

View reviewed changes

xabbuh mentioned this pull request Sep 29, 2025

use the empty string instead of null as an array offset #12181

Merged

xabbuh mentioned this pull request Oct 7, 2025

Add a CI job that fails on deprecations #12188

Merged

xabbuh closed this Oct 7, 2025

xabbuh deleted the php-8.5 branch October 7, 2025 13:51

	if (isset($this->class->fieldMappings[$this->class->versionField ?? '']->inherited)) {
	if ($this->class->versionField !== null && isset($this->class->fieldMappings[$this->class->versionField]->inherited)) {

Conversation

xabbuh commented Sep 9, 2025

Uh oh!

greg0ire commented Sep 9, 2025

Uh oh!

xabbuh commented Sep 10, 2025

Uh oh!

greg0ire commented Sep 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commented Sep 20, 2025

Uh oh!

derrabus commented Sep 26, 2025

Uh oh!

derrabus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commented Sep 29, 2025

Uh oh!

xabbuh commented Oct 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xabbuh Sep 10, 2025 •

edited

Loading