Fix fatal error in trim output filter #16757
Conversation
|
@jenswittmann does that happen outside of pdotools? |
opengeek
changed the title
Yes, you can try this with a Snippet that return an Array: When you use |
|
@smg6511 here's a real-world example that might not work with the discussed fix maybe 🤔 Check if the user has a name in a custom session variable and output a link based on it: Every output modifier gets trimmed in line 66. So, with return, you always get the else value, right? |
|
@jenswittmann - No, I did some tests with |
|
@jenswittmann Hi again, Jens. SInce you were active earlier today (on the file browser update I did), I thought I'd see if you wanted to take a second look and consider implementing the changes I'd suggested for your PR here. Then I can re-review and maybe we can get this one merged in soon as well ;-) |
Co-authored-by: Jim Graham <jim@pixelsandstrings.com>
|
@smg6511 can confirm that both suggestion fixed it! |
There was a problem hiding this comment.
Unfortunately, this breaks three of the unit tests with parsing and cannot be accepted as is.
There were 3 failures:
1) MODX\Revolution\Tests\Model\modParserTest::testProcessElementTags with data set #27 (array(1, 'Doesn't exist'), '[[!+invalid_tag:notempty=`<p>...ist`]]', array('', true, false, '[[', ']]', array(), 0))
Did not get expected results from parsing [[!+invalid_tag:notempty=`<p>[[!+[[!+invalid_tag]]]]</p>`:default=`Doesn't exist`]].
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
Array (
- 'processed' => 1
- 'content' => 'Doesn't exist'
+ 'processed' => 0
+ 'content' => '[[!+invalid_tag:notempty=`<p>[[!+[[!+invalid_tag]]]]</p>`:default=`Doesn't exist`]]'
)
/home/runner/work/revolution/revolution/_build/test/Tests/Model/modParserTest.php:137
2) MODX\Revolution\Tests\Model\modParserTest::testProcessElementTags with data set #28 (array(1, 'Tag2'), '[[!+invalid_tag:notempty=`<p>...]]]`]]', array('', true, false, '[[', ']]', array(), 0))
Did not get expected results from parsing [[!+invalid_tag:notempty=`<p>[[!+tag[[+is1]]]]</p>`:default=`[[!+tag[[+is2]]]]`]].
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
Array (
'processed' => 1
- 'content' => 'Tag2'
+ 'content' => '[[!+invalid_tag:notempty=`<p>Tag1</p>`:default=`Tag2`]]'
)
/home/runner/work/revolution/revolution/_build/test/Tests/Model/modParserTest.php:137
3) MODX\Revolution\Tests\Model\modParserTest::testDefaultNonExistingTvValue
Did not parse non-existing TV with default modifier correctly
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'[[*foo:default=`bar`]]'
+'bar'
/home/runner/work/revolution/revolution/_build/test/Tests/Model/modParserTest.php:927
--
| if (!is_scalar($element->_output)) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
| if (!is_scalar($element->_output)) { | |
| return; | |
| } |
Ok, I see what we need to do. The short story is that we need null values to be preserved in certain instances, which is why limiting to scalar vals breaks some of the tests (I didn't go deep enough down the rabbit hole to discover exactly why this is). So we need to kill this new conditional.
There was a problem hiding this comment.
To still catch incompatible values passed to the modifiers, the way to do it would be something like the following, inserted right before the for loop (this passes the unit tests):
$commandName = $element->get('name');
try {
// Check for invalid data types
if (is_array($output)) {
throw new \Exception("An element ($commandName) in document #{#} is attempting to pass its value(s) to an output modifier, but its data type (array) is not compatible. Only string, number, boolean, and null types may be passed.");
}
} catch (\Exception $e) {
$this->modx->log(modX::LOG_LEVEL_ERROR, $e->getMessage());
return;
}There are a couple to-dos with this block of code if we do it this way:
- Get the reference data for the error (maybe warning) message (the doc id at minimum [how is this most easily done?])*
- Convert this to a lexicon string with a couple placeholders instead of hard-coding the message.
- @opengeek (or anyone else looking who can provide the answer) - Is there an easy way to get the original full tag being worked with (e.g.,
[[+something:is=X:then=Y:else=Z]])? I see how the deconstructed parts are gotten, but including at least a part of the full tag in the error message would help folks to find buggy tags.
There was a problem hiding this comment.
@jenswittmann - Hey, even though you reverted the scalar conditional we still need to implement further handling as noted above to fully avoid fatal errors (at least when an array is the subject of a modifier). Keeping this thread unresolved will keep it more visible (I'm hoping Jason will chime in on my questions here).
Just FYI, as it stands now we avoid the fatal error so long as no string functions are called as modifiers (on an array subject). But if you try something like [[demo:ucfirst]] (referring to your earlier test example) where the output of subject demo is an array, the error will occur.
Co-authored-by: Jim Graham <jim@pixelsandstrings.com>
4 participants
This solves a PHP fatal error that occurs when using something like this:
[[!#SESSION.user.password:trim]]PHP Error
System