Skip to content

IBX-10425: Translated string is returned as XML#29

Merged
konradoboza merged 9 commits into4.6from
IBX-10425-return-XML-in-a-translated-string
Apr 30, 2026
Merged

IBX-10425: Translated string is returned as XML#29
konradoboza merged 9 commits into4.6from
IBX-10425-return-XML-in-a-translated-string

Conversation

@mateuszdebinski
Copy link
Copy Markdown
Contributor

@mateuszdebinski mateuszdebinski commented Aug 7, 2025

🎫 Issue IBX-10425

Description:

For QA:

Documentation:

@mateuszdebinski mateuszdebinski self-assigned this Aug 7, 2025
@mateuszdebinski mateuszdebinski force-pushed the IBX-10425-return-XML-in-a-translated-string branch from 7945846 to d3ec14a Compare August 7, 2025 09:43
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Aug 7, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
62.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@mateuszdebinski mateuszdebinski force-pushed the IBX-10425-return-XML-in-a-translated-string branch from 57030ba to 0d8cd24 Compare November 24, 2025 14:16
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Nov 24, 2025

Quality Gate Passed Quality Gate passed

Issues
1 New issue
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

@mateuszdebinski mateuszdebinski marked this pull request as ready for review November 24, 2025 14:17
@mateuszdebinski mateuszdebinski requested a review from a team November 24, 2025 14:17
@mateuszdebinski mateuszdebinski added Bug Something isn't working Ready for review labels Nov 24, 2025
mikadamczyk
mikadamczyk reviewed Dec 16, 2025
View reviewed changes
Copy link
Copy Markdown

@mikadamczyk mikadamczyk left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should add unit coverage for EncoderHelper::clearCDATAInTextField() to prove it strips CDATA on non‑RichText fields while preserving RichText, and an integration-style Encoder test that asserts the final XML contains plain text for regular fields and the placeholder tags for RichText. This guards the new logic and prevents regressions.

Comment thread src/lib/EncoderHelper.php Outdated
@mikadamczyk mikadamczyk requested a review from a team December 16, 2025 15:41
konradoboza
konradoboza reviewed Dec 17, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@konradoboza konradoboza left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments from Mikołaj are valid.

Comment thread src/lib/EncoderHelper.php Outdated
konradoboza
konradoboza reviewed Apr 9, 2026
View reviewed changes
Comment thread tests/lib/EncoderTest.php Outdated
Comment thread tests/lib/EncoderTest.php Outdated
Comment thread tests/lib/EncoderTest.php Outdated
Comment thread phpstan-baseline.neon Outdated
konradoboza
konradoboza approved these changes Apr 9, 2026
View reviewed changes
Comment thread tests/lib/EncoderTest.php Outdated
@mateuszdebinski mateuszdebinski force-pushed the IBX-10425-return-XML-in-a-translated-string branch from 85bf0c6 to bfd0999 Compare April 9, 2026 13:04
barw4
barw4 approved these changes Apr 9, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@barw4 barw4 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems there is a huge duplication in tests/lib/EncoderTest.php per Sonar that we could tackle

@mateuszdebinski mateuszdebinski force-pushed the IBX-10425-return-XML-in-a-translated-string branch from bfd0999 to 3808037 Compare April 9, 2026 14:46
ciastektk
ciastektk reviewed Apr 10, 2026
View reviewed changes
Copy link
Copy Markdown

@ciastektk ciastektk left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we're abandoning this bundle in the near future, but please consider at least changing the naming.

Comment thread src/lib/EncoderHelper.php Outdated
use Ibexa\FieldTypeRichText\FieldType\RichText\Value as RichTextValue;
use RuntimeException;

final class EncoderHelper
Copy link
Copy Markdown

@ciastektk ciastektk Apr 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it is a matter of preference, but I really don't like use name Helper for cases like this.
IMO this could be TextFieldCdataCleaner, TextFieldDataSanitizer, TextFieldValueSanitizer, XmlTextFieldSanitizer

Comment thread src/lib/EncoderHelper.php Outdated
Comment on lines +41 to +44
if ($type !== RichTextValue::class) {
$newText = $dom->createTextNode($textNode->data);
$parent->replaceChild($newText, $textNode);
}
Copy link
Copy Markdown

@ciastektk ciastektk Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this is not SOLID and shouldn't be like this. This bundle will be abandoned in next few weeks so it is up to you if you have a time and resource to provide some more flexible solution.

Copy link
Copy Markdown
Contributor Author

@mateuszdebinski mateuszdebinski Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the name and changed the function to smaller functions

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Apr 28, 2026

Quality Gate Passed Quality Gate passed

Issues
1 New issue
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

@juskora juskora mentioned this pull request Apr 29, 2026
Closed
juskora
juskora approved these changes Apr 30, 2026
View reviewed changes
Copy link
Copy Markdown

@juskora juskora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA Approved on Ibexa DXP Commerce 4.6-dev.

@konradoboza konradoboza merged commit 850f5a6 into 4.6 Apr 30, 2026
7 checks passed
@konradoboza konradoboza deleted the IBX-10425-return-XML-in-a-translated-string branch April 30, 2026 06:10
@konradoboza
Copy link
Copy Markdown
Contributor

konradoboza commented Apr 30, 2026

@mateuszdebinski you can merge up changes.

Important note: the upmerge workflow has changed, the flow is now 4.6 -> 5.0 -> 6.0. Please make sure that you don't push main to the repository if it's on your local one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mikadamczyk mikadamczyk mikadamczyk approved these changes

@barw4 barw4 barw4 approved these changes

@konradoboza konradoboza konradoboza approved these changes

@juskora juskora juskora approved these changes

@ciastektk ciastektk Awaiting requested review from ciastektk

Assignees

@ciastektk ciastektk

@konradoboza konradoboza

@mateuszdebinski mateuszdebinski

Labels

Bug Something isn't working QA approved

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@mateuszdebinski @konradoboza @mikadamczyk @ciastektk @barw4 @juskora