[Keyword fieldtype] Added ORDER BY to keyword query

[mnocon]

Question	Answer
JIRA issue	n/a
Type	improvement
Target Ibexa version	v3.3 and higher
BC breaks	no

The keyword fieldtype tests sometimes fail:
https://github.com/ibexa/core/actions/runs/3195771808/jobs/5217051683

1) Ibexa\Tests\Integration\Core\Repository\FieldType\KeywordIntegrationTest::testDeleteTranslation
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
     'fieldDefIdentifier' => 'data'
     'value' => Ibexa\Core\FieldType\Keyword\Value Object (
         'values' => Array (
-            0 => 'eng-US sindelfingen'
+            0 => 'eng-US bar'
             1 => 'eng-US foo'
-            2 => 'eng-US bar'
+            2 => 'eng-US sindelfingen'
         )
     )
     'languageCode' => 'eng-US'
     'fieldTypeIdentifier' => 'ezkeyword'
 )

The returned order of keywords is different from the one we except. It's because the query does not specify the expected order, meaning they can be returned in a random one.

Things done:

1. Added ORDER BY to the keyword query to make the returned value consistent
2. Applied suggestions from Andrew made during private conversation when I first mentioned this failure:

• extracted $query->expr() to a separate variable
• changed parameter naming to follow our convention
• changed deprecated fetchAll(FetchMode::COLUMN) usage

Testing

Basic sanities using the keyword fieldtype are needed (creating, updating)

Checklist:

• Provided PR description.
• Tested the solution manually.
• Provided automated test coverage.
• Checked that target branch is set correctly (master for features, the oldest supported for bugs).
• Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
• Asked for a review (ping @ezsystems/engineering-team).

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[Steveb-p]

Do note however that Doctrine documentation does not endorse storing ExpressionBuilder in a variable: https://www.doctrine-project.org/projects/doctrine-orm/en/2.13/reference/query-builder.html#working-with-querybuilder

You can see there that they are re-requesting the Expression Builder from the Query Builder every time:

<?php
// $qb instanceof QueryBuilder

$qb->select(array('u')) // string 'u' is converted to array internally
   ->from('User', 'u')
   ->where($qb->expr()->orX(
       $qb->expr()->eq('u.id', '?1'),
       $qb->expr()->like('u.nickname', '?2')
   ))
   ->orderBy('u.surname', 'ASC');

[alongosz]

Do note however that Doctrine documentation does not endorse storing ExpressionBuilder in a variable: https://www.doctrine-project.org/projects/doctrine-orm/en/2.13/reference/query-builder.html#working-with-querybuilder

You can see there that they are re-requesting the Expression Builder from the Query Builder every time:

<?php
// $qb instanceof QueryBuilder

$qb->select(array('u')) // string 'u' is converted to array internally
   ->from('User', 'u')
   ->where($qb->expr()->orX(
       $qb->expr()->eq('u.id', '?1'),
       $qb->expr()->like('u.nickname', '?2')
   ))
   ->orderBy('u.surname', 'ASC');

@Steveb-p not everything that is there follows best practices. This at some point popped up on some profiling tools not as performance issue (atm), but as something which might impact performance in the future. If you have deterministic function result called multiple times, you should expand it to variable, unless 3rd party doc explicitly says why you shouldn't.

[Steveb-p]

@Steveb-p not everything that is there follows best practices.

I have no doubt about that, but you can criticize any code library in a similar fashion, because there is always something that can be done differently.

If you have deterministic function result called multiple times, you should expand it to variable, unless 3rd party doc explicitly says why you shouldn't.

It may stop being a deterministic function, because it is never stated that it is. It may start returning new ExpressionBuilder objects due to the need to maintain some sort of state (just like QueryBuilder does). Assuming it will not is, well, assuming something. And that works most of the time. Most.

[alongosz]

We shouldn't add time and cognitive complexity to our code just because in a distant unlikely future something might change. The quoted examples are not convincing enough argument for me to change the approach. They're meant to visualize something else. If Doctrine states "always call $queryBuilder->expr() because it might become dynamic in the future", then we will. Until then we should follow best practices and don't call multiple times within a given scope a method which returns the same result.

[tomaszszopinski]

QA approved on IbexaDXP 3.3 commerce.