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

EZP-30513: eZXMLText Migration: ignore nested embeds in the link #98

Closed

Conversation

SerheyDolgushev
Copy link
Contributor

@SerheyDolgushev SerheyDolgushev commented May 7, 2019

Question Answer
JIRA issue EZP-30513
Type Improvement
Target version 1.7
BC breaks no
Doc needed no

We found that in some cases there might be nested embed-inline elements inside link in eZ XML Text:

<link node_id="333">some text
  <embed-inline view="embed-inline" size="original" object_id="444"/>
</link>

And it will be converted to invalid RichText:

<link xlink:href="ezlocation://333" xlink:show="none">some text
  <ezembedinline xlink:href="ezcontent://444" view="embed-inline">
    <ezlink xlink:href="ezlocation://333" xlink:show="none"/>
    <ezconfig>
      <ezvalue key="size">original</ezvalue>
    </ezconfig>
  </ezembedinline>
</link>

Such kind embeds need to be ignored.

TODO:

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

@SerheyDolgushev SerheyDolgushev changed the base branch from master to 1.7 May 8, 2019 08:24
@SerheyDolgushev SerheyDolgushev force-pushed the fix-nested-embeds branch 2 times, most recently from 176b215 to beeceff Compare May 8, 2019 12:14
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.

Do you know in what scenario the user might got such nested embeds inside links? Are the embeds rendered somehow in legacy? if it is, maybe the embed should be moved before or after the link?

@SerheyDolgushev
Copy link
Contributor Author

Sorry, not sure what is the best scenario to reproduce this kind of data in legacy. Most likely it was populated by some legacy import scripts.

@SerheyDolgushev
Copy link
Contributor Author

SerheyDolgushev commented Aug 8, 2019

@mnocon can you please check why tests are failing here?

E: Package 'mysql-server-5.6' has no installation candidate
E: Package 'mysql-client-core-5.6' has no installation candidate
E: Package 'mysql-client-5.6' has no installation candidate

@mnocon
Copy link
Member

mnocon commented Aug 8, 2019

@SerheyDolgushev see #109

@mnocon
Copy link
Member

mnocon commented Aug 9, 2019

@SerheyDolgushev #109 is merged, rebasing should help here

@SerheyDolgushev
Copy link
Contributor Author

@mnocon thanks!

@andrerom
Copy link
Contributor

andrerom commented Aug 13, 2019

@vidarl isn't embed-inline a text type inline tag which can be within anything (link, paragraph, bold, custom-inline, ..) ?

@SerheyDolgushev do we have something to convert to instead of removing them?

@SerheyDolgushev
Copy link
Contributor Author

@andrerom don't think there is any reason to convert this. As the only source for such kind content is custom imports/etc. And most likely such content wasn't displayed properly in the legacy. So most likely it is something which was imported in a wrong way, and nobody noticed it in the legacy. That's why I vote just for skipping it.

@vidarl
Copy link
Member

vidarl commented Aug 13, 2019

@vidarl isn't embed-inline a text type inline tag which can be within anything (link, paragraph, bold, custom-inline, ..) ?

yes, well. I am not sure if legacy can have it within anything. That is why I asked how legacy rendered this. We might have to move (not delete) it as we do with links inside links:

https://github.com/ezsystems/ezplatform-xmltext-fieldtype/blob/master/lib/FieldType/XmlText/Converter/RichText.php#L422

@andrerom
Copy link
Contributor

@SerheyDolgushev Could you:

  • Double check assumptions here and get someone to render the content where you encountered this on the said legacy install?
  • Is it ok to go for the approach @vidarl suggests above?

@vidarl
Copy link
Member

vidarl commented Aug 19, 2019

I tested legacy, and legacy indeed renders both the link and the inline-embed.
It is quite possible to create such markup using simplified xml input. Was not able to do it with OE though

@SerheyDolgushev
Copy link
Contributor Author

Sorry, so what would be the next steps here? Should we move (but not delete, as @vidarl suggested) those elements?

@vidarl
Copy link
Member

vidarl commented Oct 1, 2019

Yes, move the embed outside of the link tag so that it is not lost

@vidarl
Copy link
Member

vidarl commented Oct 16, 2019

Closed this one in favor of #110

@vidarl vidarl closed this Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants