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

Implement EZP-21315: Implement embed tag for RichText #758

Merged
merged 18 commits into from Mar 7, 2014

Conversation

4 participants
@pspanja
Copy link
Contributor

commented Feb 28, 2014

This PR resolves issues https://jira.ez.no/browse/EZP-21315 and https://jira.ez.no/browse/EZP-21387

Dedicated embed tags ezembed and ezembedinline are implemented.
In internal format those support configuration as an arbitrary hash structure. This is also available in XHTML edit format, anticipating for feature like CKEditor widgets. Example:

<ezembed xlink:href="ezlocation://601" view="embed-inline">
  <ezconfig>
    <ezvalue key="size">medium</ezvalue>
    <ezvalue key="offset">10</ezvalue>
    <ezvalue key="limit">5</ezvalue>
  </ezconfig>
</ezembed>

RichText now defines EmbedRendererInterface, implemented in Symfony MVC layer using FragmentHandler with ESI strategy. This will fallback to inlining in case when reverse proxy is not available.

EmbedRenderer uses new actions implemented in ViewController: embedContent() and embedLocation(), taking care of checking permissions and handling visibility. This means existing view provider and custom controller configuration applies for them, also rendering fallback to Legacy Stack is possible.

Possible followup on this part is https://jira.ez.no/browse/EZP-22068.

@ezrobot

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2014

This Pull Request does not respect our PHP Coding Standards, please, see the report below:

FILE: ...niffer/workspace/eZ/Publish/Core/FieldType/RichText/Converter/Embed.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 138 | ERROR | A space is required after opening parenthesis of function call
 138 | ERROR | A space is required before closing parenthesis of function call
--------------------------------------------------------------------------------
@pspanja

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2014

if ( empty( $resourceReference ) )
{
if ( isset( $this->logger ) )

This comment has been minimized.

Copy link
@lolautruche

lolautruche Feb 28, 2014

Contributor

Not needed to check this as you enforce the presence of @logger

This comment has been minimized.

Copy link
@pspanja

pspanja Feb 28, 2014

Author Contributor

Oops, that needs to be fixed :)

This comment has been minimized.

Copy link
@pspanja

pspanja Feb 28, 2014

Author Contributor

Fixed in b80c5f1.

if ( !$viewType = $embed->getAttribute( "view" ) )
{
// Mapping default view names
$map = array(

This comment has been minimized.

Copy link
@andrerom

andrerom Feb 28, 2014

Member

This can be defined outside foreach

This comment has been minimized.

Copy link
@pspanja

pspanja Feb 28, 2014

Author Contributor

Fixed in 3cb75d8.

@@ -158,42 +232,89 @@ public function viewLocation( $locationId, $viewType, $layout = false, array $pa
*/
public function viewContent( $contentId, $viewType, $layout = false, array $params = array() )
{
// @todo: remove fallback to self::embedContent(), kept for BC (5.2)

This comment has been minimized.

Copy link
@lolautruche

lolautruche Feb 28, 2014

Contributor

Please avoid adding todos in code, prefer filing an issue on Jira :-)

This comment has been minimized.

Copy link
@pspanja

pspanja Feb 28, 2014

Author Contributor

Removed in 8a6052c, followup issue will be created when this is merged.

// Check both 'content/read' and 'content/view_embed'.
if (
!$this->getRepository()->canUser( 'content', 'read', $content )
&& !$this->getRepository()->canUser( 'content', 'view_embed', $content )

This comment has been minimized.

Copy link
@lolautruche

lolautruche Feb 28, 2014

Contributor

Use SecurityContext instead.

This comment has been minimized.

Copy link
@pspanja

pspanja Feb 28, 2014

Author Contributor

Updated in 3ece439.

$rendered = $this->fragmentHandler->render(
$controllerReference,
self::RENDERING_STRATEGY,

This comment has been minimized.

Copy link
@lolautruche

lolautruche Feb 28, 2014

Contributor
  1. static instead ?
  2. The rendering strategy should be configurable IMHO

This comment has been minimized.

Copy link
@pspanja

pspanja Feb 28, 2014

Author Contributor

Implemented in ae9d810.

@andrerom

This comment has been minimized.

Copy link
Member

commented Mar 1, 2014

Looks good, but if I remember our discussion around this one missing piece is use of templates for embeds[-inline] tags, or do you plan to do that as followup when handling custom tags?

@pspanja

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2014

Not sure I follow, this implements embedding of the standard view, so it is always rendered through templates with all the usual stuff (matchers, templates, custom controller).

Custom tags would be somewhat similar to how embeds are implemented here. They should be configurable in YAML, would store configuration similar to embeds but could have content of the tag as well. They should be simple, static and not rendered in a sub-request but by the template engine directly.

@andrerom

This comment has been minimized.

Copy link
Member

commented Mar 4, 2014

I was referring to the tag rendering themselves which seems to be done in php here, while I thought we agreed to do it in templates for embed and custom tags.

Like this basically: https://github.com/ezsystems/ezpublish-legacy/tree/master/design/standard/templates/content/datatype/view/ezxmltags
But using render_esi calls in template instead of content_view_gui.

@pspanja

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2014

Ok you're right :) I guess this can be added with custom/template tags.

@andrerom

This comment has been minimized.

Copy link
Member

commented Mar 5, 2014

yes, this can be done later when we do custom tag of course, just wanted to mention it.

+1 for me, ping @lolautruche

@pspanja

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2014

Follow-up for template tags: https://jira.ez.no/browse/EZP-22443

Review ping @lolautruche

*
* @param \DOMDocument $document
*
* @return null

This comment has been minimized.

Copy link
@lolautruche

lolautruche Mar 7, 2014

Contributor

Wrong @return

This comment has been minimized.

Copy link
@pspanja

pspanja Mar 7, 2014

Author Contributor

Fixed in 3231317.

@lolautruche

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2014

+1

pspanja added a commit that referenced this pull request Mar 7, 2014

Merge pull request #758 from ezsystems/implement-EZP-21315-richtext-e…
…mbed

Implement EZP-21315: Implement embed tag for RichText

@pspanja pspanja merged commit 2506007 into master Mar 7, 2014

1 check passed

default The Travis CI build passed
Details

@pspanja pspanja deleted the implement-EZP-21315-richtext-embed branch Mar 7, 2014

@pspanja pspanja referenced this pull request Mar 23, 2014

Merged

Implement EZP-22443: RichText generic template tags #779

3 of 3 tasks complete

pspanja added a commit that referenced this pull request May 15, 2014

pspanja added a commit to ezsystems/ezplatform-xmltext-fieldtype that referenced this pull request Oct 9, 2015

pspanja added a commit to ezsystems/ezplatform-xmltext-fieldtype that referenced this pull request Oct 20, 2015

pspanja added a commit to ezsystems/ezplatform-xmltext-fieldtype that referenced this pull request Oct 20, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.