modxcms / revolution

MODX Revolution - Content Management Framework
https://modx.com/
GNU General Public License v2.0
1.36k stars 529 forks source link

allowed TV names #12948

Closed Skippidar closed 6 years ago

Skippidar commented 8 years ago

Summary

MODX allows any characters in TV names, but some applications work not correct when TV name has any symbols like space or dot.

Step to reproduce & Observed behavior

For example - pdoResource and sortby parameter &sortbyTV=price &sortbyTVType=integer - sorts prices as integers &sortbyTV=product.price &sortbyTVType=integer - sorts prices as string

Expected behavior

I think there should be any restrictions on TV names to avoid that problem.

Environment

MODX Revolution 2.4.3, PHP 5.3.29

OptimusCrime commented 8 years ago

While you are correct in that there should not be any limitations on TV names, this does collide with other functionality that the MODX parser has.

For example, consider the following variable [[+foo]]. If you apply a dot there, following any other content, like [[+foo.bar]], that actually means that foo is a array, and bar is a key within that array. This is commonly done in chunks with the getChunk method.

If dots should be allowed in a name that would require rewrite of the parser and potentially huge breaking changes.

I'd go for the "wontfix" as this is by design. You should instead go for a name that does not contain a dot (e.g. underscore).

sottwell commented 8 years ago

What the OP is saying is that there should be limitations on what can be used as a TV name, since using a dot will cause a problem.

Skippidar commented 8 years ago

Yes, i know that that's parser's feature, but let's imagine: I'm newbie, I've just installed modx and I don't know how parser works, what i can\can't type, etc. And I want to create a TV named 'product.my new tv'. I press "Save" and it saves successfully, but in any time I'll find out that in some cases it causes problems, but i already have big project and changing TV names in all chunks\sippets\etc would be a big prolem.

I just offer to write "Please, don't use _not_allowedcharacters in TV name " on saving TV with incorrect name.

jaygilmore commented 8 years ago

@opengeek is it his actually intended—that the parser considers [[+foo.bar]] as array[key] or is this something assumed by people using getChunk or others?

@Skippidar If this is intended behavior, the UX issue it creates is in having undisclosed/hidden field limitations the user doesn't discover until they've already done the wrong thing. Inputs should never assume the user knows what can and can't go into the field. This is the same issue I have with illegal characters in passwords in Revo right now which are not disclosed anywhere in setup or the User UI.

These may seem like minor annoyances but we should be designing so the user knows what can go in an input and only if accidental invalid entry provide an alert/error message. We should not present a title field in a specific context (TV create) and then only in that context throw an error after the user puts in the wrong thing, in this case a title with a ..

opengeek commented 8 years ago

Not sure how this discussion got so off topic. The real issue here is that some extras can't handle TV names with a . in them? That sounds like a bug in that extra to me. And it is common usage to transform arrays into flat keys for placeholders using ., but that is irrelevant to TV naming—not even sure why that was brought up here.

Skippidar commented 8 years ago

@opengeek but some snippets (like getResources\pdoResources) use placeholders with TV names in tpls - and it causes unobvious problems sometimes. So i think the best solution - just restrict TV names

opengeek commented 8 years ago

The only example you gave was a problem with pdoResources not sorting something properly by TV name. Are there more examples? This seems like a pretty specific issue to pdoResources, but please correct me if I'm missing other instances of this problem.

Skippidar commented 8 years ago

I cant remember any other examples, but will it be ok if I name TV "mytv.value" and then use smth like $where=array("modTemplateVar.mytv.value.value:=":"value") in snippet? (I dont know)

OptimusCrime commented 8 years ago

So, is this a MODX Core bug, Extra bug or intended?

bezumkin commented 8 years ago

@opengeek It seems, that bug is not in pdoTools.

Example:

$c = $modx->newQuery('modResource');
$c->select($modx->getSelectColumns('modResource'));
$c->select('IFNULL(`TVtv.pt.event.city`.`value`, "") AS `tv.tv.pt.event.city`');
$c->leftJoin('modTemplateVarResource', 'TVtv.pt.event.city', '`TVtv.pt.event.city`.`contentid` = `modResource`.`id` AND `TVtv.pt.event.city`.`tmplvarid` = 4');
$c->where(array(
    'TVtv.pt.event.city' => 'London',
    'modResource.published' => 1,
    'modResource.deleted' => 0,
));
$c->sortby('modResource.publishedon', 'DESC');
$c->limit(10);
$c->prepare(); echo $c->toSQL();

Result:

SELECT `id`, `type`, `contentType`, `pagetitle`, `longtitle`, `description`, `alias`, `link_attributes`, `published`, `pub_date`, `unpub_date`, `parent`, `isfolder`, `introtext`, `content`, `richtext`, `template`, `menuindex`, `searchable`, `cacheable`, `createdby`, `createdon`, `editedby`, `editedon`, `deleted`, `deletedon`, `deletedby`, `publishedon`, `publishedby`, `menutitle`, `donthit`, `privateweb`, `privatemgr`, `content_dispo`, `hidemenu`, `class_key`, `context_key`, `content_type`, `uri`, `uri_override`, `hide_children_in_tree`, `show_in_tree`, `properties`, IFNULL(`TVtv.pt.event.city`.`value`, "") AS `tv.tv.pt.event.city`
FROM `mysql_site_content` AS `modResource`
LEFT JOIN `mysql_site_tmplvar_contentvalues` `TVtv.pt.event.city` ON `TVtv.pt.event.city`.`contentid` = `modResource`.`id` AND `TVtv.pt.event.city`.`tmplvarid` = 4
WHERE ( `TVtv`.`pt` = 'London' AND `modResource`.`published` = 1 AND `modResource`.`deleted` = 0 )
ORDER BY modResource.publishedon DESC
LIMIT 10

Where function changes

'TVtv.pt.event.city' => 'London',

to

`TVtv`.`pt` = 'London'

MODX 2.5.0-pl

labr1005 commented 7 years ago

@opengeek "The only example you gave was a problem with pdoResources not sorting something properly by TV name. Are there more examples?"

Collections has this problem, too:

https://docs.modx.com/extras/revo/collections

Search for "TV name must NOT contain a dot".

opengeek commented 7 years ago

Then fix collections and pdoResources.

labr1005 commented 7 years ago

Sorry, should have read the whole conversation.

sottwell commented 7 years ago

But is this a problem with the extra, or with xPDO when it does something like this...

$c->where(array(
    'TVtv.pt.event.city' => 'London',
    'modResource.published' => 1,
    'modResource.deleted' => 0,
));

Results in this query

WHERE ( `TVtv`.`pt` = 'London' AND `modResource`.`published` = 1 AND `modResource`.`deleted` = 0 )

Why is the WHERE condition getting truncated when generating the query?

opengeek commented 7 years ago

xPDO detects dots in aliases and automatically escapes them. Do not use dots in your alias. It thinks it is a table.column_name.

sottwell commented 7 years ago

So instead of passing the actual TV name, it should be creating an alias for it...

opengeek commented 7 years ago

Right. To solve the issue, you can transform those when generating the alias for the statement.

sottwell commented 7 years ago

Interesting. Good to know.

OptimusCrime commented 7 years ago

@opengeek Can we close this as an intentional implementation?

opengeek commented 7 years ago

Hang tight — don't close it just yet. I am considering an idea to avoid this moving forward and will close this when I propose the solution. But I still think existing extras that are running into this problem could easily solve it by simply transforming dots to something else in the alias they are creating.

wuuti commented 7 years ago

@opengeek Are you still on this or is this now a close candidate? #modxbughunt

sonicpunk commented 7 years ago

modxbughunt #1point to @wuuti

OptimusCrime commented 7 years ago

@opengeek We're all wondering about this.

opengeek commented 7 years ago

I remember I had an idea on how to solve this if only I could recall what that idea was. If I don't get back to this within 48 hours, close this as intentional implementation.

OptimusCrime commented 6 years ago

@modxbot close