EZP-31851: Added Twig helpers that abstracts content rendering #115

Nattfarinn · 2020-10-05T15:51:34Z

Question	Answer
JIRA issue	EZP-31851
Type	feature
Target eZ Platform version	`v3.3`
BC breaks	no
Doc needed	yes

Still WIP (Todo section below), but open for brief review as main feature is done.

Based on a Proof of Concept/Experiment: ezsystems/ezpublish-kernel#1518

This PR provides two Twig helper functions and extensibility point for more:

ez_render_content - abstracts Content rendering via different rendering methods (default inline, subrequest, esi)
ez_render_location - abstracts Location rendering via different rendering methods (default inline, subrequest, esi)
ez_render - provides strategy (that comes with abstraction for extensibility) for sub-renderers (like one used directly by ez_render_content or ez_render_location helper)

Usage

Strategy used, default method:

{{ ez_render(content) }}

Direct renderer, default method:

{{ ez_render_content(content) }}

Direct renderer, subrequest method:

{{ ez_render_location(location, { method: "subrequest" }) }}

Todo:

Tests, tests tests...
~~(Optional) Strategy for Location object with ez_render_location as helper.~~

Checklist:

Provided PR description.
Tested the solution manually.
Provided automated test coverage.
Checked that target branch is set correctly (master for features, the oldest supported for bugs).
Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
Asked for a review (ping @ezsystems/php-dev-team).

Nattfarinn · 2020-10-05T16:20:48Z

Update: Investigating Travis issue right now.

eZ/Publish/Core/MVC/Symfony/Templating/RenderStrategy.php

Steveb-p · 2020-10-06T08:09:12Z

eZ/Publish/Core/MVC/Symfony/Templating/RenderContentStrategy.php

+
+        $request = new Request();
+        $request->attributes->add([
+            '_controller' => 'ez_content:viewAction',


This way of referencing controllers is deprecated, double colon :: should be used instead?

Steveb-p · 2020-10-06T08:20:38Z

eZ/Bundle/EzPublishCoreBundle/Resources/config/templating.yml

+    eZ\Publish\Core\MVC\Symfony\Templating\RenderContentStrategy:
+        arguments:
+            $defaultMethod: 'inline'
+            $renderMethods: !tagged_iterator ezplatform.render.method


Wouldn't it be better to use tagged_locator instead, since we will always look for a particular render method anyway (using the key)?

Wouldn't it be better to use tagged_locator instead, since we will always look for a particular render method anyway (using the key)?

Locators are not the best choice when it comes to both strict types && DDD. This is gonna still be a generic objects container with get instead of something specifc. Until PHP has generics / STL support, the code is gonna usually cause a static analysis issue. Judging from this example alone...

I prefer to have an iterable and an array property with "strict" @param \Foo\Bar[] type. Since arrays itself are not strict, there's indeed not much of a difference. The difference is in DDD approach. My class should have no coupling on some ServiceLocator coming from a framework.It's a domain & responsibility mixup and a poor choice by Symfony (IMHO). iterable (or whatever more complex, but built-in type) seems to be cleaner.

eZ/Publish/Core/MVC/Symfony/Templating/Exception/InvalidResponseException.php

eZ/Publish/Core/MVC/Symfony/Templating/RenderMethod/RenderInline.php

eZ/Publish/Core/MVC/Symfony/Templating/Twig/Extension/RenderContentExtension.php

eZ/Publish/Core/MVC/Symfony/Templating/Twig/Extension/RenderExtension.php

alongosz

Seems like the existing tests are also failing.
Besides:

alongosz · 2020-10-06T09:26:30Z

eZ/Bundle/EzPublishCoreBundle/Resources/config/templating.yml

+    eZ\Publish\Core\MVC\Symfony\Templating\RenderContentStrategy:
+        arguments:
+            $defaultMethod: 'inline'
+            $renderMethods: !tagged_iterator ezplatform.render.method


Wouldn't it be better to use tagged_locator instead, since we will always look for a particular render method anyway (using the key)?

Locators are not the best choice when it comes to both strict types && DDD. This is gonna still be a generic objects container with get instead of something specifc. Until PHP has generics / STL support, the code is gonna usually cause a static analysis issue. Judging from this example alone...

I prefer to have an iterable and an array property with "strict" @param \Foo\Bar[] type. Since arrays itself are not strict, there's indeed not much of a difference. The difference is in DDD approach. My class should have no coupling on some ServiceLocator coming from a framework.It's a domain & responsibility mixup and a poor choice by Symfony (IMHO). iterable (or whatever more complex, but built-in type) seems to be cleaner.

eZ/Publish/Core/MVC/Symfony/Templating/BaseRenderStrategy.php

eZ/Bundle/EzPublishCoreBundle/Resources/config/templating.yml

eZ/Publish/Core/MVC/Symfony/Templating/RenderMethod/RenderEdgeSideInclude.php

eZ/Publish/Core/MVC/Symfony/Templating/RenderStrategy.php

eZ/Publish/Core/MVC/Symfony/Templating/Twig/Extension/RenderContentExtension.php

eZ/Publish/Core/MVC/Symfony/Templating/Twig/Extension/RenderExtension.php

eZ/Publish/SPI/MVC/Templating/RenderMethod.php

eZ/Publish/SPI/MVC/Templating/RenderStrategy.php

Nattfarinn · 2020-10-06T11:28:50Z

Travis CI issue fixed.

alongosz · 2020-10-06T13:21:12Z

eZ/Publish/SPI/Options/OptionsBag.php

+{
+    public function all(): array;
+
+    public function get(string $key, $default = null);


Type hints for implicitly mixed param and return would be good here.

Fixed 👍. Obsolete comment.

sonarqubecloud · 2020-10-06T13:51:35Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

* IBX-2823: Added loadRelation method https://issues.ibexa.co/browse/IBX-2823 * Update src/lib/Resources/settings/storage_engines/cache.yml Co-authored-by: Konrad Oboza <konrad.oboza@ez.no> * Update src/lib/Resources/settings/storage_engines/cache.yml Co-authored-by: Konrad Oboza <konrad.oboza@ez.no> * Fixed invalidate tags in removeRelation * Added tests * Update src/lib/Persistence/Legacy/Content/Gateway/DoctrineDatabase.php Co-authored-by: Paweł Niedzielski <pawel.tadeusz.niedzielski@gmail.com> * Update tests/lib/Persistence/Legacy/Content/ContentHandlerTest.php Co-authored-by: Paweł Niedzielski <pawel.tadeusz.niedzielski@gmail.com> * Update tests/lib/Persistence/Legacy/Content/ContentHandlerTest.php Co-authored-by: Paweł Niedzielski <pawel.tadeusz.niedzielski@gmail.com> * Update tests/lib/Persistence/Legacy/Content/Gateway/DoctrineDatabaseTest.php Co-authored-by: Paweł Niedzielski <pawel.tadeusz.niedzielski@gmail.com> * Update src/lib/Persistence/Legacy/Content/Gateway/DoctrineDatabase.php Co-authored-by: Andrew Longosz <alongosz@users.noreply.github.com> * Update tests/lib/Persistence/Legacy/Content/ContentHandlerTest.php Co-authored-by: Andrew Longosz <alongosz@users.noreply.github.com> * Update tests/lib/Persistence/Legacy/Content/Gateway/DoctrineDatabaseTest.php Co-authored-by: Andrew Longosz <alongosz@users.noreply.github.com> * Update src/lib/Persistence/Legacy/Content/Handler.php Co-authored-by: Andrew Longosz <alongosz@users.noreply.github.com> * Added deprecated info when destinationContentId is not passed * Fixed phpdoc for removeRelation method * Used const for relationId * Fixed cs * Update src/lib/Persistence/Cache/ContentHandler.php Co-authored-by: Andrew Longosz <alongosz@users.noreply.github.com> * Update src/lib/Persistence/Legacy/Content/Handler.php Co-authored-by: Andrew Longosz <alongosz@users.noreply.github.com> * Fixes after review * Fixed ContentHandlerTest::testLoadRelation Co-authored-by: Konrad Oboza <konrad.oboza@ez.no> Co-authored-by: Paweł Niedzielski <pawel.tadeusz.niedzielski@gmail.com> Co-authored-by: Andrew Longosz <alongosz@users.noreply.github.com>

Nattfarinn requested review from kaff, adamwojs, bdunogier, mikadamczyk, damianz5, alongosz, ciastektk, webhdx and ViniTou October 5, 2020 15:51

Nattfarinn added Ready for review Work in progress labels Oct 5, 2020

kaff approved these changes Oct 6, 2020

View reviewed changes

eZ/Publish/Core/MVC/Symfony/Templating/RenderStrategy.php Show resolved Hide resolved

Steveb-p reviewed Oct 6, 2020

View reviewed changes

adamwojs reviewed Oct 6, 2020

View reviewed changes

alongosz requested changes Oct 6, 2020

View reviewed changes

Nattfarinn requested review from alongosz, Steveb-p and adamwojs October 6, 2020 11:27

adamwojs changed the title ~~(WIP) EZP-31851: Added Twig helpers that abstracts content rendering~~ EZP-31851: Added Twig helpers that abstracts content rendering Oct 6, 2020

adamwojs approved these changes Oct 6, 2020

View reviewed changes

damianz5 approved these changes Oct 6, 2020

View reviewed changes

alongosz approved these changes Oct 6, 2020

View reviewed changes

Steveb-p approved these changes Oct 6, 2020

View reviewed changes

EZP-31851: Added Twig helpers that abstracts content rendering

587719e

Nattfarinn force-pushed the ezp-31851-twig-helpers branch from a33cafc to 587719e Compare October 6, 2020 14:26

Nattfarinn merged commit 25f4e4a into master Oct 6, 2020

Nattfarinn deleted the ezp-31851-twig-helpers branch October 6, 2020 14:26

ViniTou pushed a commit that referenced this pull request Nov 4, 2020

EZP-31851: Added Twig helpers that abstracts content rendering (#115)

1bb0153

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-31851: Added Twig helpers that abstracts content rendering #115

EZP-31851: Added Twig helpers that abstracts content rendering #115

Nattfarinn commented Oct 5, 2020 •

edited

Loading

Nattfarinn commented Oct 5, 2020

Steveb-p Oct 6, 2020

Nattfarinn Oct 6, 2020

Steveb-p Oct 6, 2020

alongosz Oct 6, 2020

alongosz left a comment

alongosz Oct 6, 2020

Nattfarinn commented Oct 6, 2020

alongosz Oct 6, 2020

Nattfarinn Oct 6, 2020

sonarqubecloud bot commented Oct 6, 2020

EZP-31851: Added Twig helpers that abstracts content rendering #115

EZP-31851: Added Twig helpers that abstracts content rendering #115

Conversation

Nattfarinn commented Oct 5, 2020 • edited Loading

Still WIP (Todo section below), but open for brief review as main feature is done.

Usage

Strategy used, default method:

Direct renderer, default method:

Direct renderer, subrequest method:

Todo:

Checklist:

Nattfarinn commented Oct 5, 2020

Steveb-p Oct 6, 2020

Choose a reason for hiding this comment

Nattfarinn Oct 6, 2020

Choose a reason for hiding this comment

Steveb-p Oct 6, 2020

Choose a reason for hiding this comment

alongosz Oct 6, 2020

Choose a reason for hiding this comment

alongosz left a comment

Choose a reason for hiding this comment

alongosz Oct 6, 2020

Choose a reason for hiding this comment

Nattfarinn commented Oct 6, 2020

alongosz Oct 6, 2020

Choose a reason for hiding this comment

Nattfarinn Oct 6, 2020

Choose a reason for hiding this comment

sonarqubecloud bot commented Oct 6, 2020

Nattfarinn commented Oct 5, 2020 •

edited

Loading