IBX-9446: [BO] Fatal error after non-translatable field added to Content Type

[vidarl]

🎫 Issue	IBX-9446

Description:

This is a fix for a regression caused by the "virtual fields" introduced in 98b7b50

It might be an idea to read https://issues.ibexa.co/browse/IBX-9446?focusedId=291445&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-291445

For QA:

See ticket for step-by-step on how to reproduce the problem

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[adamwojs]

@vidarl Could you please resolve issues reported by PHPStan ?

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[vidarl]

@vidarl Could you please resolve issues reported by PHPStan ?

@adamwojs : done

[vidarl]

Removed "draft" state for this PR now, as the test case is finally able to reproduce the problem and only success when fix is applied.

However, I cannot tell for sure if this is the correct fix for the problem as the topic is quite heavy IMO

@alongosz

[alongosz]

Thanks for the test case. It was important and necessary, so I can spend less time debugging the actual issue.

This looks like either a side-effect of performance fix ezsystems/ezplatform-kernel#409 or indeed, as you've mentioned, ezsystems/ezplatform-kernel#397 and for me the fix is fine!

Remarks regarding the test itself:

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[vidarl]

@alongosz : Ref my findings in https://issues.ibexa.co/browse/IBX-9446?focusedId=291445&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-291445, I wonder if we should always also put mainLanguage as first element in $allLanguageCodes ?

ie

diff --git a/src/lib/Repository/ContentService.php b/src/lib/Repository/ContentService.php
index 2cee84e93a..fd1946108b 100644
--- a/src/lib/Repository/ContentService.php
+++ b/src/lib/Repository/ContentService.php
@@ -1339,6 +1339,9 @@ class ContentService implements ContentServiceInterface
         }

         $allLanguageCodes = $this->contentMapper->getLanguageCodesForUpdate($contentUpdateStruct, $content);
+        array_unshift($allLanguageCodes, $content->contentInfo->mainLanguageCode);
+        $allLanguageCodes = array_unique($allLanguageCodes);
+
         $contentLanguageHandler = $this->persistenceHandler->contentLanguageHandler();
         foreach ($allLanguageCodes as $languageCode) {
             $contentLanguageHandler->loadByLanguageCode($languageCode);

As far as I know, there are no edge cases where $allLanguageCodes will not contain mainLanguage

[alongosz]

@alongosz : Ref my findings in https://issues.ibexa.co/browse/IBX-9446?focusedId=291445&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-291445, I wonder if we should always also put mainLanguage as first element in $allLanguageCodes ?

ie

diff --git a/src/lib/Repository/ContentService.php b/src/lib/Repository/ContentService.php
index 2cee84e93a..fd1946108b 100644
--- a/src/lib/Repository/ContentService.php
+++ b/src/lib/Repository/ContentService.php
@@ -1339,6 +1339,9 @@ class ContentService implements ContentServiceInterface
         }

         $allLanguageCodes = $this->contentMapper->getLanguageCodesForUpdate($contentUpdateStruct, $content);
+        array_unshift($allLanguageCodes, $content->contentInfo->mainLanguageCode);
+        $allLanguageCodes = array_unique($allLanguageCodes);
+
         $contentLanguageHandler = $this->persistenceHandler->contentLanguageHandler();
         foreach ($allLanguageCodes as $languageCode) {
             $contentLanguageHandler->loadByLanguageCode($languageCode);

As far as I know, there are no edge cases where $allLanguageCodes will not contain mainLanguage

@vidarl I don't follow how this change would help you with the issue.

[vidarl]

@vidarl I don't follow how this change would help you with the issue.

@alongosz : For this specific issue (fatal error) it won't change anything. so guess it can be treated as a separate but related issue.

But let me explain some odd behavior. If you run this PR's test as-is, and then check the DB you'll see the following:
(contentclassattribute_id=296 is the non-translatable field):

select id,version,data_text,data_type_string,language_code from ezcontentobject_attribute where contentclassattribute_id=296;
+-----+---------+-----------+------------------+---------------+
| id  | version | data_text | data_type_string | language_code |
+-----+---------+-----------+------------------+---------------+
| 223 |       2 | NULL      | ezstring         | eng-US        |
+-----+---------+-----------+------------------+---------------+
1 row in set (0.000 sec)

Then, in the test replace all eng-US with eng-GB an rerun the test, you'll end up with:

select id,version,data_text,data_type_string,language_code from ezcontentobject_attribute where contentclassattribute_id=296;
+-----+---------+-----------+------------------+---------------+
| id  | version | data_text | data_type_string | language_code |
+-----+---------+-----------+------------------+---------------+
| 223 |       2 | NULL      | ezstring         | ger-DE        |
| 224 |       2 | NULL      | ezstring         | eng-GB        |
+-----+---------+-----------+------------------+---------------+
2 rows in set (0.000 sec)

The reason is this thing with order of $allLanguageCodes array. However, when trying to reproduce this with admin-ui am only seeing behavior two ( with admin-ui a row for each language is also created for non-translatable fields ).

I guess admin-ui behavior is the correct one, and in that case my proposal for ordering $allLanguageCodes array is definitely wrong as this will cause only one row to be created both if eng-US or eng-GB is used in the testcase

But there is something fishy here

[alongosz]

@vidarl there might be, because reordering the language like that should have no effect to API. I have a feeling this would produce some unrelated edge cases, so I'd like to avoid it if not necessary.

[alongosz]

From my side the solution as-is is okay.