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.

Sign up for GitHub

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

Jump to bottom

EZP-30507: Strict possible values for align attribute #96

Merged
merged 7 commits into from
Aug 13, 2019
Merged

EZP-30507: Strict possible values for align attribute #96

merged 7 commits into from
Aug 13, 2019

Conversation

SerheyDolgushev
Copy link
Contributor

@SerheyDolgushev SerheyDolgushev commented May 3, 2019 •
edited
Loading

Question Answer
JIRA issue EZP-30507
Bug/Improvement yes
New feature no
Target version 1.7
BC breaks no
Tests pass yes
Doc needed no

There is a strict list of possible values for ezxhtml:textalign: https://github.com/ezsystems/ezplatform-richtext/blob/master/src/lib/eZ/RichText/Resources/schemas/docbook/ezpublish.rng#L744-L756.

So custom values there were causing validation errors. This PR fixes it.

TODO:

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

@andrerom andrerom requested a review from vidarl May 3, 2019 13:49
@SerheyDolgushev SerheyDolgushev changed the title Fix custom align EZP-30507: Strict possible values for align attribute May 3, 2019
vidarl
vidarl requested changes May 7, 2019
View reviewed changes
Copy link
Member

@vidarl vidarl left a comment

Choose a reason for hiding this comment

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

This PR needs to be based against 1.7, not master

tests/lib/FieldType/Converter/Xslt/_fixtures/ezxml/002-para.xml Show resolved Hide resolved
vidarl
vidarl reviewed May 7, 2019
View reviewed changes
lib/FieldType/XmlText/Input/Resources/stylesheets/eZXml2Docbook_core.xsl Outdated
<xsl:otherwise>
<!-- we can not use custom values here, as it will break richtext shema
Copy link
Member

Choose a reason for hiding this comment

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

Just remove this <xsl:otherwise> block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

6360dd5#diff-d46ebb9d05db0e0a34282d3383ae3512L782

@SerheyDolgushev SerheyDolgushev changed the base branch from master to 1.7 May 7, 2019 14:15
@SerheyDolgushev SerheyDolgushev changed the base branch from 1.7 to master May 7, 2019 14:15
@SerheyDolgushev SerheyDolgushev changed the base branch from master to 1.7 May 8, 2019 07:38
@SerheyDolgushev SerheyDolgushev force-pushed the fix-custom-align branch 2 times, most recently from bdf375d to 6d373a0 Compare May 8, 2019 08:12
@SerheyDolgushev SerheyDolgushev mentioned this pull request Jul 30, 2019
Closed
4 tasks
alongosz
alongosz reviewed Jul 30, 2019
View reviewed changes
lib/FieldType/XmlText/Input/Resources/stylesheets/eZXml2Docbook_core.xsl Outdated
</xsl:otherwise>
</xsl:choose>
</xsl:attribute>
<xsl:call-template name="customtagalign">
Copy link
Member

Choose a reason for hiding this comment

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

Why would you call customtagalign for embed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before this change any custom value was allowed for embed's ezxhtml:align. And it is wrong, and in some cases might cause validation errors.

Copy link
Member

Choose a reason for hiding this comment

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

Before this change any custom value was allowed for embed's ezxhtml:align. And it is wrong, and in some cases might cause validation errors.

Still don't follow what is the relation between embed and custom tags in this context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ezembed (as well as custom tags) uses ezxhtml:align attribute. And possible values here are: center , left and right (https://github.com/ezsystems/ezplatform-richtext/blob/master/src/lib/eZ/RichText/Resources/schemas/docbook/ezpublish.rng#L758-L769). And we are using the same xsl function (customtagalign) to handle that attribute for embeds and custom tags.

Copy link
Member

Choose a reason for hiding this comment

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

The template should just be named align, then we have a template textalign which deals with ezxhtml:textalign, and a align which deals with ezxhtml:align

lib/FieldType/XmlText/Input/Resources/stylesheets/eZXml2Docbook_core.xsl Show resolved Hide resolved
@SerheyDolgushev
Copy link
Contributor Author

@alongosz sorry, how can we fix The following exception is caused by a lack of memory or swap, or not having swap configured error in the tests?

@mnocon
Copy link
Member

mnocon commented Aug 5, 2019 •
edited
Loading

@SerheyDolgushev I'll have a look at it

EDIT: See #108

@mnocon mnocon mentioned this pull request Aug 5, 2019
Merged
2 tasks
@mnocon
Copy link
Member

mnocon commented Aug 5, 2019

@SerheyDolgushev #108 is merged, if you rebase the build should be green now

@SerheyDolgushev
Copy link
Contributor Author

@mnocon thanks, it helped!

@mnocon mnocon mentioned this pull request Aug 9, 2019
Merged
vidarl
vidarl requested changes Aug 9, 2019
View reviewed changes
Copy link
Member

@vidarl vidarl left a comment

Choose a reason for hiding this comment

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

template customtagalign needs to be renamed. Besides that, OK

lib/FieldType/XmlText/Input/Resources/stylesheets/eZXml2Docbook_core.xsl Outdated
</xsl:otherwise>
</xsl:choose>
</xsl:attribute>
<xsl:call-template name="customtagalign">
Copy link
Member

Choose a reason for hiding this comment

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

The template should just be named align, then we have a template textalign which deals with ezxhtml:textalign, and a align which deals with ezxhtml:align

@SerheyDolgushev
Copy link
Contributor Author

@vidarl please check 05436f6.

SylvainGuittard
SylvainGuittard approved these changes Aug 12, 2019
View reviewed changes
Copy link

@SylvainGuittard SylvainGuittard left a comment

Choose a reason for hiding this comment

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

Thanks @SerheyDolgushev for your contribution

@andrerom andrerom merged commit 7ada681 into ezsystems:1.7 Aug 13, 2019
@SerheyDolgushev
Copy link
Contributor Author

@andrerom can you please merge it in 1.8/1.9/master aswell?

@andrerom
Copy link
Contributor

Thats the plan, but was hoping some of the other PR's would reach ready for merge as well first ;)

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

@alongosz alongosz alongosz approved these changes

@SylvainGuittard SylvainGuittard SylvainGuittard approved these changes

@vidarl vidarl vidarl approved these changes

@adamwojs adamwojs adamwojs approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@SerheyDolgushev @mnocon @andrerom @adamwojs @vidarl @SylvainGuittard @alongosz