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.

Sign up for GitHub

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

Fix non-existent snippet tags being broken up by an @ tag #16322

Draft
wants to merge 1 commit into
base: 3.x
Choose a base branch
from
Draft

Fix non-existent snippet tags being broken up by an @ tag #16322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
168 changes: 168 additions & 0 deletions _build/test/Tests/Model/modParserTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
namespace MODX\Revolution\Tests\Model;


use MODX\Revolution\modElementPropertySet;
use MODX\Revolution\modPropertySet;
use MODX\Revolution\modSnippet;
use MODX\Revolution\modX;
use MODX\Revolution\MODxTestCase;
use MODX\Revolution\MODxTestHarness;
Expand Down Expand Up @@ -646,7 +649,172 @@ public function providerProcessElementTags() {
'depth' => 0
]
],
// #16318 parsing tags with @ in the value causes it to break the tag
[
[
'processed' => 1,
'content' => "aaa
[[nonExistentSnippet? &x=`bbb@ccc`]]
ddd
eee"
],
"[[+empty_content:empty=`aaa
[[nonExistentSnippet? &x=`bbb@ccc`]]
ddd
`]]eee",
[
'parentTag' => '',
'processUncacheable' => true,
'removeUnprocessed' => false,
'prefix' => '[[',
'suffix' => ']]',
'tokens' => [],
'depth' => 0
]
],
];
}

/**
* @dataProvider providerPropertySetCall
* @param $content
* @param $expected
* @param $propertySet
* @param $params
* @return void
*/
public function testPropertySetCall($content, $expected, $propertySet, $params)
{
/** @var modPropertySet $set */
$set = $this->modx->newObject(modPropertySet::class);
$set->set('name', 'propset_' . bin2hex(random_bytes(4)));
$set->setProperties($propertySet);
self::assertTrue($set->save());

/** @var modSnippet $set */
$snippet = $this->modx->newObject(modSnippet::class);
$snippet->set('name', 'snippet_' . bin2hex(random_bytes(4)));
$snippet->set('content', '<?php
return $scriptProperties["prop"] ?? "";');
self::assertTrue($snippet->save());

$join = $this->modx->newObject(modElementPropertySet::class);
$join->fromArray([
'element' => $snippet->get('id'),
'element_class' => $snippet->_class,
'property_set' => $set->get('id'),
], '', true);
$join->save();
self::assertTrue($join->save());

$content = str_replace('propSetName', $set->get('name'), $content);
$content = str_replace('snippetName', $snippet->get('name'), $content);

$c = $content;

$this->modx->parser->processElementTags(
$params['parentTag'],
$content,
$params['processUncacheable'],
$params['removeUnprocessed'],
$params['prefix'],
$params['suffix'],
$params['tokens'],
$params['depth']
);

$set->remove();
$snippet->remove();
$join->remove();

$this->assertEquals($expected, $content, "Did not get expected results from parsing {$c}.");
}

public function providerPropertySetCall()
{
// In this test, snippetName and propSetName are replaced with a random string
// for each run
return [
[
'[[snippetName? &prop=`123`]]',
'123',
[],
[
'parentTag' => '',
'processUncacheable' => true,
'removeUnprocessed' => false,
'prefix' => '[[',
'suffix' => ']]',
'tokens' => [],
'depth' => 10
]
],
[
'[[snippetName@propSetName]]',
'123',
[
'prop' => '123',
],
[
'parentTag' => '',
'processUncacheable' => true,
'removeUnprocessed' => false,
'prefix' => '[[',
'suffix' => ']]',
'tokens' => [],
'depth' => 10
]
],
[
'[[snippetName@propSetName? &otherProp=`foo`]]',
'789',
[
'prop' => '789',
],
[
'parentTag' => '',
'processUncacheable' => true,
'removeUnprocessed' => false,
'prefix' => '[[',
'suffix' => ']]',
'tokens' => [],
'depth' => 10
]
],
[
'[[snippetName@propSetName? &prop=`123`]]',
'123',
[
'prop' => '456', // needs to be ignored because &prop is specified as override
],
[
'parentTag' => '',
'processUncacheable' => true,
'removeUnprocessed' => false,
'prefix' => '[[',
'suffix' => ']]',
'tokens' => [],
'depth' => 10
]
],
[
'This is a [[snippetName@propSetName:default=`default value`]]',
'This is a default value',
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the test that's failing. The snippet returns the value of the prop property, which doesn't exist in this scenario, so the snippet returns an empty value. That should make it return the default filter, but instead it returns nothing at all (This is a ).

Can reproduce the same behavior when creating things manually on a template/resource, so that suggests something is broken?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a chance this may've been related to #16337, so will need to test again.

[
'otherProp' => 'not this one', // needs to be ignored because &prop is specified as override
],
[
'parentTag' => '',
'processUncacheable' => true,
'removeUnprocessed' => false,
'prefix' => '[[',
'suffix' => ']]',
'tokens' => [],
'depth' => 10
]
],
];

}

/**
Expand Down
21 changes: 8 additions & 13 deletions core/src/Revolution/modElement.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -726,19 +726,14 @@ public function getPropertySet($setName = null)
{
$propertySet = null;
$name = $this->get('name');
if (strpos($name, '@') !== false) {
$psName = '';
$split = xPDO:: escSplit('@', $name);
if ($split && isset($split[1])) {
$name = $split[0];
$psName = $split[1];
$filters = xPDO:: escSplit(':', $setName);
if ($filters && isset($filters[1]) && !empty($filters[1])) {
$psName = $filters[0];
$name .= ':' . $filters[1];
}
$this->set('name', $name);
}

$nameSplit = explode(':', $name);
$tagName = array_shift($nameSplit);
$remainingTag = implode(':', $nameSplit);
if (strpos($tagName, '@') !== false) {
$split = xPDO:: escSplit('@', $tagName);
$psName = $split[1];
$this->set('name', $split[0] . $remainingTag);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is where the issue lies (same goes in the modTag file L520). Here, when modifiers are present, you end up with snippetNamedefault=`default val` instead of snippetName:default=`default val` (note the colon missing in the former) when setting the name on the Element.

Copy link
Collaborator

@smg6511 smg6511 Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple other things:

  • I made my own submission for this (Follow up to 16322: Fix non-existent snippet tags being broken up by an @ tag #16682) only because it was easier to do, particularly because I wanted to make sure whatever I came up with passed on the latest 3.1 dev branch and I've been having issues getting the unit tests to run on my local dev install.
  • I refactored how the two parts of the tag are broken up (using substr approach instead of explode/implode), as it seemed easier to see what was happening.

if (!empty($psName)) {
$psObj = $this->xpdo->getObjectGraph(modPropertySet::class, '{"Elements":{}}', [
'Elements.element' => $this->id,
Expand Down
20 changes: 7 additions & 13 deletions core/src/Revolution/modTag.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,19 +511,13 @@ public function getPropertySet($setName = null)
{
$propertySet = null;
$name = $this->get('name');
if (strpos($name, '@') !== false) {
$psName = '';
$split = xPDO:: escSplit('@', $name);
if ($split && isset($split[1])) {
$name = $split[0];
$psName = $split[1];
$filters = xPDO:: escSplit(':', $setName);
if ($filters && isset($filters[1]) && !empty($filters[1])) {
$psName = $filters[0];
$name .= ':' . $filters[1];
}
$this->set('name', $name);
}
$nameSplit = explode(':', $name);
$tagName = array_shift($nameSplit);
$remainingTag = implode(':', $nameSplit);
if (strpos($tagName, '@') !== false) {
$split = xPDO:: escSplit('@', $tagName);
$psName = $split[1];
$this->set('name', $split[0] . $remainingTag);
if (!empty($psName)) {
$psObj = $this->modx->getObject(modPropertySet::class, ['name' => $psName]);
if ($psObj) {
Expand Down