Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IBX-5961: Fixed validation error due to not accepted TextAlign value #233

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

vidarl
Copy link
Member

@vidarl vidarl commented Jun 14, 2023

Question Answer
JIRA issue IBX-5961
Bug/Improvement yes
New feature no
Target version Ibexa DXP 3.3 and 4.5
BC breaks no
Tests pass yes
Doc needed no

When cut&pasting from word, you might get TextAlign values that doesn't match our schema.

With this patch, TextAlign will default to"left" if the value specified is not acceptable

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@vidarl vidarl changed the title IBX-5961: Cut&pasting from Word might cause invaliation error due to … IBX-5961: Cut&pasting from Word might cause invaliation error due to not accepted TextAlign value Jun 14, 2023
@Steveb-p Steveb-p changed the title IBX-5961: Cut&pasting from Word might cause invaliation error due to not accepted TextAlign value IBX-5961: Cut&pasting from Word might cause validation error due to not accepted TextAlign value Jun 14, 2023
@Steveb-p Steveb-p changed the title IBX-5961: Cut&pasting from Word might cause validation error due to not accepted TextAlign value IBX-5961: Pasting from Word might cause validation error due to not accepted TextAlign value Jun 14, 2023
<xsl:when test="$alignValue = 'left' or $alignValue = 'center' or $alignValue = 'right' or $alignValue = 'justify'">
<xsl:value-of select="$alignValue"/>
</xsl:when>
<xsl:otherwise>left</xsl:otherwise>
Copy link
Member Author

Choose a reason for hiding this comment

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

So, I here a default value of left is used when the value provided is not supported.

This means that we silently changes whatever user provided. However, the validation errors provided by libxml is anyway not very informative so I think this is okay

@vidarl
Copy link
Member Author

vidarl commented Jun 15, 2023

@ezsystems/php-dev-team : please review

@konradoboza konradoboza requested review from alongosz and a team June 15, 2023 13:35
Copy link
Member

@konradoboza konradoboza left a comment

Choose a reason for hiding this comment

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

Please provide some test coverage showing that text-align: left is indeed set by default.

@konradoboza konradoboza requested a review from a team June 15, 2023 13:39
@vidarl
Copy link
Member Author

vidarl commented Jun 15, 2023

Please provide some test coverage showing that text-align: left is indeed set by default.

Ahh.. two file are missing from the commit.. comming right up...

Copy link
Member

@konradoboza konradoboza left a comment

Choose a reason for hiding this comment

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

LGTM concerning challenges coming from converting inconsistent Word-related formatting. Probably @alongosz might have some further remarks.

@konradoboza konradoboza requested a review from a team June 15, 2023 13:57
@alongosz alongosz changed the title IBX-5961: Pasting from Word might cause validation error due to not accepted TextAlign value IBX-5961: Fixed validation error due to not accepted TextAlign value Jun 19, 2023
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

I still believe that input sanitization is a responsibility of an input provider, not handler, which is Online Editor in this case. We're opening ourselves here to wide range of bug fixes which might in the end pollute already complicated XSLT.

But given this solves the issue in a clean way (at least for XSL standards): +1

Copy link
Member

@micszo micszo 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 Commerce 3.3.34-dev.

@micszo micszo removed their assignment Jul 19, 2023
@vidarl vidarl force-pushed the IBX-5961_TextAlign_start branch from 4eecd3c to a1f64e9 Compare July 19, 2023 10:14
@konradoboza konradoboza merged commit d1d4a84 into 2.3 Jul 19, 2023
@konradoboza konradoboza deleted the IBX-5961_TextAlign_start branch July 19, 2023 10:25
alongosz added a commit to ibexa/fieldtype-richtext that referenced this pull request Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants