Improve elements to allow file and inline bindings

[mvoevodskiy]

What does it do?

Added bindings @code, @inline, @file bindings with elements: snippets, chunk. This available for templates and plugins, but it's not callable as "$modx->getChunk() " / "$modx->runSnippet()".
Also, this bindings available in resource content or templates body and other parsible places.

Why is it needed?

This ability do simplier site development because you can not create chunks and snippets in database in all cases.

Also, fixed bug with static elements: now correctly check static file modofication time and if it's more than cache file, cache file will be rewrited.

Small fix PHP notice in modmediasource.class.php.

How works @FILE binding

@FILE binding creates temporary static element (chunk, snippet, template). Then static element works as usual.
You can use 2 system settings:

• static_elements_default_mediasource for choose media source. If not selected, MODX wiil be use default media source ("Filesystem" or other)
• static_elements_basepath for specify elements directory from media source root.

If you leave blank both settongs, specify full path for @FILE binding from site root.
If you fill static_elements_basepath setting, path for @FILE binding should be relative from path in setting.

How works @INLINE for chunks

You can use MODX tags in inline chunks with both: [[ ... ]] or {{ ... }}
{{ ... }} need to support MODX tags in inline chunk when it specified at other chunk or template.

Examples

Files

chunks/example.tpl:

[[++site_name]]
<h2>Hello, [[+modx.user.username]]!</h2>
link to home page: [[~1]]
<p>[[+param1]]</p>
<p>[[+param100]]</p>

snippets/example.php:

<?php
return $modx->user->username . ' Param: ' . $snParam;

PHP

chunks:

$chunk = '@INLINE {{++site_name}} <i>[[++emailsender]]</i> ';
echo $modx->getChunk($chunk));

$chunkName = '@FILE chunks/example.tpl';
echo $modx->getChunk($chunkName, array('param1' => 'value1 :)', 'param100' => 'cache success!!!'));

snippets:

$snippet = '@FILE snippets/example.php';
// echo $modx->runSnippet($snippet, array('snParam' => 'testValue and Cache!!'));

HTML

chunks:

[[$@INLINE {{++site_name}} <i>[[++emailsender]]</i>]]
[[$@FILE chunks/example.tpl ? &param1=`value 1 :)` &param100=`cache success!!!`]]

snippets:

[[@FILE snippets/example.php]]

Related issue(s)/PR(s)

#14201

Thanks to @bezumkin

Thanks to @bezumkin and his pdoTools. A part of logic for creating dynamic elements taken from pdoTools::_loadElement().

[Mark-H]

I'll need to look at this more closely later, but it looks like this would allow syntax like [[!@CODE: runArbitraryPHP(); ]].

In 3.x, the @EVAL binding was removed because it could be used in TVs to run arbitrary code. This proposal seemingly would allow any access to anything editable in MODX to result in arbitrary code execution, making it a potential very disastrous vulnerability.

[mvoevodskiy]

I'll need to look at this more closely later, but it looks like this would allow syntax like [[!@CODE: runArbitraryPHP(); ]].

In 3.x, the @EVAL binding was removed because it could be used in TVs to run arbitrary code. This proposal seemingly would allow any access to anything editable in MODX to result in arbitrary code execution, making it a potential very disastrous vulnerability.

Disabled this. @CODE and @INLINE only for chunks and templates.

[mvoevodskiy]

Updated description.

[JoshuaLuckers]

There is a typo (temlplates) in this path: core/elements/temlplates/.gitignore

[mvoevodskiy]

There is a typo (temlplates) in this path: core/elements/temlplates/.gitignore

Done.

[sdrenth]

@mvoevodskiy Thank you for putting in the great work! I think your PR should be rebased because of the big PR that has been merged earlier where all classes where refactored which is causing merge conflicts.

Would be great to see this PR merged 👍

[sdrenth]

@mvoevodskiy Any thoughts? Would be great to see this PR merged :)

[Ibochkarev]

@opengeek @Mark-H If Michael does a rebase of this PR, will he be accepted or in doubt?

[Mark-H]

Well, I can guarantee that it wont get accepted it if it is not rebased ;)

I personally am not 100% sold, but not 100% against either. Probably leaning slightly in favour, but not enough yet to smash that green button. This is one of those things that I don't use often myself, so more user feedback and testers indicating it works the way they'd want it to work would need to bump my confidence in this PR.

IMO, this is the type of thing a MAB decision would be useful on to determine consensus, just to make sure it's well considered and people have had a chance to poke holes (so they can be plugged) before it goes to implementation.

Code-wise it looks okay. core/elements/ would need to be added into _build/build.xml for it to be included in the transport, some loose type checks could be turned into a strict type check, and we can use short array syntax in 3.x, but I see no immediate blocking issues from a code point of view. Tests would be good.

Seems closely related to the automatic static elements introduced in 2.7 which had the same problem of neither Jason nor myself having much plans of using it. :P

[sdrenth]

@Mark-H What's not to like about this pull request? We talked with all website makers and this is what everybody needs in their workflow. I think it would be a great improvement enabling MODX developers to work file based.

[Mark-H]

You've talked with all the site builders, and everybody wants this in their workflow, really? I know Sterc is all about this sort of things in your workflows, but please don't extrapolate that to the entire community. The fact that I personally don't expect to use this makes your bold claim false ;)

I'm not saying that I don't like this PR. All PRs are awesome because they're an opportunity to make MODX better. This one, too.

When it comes to new features, I consider it my responsibility as integrator to think ahead and try to determine if it's something that fits MODX strategically. Is a proposal something that will still benefit MODX in 5 years or will we regret having added it (and committing to maintaining it) in the same way everyone hates on ExtJS? What may be unintended side effects, or potential maintenance issues in the future? Is it really something that should be in core, or is it more of a responsibility for extras? Is it the right way to solve a certain problem/use case or can we do better?

I suggested it would be a good idea to get wider community consensus on (e.g. MAB) to make a fair decision on wether it's something we want to do and commit to or not. I'll happily put such a strategic decision over my personal preferences. For many PRs I'm comfortable making a decision myself, but for this one I'm unsure as it's just not a part of my typical workflow, unlike other people.

Just to state this (again), that does not mean I'm against this PR, it means I'm undecided. That's a very big difference.

Often if I'm undecided (and low on the SSOGAF), I just let a PR sit until others discuss it and reach a consensus, but in this case @Ibochkarev specifically asked for my thoughts, and the PR has been open for a while without much discussion about the feature so I figured I'd at least try to address the question for as much as I have made up my mind about it.

[rthrash]

This pull request has been mentioned on MODX Community. There might be relevant details there:

https://community.modx.com/t/allowing-file-and-inline-bindings-for-chunks-and-snippets-14490/1496/1

[Mark-H]

I've opened a topic in the MAB board: https://community.modx.com/t/allowing-file-and-inline-bindings-for-chunks-and-snippets-14490/1496.. and apparently @rthrash is a bot now that automatically cross-posts, that's cool :P

[BobRay]

NewsPublisher uses {} tags in the Resource content field to allow it to work with allow_tags_in_post set to false. Standard tags are converted to ]] on load and {{ tags are converted to ]] on save. I don't think the PR creates a problem for NP, but I'm not sure.

[JoshuaLuckers]

To make a well-argued decision I would like to gather some more pro's and con's.

Why is it needed?

This ability do simplier site development because you can not create chunks and snippets in database in all cases.

If you can not create chunks and snippets why would it be good to provide method(s) to bypass this?

We talked with all website makers and this is what everybody needs in their workflow. I think it would be a great improvement enabling MODX developers to work file based.

How does this file based workflow look like? What are other possible solutions for this problem?

In recent projects it would have made my life as a developer easier if I could have used @INLINE for chunks.

I'm not in favor of the @FILE binding since I'm not totally convinced we will not shoot ourself in the foot (security wise and user/developer friendliness).

[gpsietzema]

I thought about this and try to add some of our thoughts to this.

To make a well-argued decision I would like to gather some more pro's and con's.

Why is it needed?

This ability do simplier site development because you can not create chunks and snippets in database in all cases.

If you can not create chunks and snippets why would it be good to provide method(s) to bypass this?

The reason to bypass it is from a performance-standpoint. So at the start we used Static Elements, so we were finally able to work in our favorite IDE (phpstorm). Then we came to the conclusion that pdoTools/Fenom had a way of using static elements by bypassing the database, which made it perform better. We did some serious benchmarking on this for large websites and pdoTools/Fenom was way faster. So it's not like we're killing chunks/templates/snippets, it's just that they are not in the elements-tree in MODX in our projects, but in an elements folder in the MODX-filesystem.

We talked with all website makers and this is what everybody needs in their workflow. I think it would be a great improvement enabling MODX developers to work file based.

How does this file based workflow look like? What are other possible solutions for this problem?

Exactly the same as you would do with the current Static Elements in MODX, but just without the database entries which reference to your actual static files.

In recent projects it would have made my life as a developer easier if I could have used @INLINE for chunks.

I'm not in favor of the @FILE binding since I'm not totally convinced we will not shoot ourself in the foot (security wise and user/developer friendliness).

It's a tough call. I'm not entirely sure about where to draw the line. If we protect our users from using this, because we're afraid they will get hacked... I dunno. Can we allow people to make custom snippets? Because they could evaluate user-generated-input there as well. We could disable it by default and have big ass warning message popup to never include user generated content in there. Or we can have the @file to only include text and not other MODX placeholders or code...

[JoshuaLuckers]

Thanks @gpsietzema for sharing hour thoughts!
I would like to see this PR rebased and to have some tests include inside modParserTest.

[Mark-H]

This is now past the feature/breaking change cut-off for alpha1 so pushing further. Shame there's not been discussion from the other MAB members about it.

[CrazyBoy49z]

This is great PR for working with Git. This is in the kernel of pdotools that many use for phpstorm and Git

[Ibochkarev]

Guys, are there any progress on this PR? Can we add this improvement?