EZP-31626: Fixed rendering of embed locations

[SerheyDolgushev]

Question	Answer
JIRA issue	EZP-31626
Requires	ezsystems/ezplatform-admin-ui#1374
Bug/Improvement	yes
New feature	no
Target version	2.0
BC breaks	no
Tests pass	yes
Doc needed	no

We faced this issue during upgrade to eZ Platform v3 from eZ Publish. And seems like the only ezcontent:// links are used in eZ Platform v3 UI, which makes hard to reproduce it on the clean eZ Platform v3.0.3 installation.

Steps to reproduce

1. Create a new Article, and embed location in its RichText field. The simplest way is to use [https://gitlab.com/contextualcode/ezplatform-alloyeditor-source] and add the following HTML:

<div data-ezelement="ezembed" data-href="ezlocation://2" data-ezview="embed" data-ez-node-initialized="true">&nbsp;</div>

2. Save the new article and open its full view page.

Expected result
Embed location will be rendered

Actual result
The following exception is thrown:

Argument 4 passed to eZ\Publish\Core\Repository\Permission\CachedPermissionService::canUser() must be of the type array, object given, called in XXX/vendor/ezsystems/ezplatform-kernel/eZ/Publish/Core/MVC/Symfony/Security/Authorization/Voter/ValueObjectVoter.php on line 63

TODO:

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

[alongosz]

On second thought, the bug is also valid for 2.5 LTS. Please rebase onto 1.1.

[SerheyDolgushev]

On second thought, the bug is also valid for 2.5 LTS. Please rebase onto 1.1.

Done.

[micszo]

Hi @SerheyDolgushev, I ran into this error while testing your change.
It's on eZ Platform EE 2.5 with ezplatform-alloyeditor-source v2.1.0 (it occurs on v3 with 3.1.0 too).

Steps:

1. Create new article.
2. In a richtext field click Edit source.
3. Insert code <div data-ezelement="ezembed" data-href="ezlocation://2" data-ezview="embed" data-ez-node-initialized="true">&nbsp;</div>
4. Cick Update.

Actual result: Error occurs in UI "Cannot read property 'Result' of undefined", on Network tab in browser console there is error 500 Parameter "viewId" for route "ezpublish_rest_views_load" must match "[^/]++" ("embed-load-content-info-ezlocation://2" given) to generate a corresponding URL.

Please advise.

[SerheyDolgushev]

I saw that too, but we just ignored this error. Please correct me, if I'm wrong, but the UI in v2/v3 is not designed to allow us to use ezlocation URLs. Anyway, there might be a lot of such embeds on upgraded projects. This PR is just about being able to render such embeds. A separate PR might be required to provide a way to edit such embed in the UI.

So updated steps to reproduce are:

1. Create a new Article, and embed location in its RichText field. The simplest way is to use [https://gitlab.com/contextualcode/ezplatform-alloyeditor-source] and add the following HTML:

<div data-ezelement="ezembed" data-href="ezlocation://2" data-ezview="embed" data-ez-node-initialized="true">&nbsp;</div>

2. Save the new article, ignoring the error
3. Open saved article full view page

[SerheyDolgushev]

@micszo

[alongosz]

I saw that too, but we just ignored this error.

@SerheyDolgushev I'm sorry but this is not acceptable. You cannot submit solution which solves one issue, but introduces or exposes another one.

If you want to support ezlocation it needs to be complete or at least not crash OE (sometimes we do just that to unblock separate tasks).
If you feel it's a separate scope - I'm always for smaller PRs, but please submit another PR or just add separate commit here so QA can test this.

It cannot be approved if there are errors.

[SerheyDolgushev]

Lets say theoretical eZ Publish site was upgraded to eZ Platform. And ezlocation links were used for some embeds on some pages.

Without this PR full view for those pages will crash. With this PR full view for those pages will work just fine.
And it is oblivious that this PR fixes an issue in the sources: targets expected to be an array, but Location is used there.

And I agree that a separate change might be required to provide ability to edit embed locations in the UI. Commits to implement it are more than welcome (in this PR or in the separate PR).

But right now this PR is about fixing the rendering of currently existing embed locations. And personally I do not see any reasons why it can not be merged. It does not bring any new issues. It partly solves already existing issue.

I guess partial fix is worst than complete fix, but better than no fix at all.

[alongosz]

OE cannot crash. It crashes not just when inserting such markup, but when editing existing imported item.

[SerheyDolgushev]

Sorry, just to double-check I clearly understand your point.

Right now there are two issues related to embed locations:

1. Crash on the rendering
2. OE crashes when embed locations are edited.

This PR fixes the first issue, but does not fix the second one. And it can not be approved, because it does not fixes OE crash.

[alongosz]

Sorry, just to double-check I clearly understand your point.

Right now there are two issues related to embed locations:

1. Crash on the rendering
2. OE crashes when embed locations are edited.

This PR fixes the first issue, but does not fix the second one. And it can not be approved, because it does not fixes OE crash.

You've given steps to reproduce which involve ignoring existing error. This does not meet quality requirements.
If you provide steps to reproduce w/o exposing bug[1] in OE, then we can talk.

[1] It's not really a bug in eZ Platform stack, it's just not supported. But given that we've promised migration from eZ Publish to eZ Platform will work, we're treating some of those as bugs (common sense applied) and as a separate case (migration vs OE)

[dew326]

I created a PR to fix the error (it will not provide support for ezlocation but at least it will not fail with an error) ezsystems/ezplatform-admin-ui#1374

[SerheyDolgushev]

@dew326 Thanks a lot for helping with this PR!

[alongosz]

+1 with ezsystems/ezplatform-admin-ui#1374

[micszo]

QA Approved on eZ Platform EE 2.5 with diff.
Tested together with ezsystems/ezplatform-admin-ui#1374.

[alongosz]

All related PRs merged up.
Thank you @SerheyDolgushev and @dew326 🎉