Added new View Decorators. #5567

lonnieezell · 2022-01-13T06:36:07Z

Description
Created View Decorators that allow you to modify HTML generated from the View or Parser classes just prior to it being cached.

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

iRedds

This proposal defines global decorators, which makes it impossible to generate a view without decorators or selectively apply them.

iRedds · 2022-01-13T07:29:40Z

system/View/Traits/DecoratesViews.php

+ if (empty($decorators)) {
+ return $html;
+ }


The decorators property is an array. An empty array will not be processed by foreach.
It seems to me there is no point in this check.

Using empty() is conceptually expandable to if (! isset(config('View')->decorators) || config('View')->decorators) === []) - in other words, it covers backwards-compatibility for projects that don't add the $decorators property to their App Config file. Alternatively we could (should?) add $decorators to CodeIgniter\Config\View in which case then I would agree we can drop this check.

I know @lonnieezell has been opposed to duplicating Config values between system/ and app/, but given that the View config class already exists in system/ I suspect this is the way to go.

I did add it the system config file. Well I've corrected that since I put it in the wrong file last night. Silly me :) Will remove.

iRedds · 2022-01-13T08:08:40Z

system/View/Interfaces/ViewDecorator.php

+ * chance to modify the output from the view() calls
+ * prior to it being cached.
+ */
+interface ViewDecorator


I think that the name of the interface should contain the suffix Interface.

I think the current name makes sense in the subdirectory Interfaces, but none of our other libraries have a dedicated Interfaces namespace so I would be in favor of moving this file up a directory and renaming it as @iRedds suggests.

Agree - will make it match the rest of them. I was even the one who initially argued the point that they should be named that way.

iRedds · 2022-01-13T08:10:25Z

system/View/Traits/DecoratesViews.php

+ foreach ($decorators as $className) {
+ $decorator = new $className();
+
+ if (! $decorator instanceof ViewDecorator) {
+ throw ViewException::forInvalidDecorator($className);
+ }
+
+ $html = $decorator->decorate($html);


This code can be optimized.

Suggested change

foreach ($decorators as $className) {

$decorator = new $className();

if (! $decorator instanceof ViewDecorator) {

throw ViewException::forInvalidDecorator($className);

}

$html = $decorator->decorate($html);

foreach ($decorators as $decorator)

if (! is_subclass_of($decorator, ViewDecorator::class) {

throw ViewException::forInvalidDecorator($decorator);

}

$html = (new $decorator())->decorate($html);

If we go with the static method suggestion above this can be simplified even more:

Suggested change

foreach ($decorators as $className) {

$decorator = new $className();

if (! $decorator instanceof ViewDecorator) {

throw ViewException::forInvalidDecorator($className);

}

$html = $decorator->decorate($html);

foreach ($decorators as $className) {

if (! is_subclass_of($decorator, ViewDecorator::class) {

throw ViewException::forInvalidDecorator($className);

}

$html = $decorator::decorate($html);

I like the static method suggestion here. Will change and simplify.

iRedds · 2022-01-13T08:32:18Z

Can you give a real life example of using view decorators?

MGatner

I'm super excited for this! Lots of comments, and also some CI fixes that CS Fixer/Rector ought tot take care of.

MGatner · 2022-01-13T14:04:08Z

system/View/Traits/DecoratesViews.php

+ if (empty($decorators)) {
+ return $html;
+ }


Using empty() is conceptually expandable to if (! isset(config('View')->decorators) || config('View')->decorators) === []) - in other words, it covers backwards-compatibility for projects that don't add the $decorators property to their App Config file. Alternatively we could (should?) add $decorators to CodeIgniter\Config\View in which case then I would agree we can drop this check.

I know @lonnieezell has been opposed to duplicating Config values between system/ and app/, but given that the View config class already exists in system/ I suspect this is the way to go.

MGatner · 2022-01-13T14:06:31Z

system/View/Interfaces/ViewDecorator.php

+ * chance to modify the output from the view() calls
+ * prior to it being cached.
+ */
+interface ViewDecorator


I think the current name makes sense in the subdirectory Interfaces, but none of our other libraries have a dedicated Interfaces namespace so I would be in favor of moving this file up a directory and renaming it as @iRedds suggests.

system/View/Interfaces/ViewDecorator.php

app/Config/View.php

MGatner · 2022-01-13T14:15:34Z

system/Language/en/View.php

@@ -17,4 +17,5 @@
 'noCellClass' => 'No view cell class provided.',
 'invalidCellClass' => 'Unable to locate view cell class: {0}.',
 'tagSyntaxError' => 'You have a syntax error in your Parser tags: {0}',
+ 'invalidDecoratorClass' => '{0} is not a valid ViewDecorator.',


If we go with the rename I think this would read better without the Interface suffix so I suggest making it natural:

Suggested change

'invalidDecoratorClass' => '{0} is not a valid ViewDecorator.',

'invalidDecoratorClass' => '{0} is not a valid View Decorator.',

system/View/Parser.php

system/View/Traits/DecoratesViews.php

MGatner · 2022-01-13T14:28:40Z

system/View/Traits/DecoratesViews.php

+ foreach ($decorators as $className) {
+ $decorator = new $className();
+
+ if (! $decorator instanceof ViewDecorator) {
+ throw ViewException::forInvalidDecorator($className);
+ }
+
+ $html = $decorator->decorate($html);


If we go with the static method suggestion above this can be simplified even more:

Suggested change

foreach ($decorators as $className) {

$decorator = new $className();

if (! $decorator instanceof ViewDecorator) {

throw ViewException::forInvalidDecorator($className);

}

$html = $decorator->decorate($html);

foreach ($decorators as $className) {

if (! is_subclass_of($decorator, ViewDecorator::class) {

throw ViewException::forInvalidDecorator($className);

}

$html = $decorator::decorate($html);

system/View/View.php

MGatner · 2022-01-13T14:41:35Z

Can you give a real life example of using view decorators?

@iRedds I'm excited for this because it takes some pressure off of Filters while still leveraging View Caching. Currently the only "middleware" solution we have for modifying the Response is to use a Filter with the after() method. IMO this puts too much pressure on Filters plus, as I mentioned, it means that your Filters run on the cached View content every time unnecessarily.

Most of my "HTML injection" libraries use the Filters method, here is an example that would be replaced by a single Decorator: https://github.com/tattersoftware/codeigniter4-alerts/blob/96cd7fa922cad556fb20f015d746f38fb85c748f/src/Filters/AlertsFilter.php

One other "plus" of Decorators is they are a lot easier to configure and apply! For a third-party library to activate its Filter it needs to use one of these hack-y FIlter configs:

https://github.com/tattersoftware/codeigniter4-alerts/blob/96cd7fa922cad556fb20f015d746f38fb85c748f/src/Config/Filters.php

... and then the implementing project still needs to add it to the desired routes in app/Config/Filters.php:

    public $globals = [
        'after'  => [
            'alerts' => '*',
        ],
    ];

MGatner · 2022-01-13T14:51:14Z

@lonnieezell what about some built-in tools or core handling for non-HTML output? Is there any scenario where we need to be "header-aware" (like disabling Decorators for JSON content), or is that up to the developer?

For example, I add this to all my HTML-injection Filters:

https://github.com/tattersoftware/codeigniter4-alerts/blob/develop/src/Filters/AlertsFilter.php#L55-L68

MGatner · 2022-01-13T15:12:59Z

Thinking about this more my hunch is that View doesn't care at all about formatting or headers and it would be on the developer to know that passing content through will subject it to Decorators. I suspect most APIs dealing with JSON/XML aren't using View for the content anyway.

Probably worth mentioning in the User Guide either way.

lonnieezell · 2022-01-13T16:44:17Z

Can you give a real life example of using view decorators?

Two of them:

I've implemented View Components in Bonfire, but they require on overriding the View class to work. Essentially they look for custom html tags and replace them with other content.

It could also be used for a "snippet" functionality that kept small bits of text/html and will replace placeholders with it globally. I've seen this on some CMS' before.

lonnieezell · 2022-01-13T16:46:02Z

what about some built-in tools or core handling for non-HTML output? Is there any scenario where we need to be "header-aware" (like disabling Decorators for JSON content), or is that up to the developer?

I agree with your assessment that View doesn't care here.

MGatner

I assume you are still looking at some of the others. One typing change here.

Also, is there a case to be made for View::enableDecorators()/View::disableDecorators()?

system/Config/View.php

lonnieezell · 2022-01-14T04:00:35Z

Oh - @MGatner this should target 4.2, also, shouldn't it?

lonnieezell · 2022-01-14T04:17:52Z

I think everything is accounted for now, though I had to skip the pre-commit checks since there are two PHPStan errors for me locally with the latest develop in text_helper and the Autoloader.

lonnieezell · 2022-01-14T04:50:33Z

Also, is there a case to be made for View::enableDecorators()/View::disableDecorators()?

My initial instinct is not really, but both you and IReds brought something similar up so maybe. :)

kenjis · 2022-01-14T05:16:31Z

php-cs-fixer check failed, but it seems php-cs-fixer's phpdoc_types_order has a bug.

   1) CodeIgniter4/tests/system/Cookie/CookieStoreTest.php (phpdoc_types_order)
      ---------- begin diff ----------
--- /home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Cookie/CookieStoreTest.php
+++ /home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Cookie/CookieStoreTest.php
@@ -107,7 +107,7 @@
         $prod = $cookies['prod']->getOptions();
 
         /**
-         * @var MockObject&CookieStore $store
+         * @var CookieStore|MockObject $store
          */
         $store = $this->getMockBuilder(CookieStore::class)
             ->setConstructorArgs([$cookies])

https://github.com/codeigniter4/CodeIgniter4/runs/4812733176?check_suite_focus=true

MGatner · 2022-01-14T12:07:04Z

Oh - @MGatner this should target 4.2, also, shouldn't it?

We merge 4.2 into develop! We'll see how that goes 😅

MGatner

A few wording changes. And since we are >= 7.4 now: typed properties!

system/View/ViewDecoratorTrait.php

user_guide_src/source/outgoing/view_decorators.rst

app/Config/View.php

system/Config/View.php

Co-authored-by: MGatner <mgatner@icloud.com>

MGatner

Looks good to me! See if there are any workflow issues remaining.

…4 into view-decorators

MGatner · 2022-01-15T13:00:58Z

🥳

Added new View Decorators.

06f6cd6

lonnieezell requested a review from MGatner January 13, 2022 06:36

iRedds reviewed Jan 13, 2022

View reviewed changes

kenjis added the enhancement PRs that improve existing functionalities label Jan 13, 2022

MGatner requested changes Jan 13, 2022

View reviewed changes

Apply review suggestions

8777a9d

MGatner requested changes Jan 13, 2022

View reviewed changes

system/Config/View.php Outdated Show resolved Hide resolved

lonnieezell added 2 commits January 13, 2022 22:15

Merge branch 'develop' into view-decorators

2657ce8

Additional review comments

dc36185

Rector fixes

7b1149b

MGatner requested changes Jan 14, 2022

View reviewed changes

lonnieezell and others added 6 commits January 14, 2022 11:52

Update system/View/ViewDecoratorTrait.php

161ba77

Co-authored-by: MGatner <mgatner@icloud.com>

Update user_guide_src/source/outgoing/view_decorators.rst

d06689f

Co-authored-by: MGatner <mgatner@icloud.com>

Update user_guide_src/source/outgoing/view_decorators.rst

c71169c

Co-authored-by: MGatner <mgatner@icloud.com>

Update user_guide_src/source/outgoing/view_decorators.rst

0f455a3

Co-authored-by: MGatner <mgatner@icloud.com>

Update app/Config/View.php

5e9e425

Co-authored-by: MGatner <mgatner@icloud.com>

Update system/Config/View.php

593e6ad

Co-authored-by: MGatner <mgatner@icloud.com>

MGatner approved these changes Jan 14, 2022

View reviewed changes

Added to changelog

13115b9

Merge branch 'view-decorators' of github.com:codeigniter4/CodeIgniter…

d299786

…4 into view-decorators

lonnieezell merged commit 6f46c35 into develop Jan 15, 2022

kenjis deleted the view-decorators branch January 15, 2022 06:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added new View Decorators. #5567

Added new View Decorators. #5567

lonnieezell commented Jan 13, 2022

iRedds left a comment

iRedds Jan 13, 2022

MGatner Jan 13, 2022

lonnieezell Jan 13, 2022

iRedds Jan 13, 2022

MGatner Jan 13, 2022

lonnieezell Jan 13, 2022

iRedds Jan 13, 2022

MGatner Jan 13, 2022

lonnieezell Jan 13, 2022

iRedds commented Jan 13, 2022

MGatner left a comment

MGatner Jan 13, 2022

MGatner Jan 13, 2022

MGatner Jan 13, 2022

MGatner Jan 13, 2022

MGatner commented Jan 13, 2022 •

edited

Loading

MGatner commented Jan 13, 2022

MGatner commented Jan 13, 2022

lonnieezell commented Jan 13, 2022

lonnieezell commented Jan 13, 2022

MGatner left a comment

lonnieezell commented Jan 14, 2022

lonnieezell commented Jan 14, 2022

lonnieezell commented Jan 14, 2022

kenjis commented Jan 14, 2022

MGatner commented Jan 14, 2022

MGatner left a comment

MGatner left a comment

MGatner commented Jan 15, 2022

	'invalidDecoratorClass' => '{0} is not a valid ViewDecorator.',
	'invalidDecoratorClass' => '{0} is not a valid View Decorator.',

Added new View Decorators. #5567

Added new View Decorators. #5567

Conversation

lonnieezell commented Jan 13, 2022

iRedds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iRedds commented Jan 13, 2022

MGatner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MGatner commented Jan 13, 2022 • edited Loading

MGatner commented Jan 13, 2022

MGatner commented Jan 13, 2022

lonnieezell commented Jan 13, 2022

lonnieezell commented Jan 13, 2022

MGatner left a comment

Choose a reason for hiding this comment

lonnieezell commented Jan 14, 2022

lonnieezell commented Jan 14, 2022

lonnieezell commented Jan 14, 2022

kenjis commented Jan 14, 2022

MGatner commented Jan 14, 2022

MGatner left a comment

Choose a reason for hiding this comment

MGatner left a comment

Choose a reason for hiding this comment

MGatner commented Jan 15, 2022

MGatner commented Jan 13, 2022 •

edited

Loading