IBX-6833: Copying empty fields during translation copying shouldn't be skipped

[barw4]

🎫 Issue	IBX-6833

Description:

When one is copying translations using the copyTranslationsFromPublishedVersion method during publishing empty values are left out and not stored in the database which could result in entries not appearing in ezcontentobject_attribute table resulting in missing translations and broken language masks.

For QA:

Maintainer note:

Requires extensive multilingual content updating testing, ideally in multiple tabs or even by at least 2 separate editors at the same time (though for that unrelated bugs might occur). Possible regressions: EZEE-2661 but doesn't really require scheduler interaction, rather checking if translations were copied properly.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[ViniTou]

I can not wrap my head around it, why storing empty values is somehow breaking our system? How it is handled when you just creating version with empty values?

Also, which of those is not enough so the isCopied flag is not set to true and you need to pass additional argument?

                } elseif (!$isFieldUpdated && $isLanguageNew && !$fieldDefinition->isTranslatable) {
                    $isCopied = true;
                }

[barw4]

I can not wrap my head around it, why storing empty values is somehow breaking our system?

@ViniTou The problem is that they are not stored.

Also, which of those is not enough so the isCopied flag is not set to true and you need to pass additional argument?

The isCopied flag is irrelevant here. Our system fails when ($isLanguageNew && $isEmpty) === true and that's what happens if we are copying translations from published version using the copyTranslationsFromPublishedVersion method. In such case, empty value is skipped and therefore an empty field that ought to be translated is not injected into a db.

[ViniTou]

Ok, then in which case (outside of copying translation) $isLanguageNew && $isEmpty is happening and storing is skipped? Like it seems that there is/was valid case for skipping empty values without breaking the system.

The isCopied flag is irrelevant here.

I know, it was more about, that maybe we can set it using some other logic to not skip adding without introducing new argument to method signature.

[barw4]

Ok, then in which case (outside of copying translation) $isLanguageNew && $isEmpty is happening and storing is skipped? Like it seems that there is/was valid case for skipping empty values without breaking the system.

For example when we are creating a new version in a new language and we are not basing it on another translation so that values are empty.

I know, it was more about, that maybe we can set it using some other logic to not skip adding without introducing new argument to method signature.

The isCopied flag is mostly used for non-translatable fields so I cannot see how would it help in this case:

 elseif (!$isFieldUpdated && $isLanguageNew && !$fieldDefinition->isTranslatable) {
                    $isCopied = true;
                }

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[webhdx]

It's quite a drastic change and we definitely need a solid QA round here to detect possible side effects. I also feel like fields were skipped for a reason but adding them anyway seems like a reasonable solution to keep data integrity.

[alongosz]

After digging into this deeper, I don't see any apparent issues with this change, though there might be some unforeseen side-effects. Indeed we need to vary that internal method based on either content update or copying fields during publishing, which reuses that method.

Probably more refactoring would be better suited now that we know these operations are actually not the same, but this - what @barw4 did now - seems like the simplest way to achieve it.

I've updated PR description with some extra tips for QA to investigate.

[sonarcloud]

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 SonarCloud

[tomaszszopinski]

QA approved on Ibexa DXP oss 3.3.x-dev