search

modxcms / revolution

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

Issue: Defined placeholders have to be uncached in order to use output modifiers in 2.4.4 #12970

Closed matdave closed 6 years ago

matdave commented 8 years ago

Summary

When you have a defined placeholder using something like $modx->setPlaceholder(s) you can't use an output modifier unless calling the placeholder uncached

Step to reproduce

Snippet: placeholderError

<?php $modx->setPlaceholder('testPlaceholder',$modx->resource->get('alias'));

In the page template I've called 

[[!placeholderError]]
[[+testPlaceholder]]
[[+testPlaceholder:ne=`about`:then=`test`:else=`not test`]]

On the "about" page I get image

but if I call it 

[[!placeholderError]]
[[+testPlaceholder]]
[[!+testPlaceholder:ne=`about`:then=`test`:else=`not test`]]

On the "about" page I get image

Environment

MODX 2.4.4

opengeek commented 8 years ago

@yoleg is this a new side effect of the changes made to address #12906 ?

yoleg commented 8 years ago

@opengeek Yes, it seems likely. I will look at it as soon as I can, hopefully tomorrow.

By the way, does anyone know who wrote the original modparser? It would save me some time if I could ask them about the reasons behind some of the code.

sottwell commented 8 years ago

The Slack community is a good place for such questions. All of the developers are active there. https://modx.org

opengeek commented 8 years ago

I wrote it @yoleg — though some of the reasoning I may have lost over time... ;)

yoleg commented 8 years ago

@matdave @opengeek I could not reproduce the issue on two different MODX installs, including one fresh install with all default configuration.

Are you able to reproduce it on a freshly-installed MODX? If yes, can you PM me an access to the manager so I can take a look? If not, can you narrow down which setting or extras cause the problem? Particularly, make sure you don't have pdoTools or any other parser modifier installed.

Even if this issue does occur, I believe these placeholders should be called uncached, since they are generated by an uncached snippet and are not even defined when they are first processed. This is because MODX processes all of the cached elements before any of the uncached elements.

So, on the first parser pass, only cached elements are processed. The [[!placeholderError]] snippet is skipped, and the cached testPlaceholder placeholders are processed, but they are still undefined because the snippet has not been called.

Normally, MODX skips placeholders that are undefined during initial parser passes. But, there may be a problem with the "ne" output being processed on an undefined placeholder. Of course "undefined" will always not equal "about".

Am I missing anything?

matdave commented 8 years ago

Finally found it. I tried the same steps in a non-pdoTools site, and it worked as expected cached. As an additional step (to try and point blame on pdoTools) I installed pdoTools and tested it again, but it still worked. This was really annoying...

After some further debugging, I found that at some point pdoTools stopped updating on the Installer. So older sites with pdoTools were looking like they were up-to-date, but were actually a few versions behind.

Once I uninstalled and reinstalled the newest pdoTools it all worked as defined.

opengeek commented 8 years ago

So this is not a bug — closing! YAY!

pixelchutes commented 8 years ago

I hope this really can stay closed... I think I may have run into something similar (from 2.4.3 to 2.4.4), and pdoTools is not even installed.

In short, things working fine simply broke, and that's even with the new delay parser setting disabled.

The only fix was changing a bunch of placeholders to !uncached... I'll see if I can eventually isolate, and (if related) report back here with more specifics.

EDIT: To yoleg's point, perhaps it was simply a case of bad design / unintentional behavior working in my favor. I do agree with the logic, just wasn't prepared for the breaking change in a patch release.

opengeek commented 8 years ago

Well, if you can document some situations you've had to change in this regard, it would be nice if we could warn others to any potential, though unintentional BC breaks...

gadgetto commented 8 years ago

I have the same problem: since Revo 2.5 this simple conditional output modifier doesn’t work any longer when called cached:

[[+placeholder:notempty=`
    <div>This HTML snippet isn't written to page</div>    
`]]

this works as expected:

[[!+placeholder:notempty=`
    <div>Output of this HTML snippet is OK!</div>    
`]]

The strange part is - if I output the cached content of the placeholder directly, the correct placeholder content is displayed!

yoleg commented 8 years ago

I haven't tried on 2.5 (I only tested on 2.4.4). I'll try to test it on 2.5 as soon as I get a chance. I'd be happy to troubleshoot, since this problem is possibly related to my fix, but I need to reproduce it first :/.

By the way, @matdave @gadgetto, what is the reason that you are calling placeholders cached that are generated by an uncached snippet? From the docs:

If you want to call a Snippet uncached that sets placeholders, you need to make sure the placeholders are set to uncached as well

gadgetto commented 8 years ago

Hi Oleg,

in my case, the placeholder is created by a snippet [[!scAddProduct]](from the SimlpeCart package) which needs to be called uncached. Its my fault that I didn’t see the requirement in the docs - but this worked all the years ago. Now many sites have broken features. :-(

As I said - its strange that the placeholder holds the correct value, but the output modifier doesn’t work.

Am 26.04.2016 um 06:51 schrieb Oleg Pryadko notifications@github.com:

I haven't tried on 2.5 (I only tested on 2.4.4). I'll try to test it on 2.5 as soon as I get a chance. I'd be happy to troubleshoot, since this problem is possibly related to my fix, but I need to reproduce it first :/.

By the way, @matdave https://github.com/matdave @gadgetto https://github.com/gadgetto, what is the reason that you are calling placeholders cached that are generated by an uncached snippet? From the docs https://rtfm.modx.com/revolution/2.x/making-sites-with-modx/tag-syntax:

If you want to call a Snippet uncached that sets placeholders, you need to make sure the placeholders are set to uncached as well

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/modxcms/revolution/issues/12970#issuecomment-214615833

whauben commented 8 years ago

I have to confirm this issue. Unfortunately, in our case, simply calling something uncached didn't fix the problem.

A simplified example that no longer works after upgrading to 2.4.4 (coming from 2.3.X)

[[!pdoResources:default=`[[$fallback-chunk]]` ]]

Even though the output of pdoResources is indeed empty, the fallback-chunk isn't displayed.

Jako commented 8 years ago

Maybe you have to call the chunk uncached too. If that is the case, the fix has to be modified.

whauben commented 8 years ago

No dice unfortunately, the chunk refuses to display even when called uncached.

Jako commented 8 years ago

@yoleg Could you look into that?

Bruno17 commented 8 years ago

seems :default does not work on snippets

did you try 

[[!pdoResources? &toPlaceholder=`results`]]

[[+results:default=`default`]]

this seems to work. But not:

[[!pdoResources:default=`Default`]]
whauben commented 8 years ago

I haven't tested that approach myself, but it's an unacceptable fix nonetheless.

With several implementations such as: [[++setting_name[[!snippet:is=true:then=_postfix]]]] you can imagine the headache this bug has caused =)

Bruno17 commented 8 years ago

on a fresh installation of Revo 2.5.0, only pdoTools installed, this is working for me:

[[!pdoResources:default=`[[$fallback-chunk]]`]]

so, it seems, this is not related to that fix.

yoleg commented 8 years ago

Everyone: please be aware that pdoTools changes the behavior of the MODX parser. So, if you have a problem with output filters and you have pdoTools installed, you should completely uninstall pdoTools and see if the bug persists.

If you must have pdoTools installed, please report the bug to the pdoTools developers.

@whauben @Bruno17 You should report that bug to pdoTools developers.

Is anyone having problems with output filters on uncached snippets with pdoTools absolutely, certainly NOT installed? If so, can you please try reproducing it on a fresh install of MODX and tell me the steps you used to reproduce it?

Bruno17 commented 8 years ago

@yoleg I didn't say anything about a bug on my site. For me it is the correct behaviour as it is now. I was just saying, that everthing is working as expected for me, also with pdoTools installed (on a fresh installation)

I had only an issue on an older upgraded installation, which can be a result of any of the running plugins or something, what went wrong while upgradings. But that's another story.

yoleg commented 8 years ago

@Bruno17 Thanks for the clarification!

whauben commented 8 years ago

Has it been confirmed that merely having pdoTools installed is the culprit, and if so, is there a workaround to fix this whilst keeping pdoTools installed? Calling things uncached just doesn't cut it. I'm having another case in which [[+user.comment:notempty=]] doesn't work, but [[!+user.comment:notempty=]] does. (using the Profile snippet, modx 2.5)

OptimusCrime commented 8 years ago

The "workaround" would be for the pdoTools developer to fix this issue and release a updated version I'd guess. No reason for an extra to change the parser imo.

gadgetto commented 8 years ago

I can confirm that pdoTools have nothing to do with this (at least in my case) as I don't have installed them.

yoleg commented 8 years ago

I had a colleague try to reproduce it on a new install, and he could not reproduce it either.

Can anyone send me a snapshot/ export of a site that has that problem (with no pdoTools installed)? Perhaps you, @gadgetto ?

gadgetto commented 8 years ago

@yoleg sorry, installed pdoTools in meantime ...

Jako commented 8 years ago

@gadgetto Maybe you have an older db and/or file backup available of that installation.

gadgetto commented 8 years ago

@yoleg I'll try to get a snap from an older backup (need to talk to my business partner). The problem is, the snapshot will contain alot of confidential personal data of customers and other details I don't want to hand over to 3rd parties. So it probably will take a lot of work to remove those data...

yoleg commented 8 years ago

@gadgetto Yes, I thought that might be the case. If you do find an older snapshot, can you please recursively grep the core/components directory for extends mod? For example, pdoTools uses class pdoParser extends modParser. There might be another parser-modifying component, or perhaps you had uninstalled pdoTools, and it did not fully uninstall.

gadgetto commented 8 years ago

@yoleg I definitely didn't have installed pdoTools before! I posted a reply to this thread on April but pdoTools were installed end of June for the first time ...

gadgetto commented 8 years ago

@yoleg this is the complete list of installed add-ons:

Ace
DeinstallierenNeu installierenAuf Updates prüfenLöschenDetails anzeigen
1.6.5-pl
Installiert am 
13.01.2016, 22:15
modx.com

Archivist
DeinstallierenNeu installierenAuf Updates prüfenLöschenDetails anzeigen
1.2.4-pl
Installiert am 
15.09.2015, 18:13
modx.com

ClientConfig
DeinstallierenNeu installierenAuf Updates prüfenLöschenDetails anzeigen
1.3.2-pl
Installiert am 
13.12.2015, 12:13
modmore.com

Collections
DeinstallierenNeu installierenAuf Updates prüfenLöschenDetails anzeigen
3.4.2-pl
Installiert am 
14.03.2016, 20:09
modx.com

CookieList
DeinstallierenNeu installierenAuf Updates prüfenLöschenDetails anzeigen
1.0.0-pl
Installiert am 
22.01.2016, 06:25
modx.com

FileAttach
DeinstallierenNeu installierenAuf Updates prüfenLöschenDetails anzeigen
1.0.8-pl1
Installiert am 
27.06.2016, 19:18
modx.com

FormIt
DeinstallierenNeu installierenAuf Updates prüfenLöschenDetails anzeigen
2.2.10-pl
Installiert am 
14.03.2016, 20:09
modx.com

getPage
DeinstallierenNeu installierenAuf Updates prüfenLöschenDetails anzeigen
1.2.4-pl
Installiert am 
25.04.2016, 21:09
modx.com

getResources
DeinstallierenNeu installierenAuf Updates prüfenLöschenDetails anzeigen
1.6.1-pl
Installiert am 
15.09.2015, 18:14
modx.com

getUrlParam
DeinstallierenNeu installierenAuf Updates prüfenLöschenDetails anzeigen
1.0-beta1
Installiert am 
04.12.2015, 19:35
modx.com

goodnews
DeinstallierenNeu installierenLöschenDetails anzeigen
1.4.1-pl
Installiert am 
05.03.2016, 17:00

If
DeinstallierenNeu installierenAuf Updates prüfenLöschenDetails anzeigen
1.1.1-pl
Installiert am 
05.12.2015, 15:10
modx.com

Login
DeinstallierenNeu installierenAuf Updates prüfenLöschenDetails anzeigen
1.9.2-pl
Installiert am 
07.01.2016, 23:21
modx.com

pdoTools
DeinstallierenNeu installierenUpdateLöschenDetails anzeigen
2.5.4-pl
Installiert am 
01.07.2016, 15:32
modx.com

pThumb
DeinstallierenNeu installierenAuf Updates prüfenLöschenDetails anzeigen
2.3.3-pl
Installiert am 
09.10.2015, 16:46
modx.com

Quickstart Buttons
DeinstallierenNeu installierenAuf Updates prüfenLöschenDetails anzeigen
1.3.0-rc1
Installiert am 
14.03.2016, 20:10
modmore.com

Redactor
DeinstallierenNeu installierenAuf Updates prüfenLöschenDetails anzeigen
2.2.0-pl
Installiert am 
24.05.2016, 23:19
modmore.com

resizer
DeinstallierenNeu installierenAuf Updates prüfenLöschenDetails anzeigen
1.0.1-pl
Installiert am 
09.10.2015, 16:46
modx.com

SimpleCart
DeinstallierenNeu installierenAuf Updates prüfenLöschenDetails anzeigen
2.5.0-dev1
Installiert am 
03.05.2016, 15:05
modmore.com

PayPal Gateway for SimpleCart
DeinstallierenNeu installierenAuf Updates prüfenLöschenDetails anzeigen
1.1.5-pl
Installiert am 
14.03.2016, 20:11
modmore.com

simpleport
DeinstallierenNeu installierenLöschenDetails anzeigen
0.2.4-alpha1
Installiert am 
27.06.2016, 19:14

SimpleSearch
DeinstallierenNeu installierenAuf Updates prüfenLöschenDetails anzeigen
1.9.2-pl
Installiert am 
31.12.2015, 14:53
modx.com

Tagger
DeinstallierenNeu installierenAuf Updates prüfenLöschenDetails anzeigen
1.8.0-pl
Installiert am 
27.06.2016, 19:19
modx.com

translit
DeinstallierenNeu installierenAuf Updates prüfenLöschenDetails anzeigen
1.0.0-beta
Installiert am 
26.09.2015, 13:17
modx.com

UltimateParent
DeinstallierenNeu installierenAuf Updates prüfenLöschenDetails anzeigen
2.0-pl
Installiert am 
13.12.2015, 12:14
modx.com

Wayfinder
DeinstallierenNeu installierenAuf Updates prüfenLöschenDetails anzeigen
2.3.3-pl
Installiert am 
15.09.2015, 18:04
modx.com
yoleg commented 8 years ago

@gadgetto I have not used all of these components. Can you do `grep -R 'extends mod' /path/to/your/www/core/components/'? That's all I can think of to do without being able to reproduce the problem myself and run it through a debugger.

gadgetto commented 8 years ago

@yoleg OK, will need time until tomorrow evening...

yoleg commented 8 years ago

By the way, if anyone filed or saw a public bug report on this topic with pdoTools, please add the link here for reference.

yoleg commented 8 years ago

@whauben I think the "proper" fix for that is to call [[++setting_name uncached: [[!++setting_name[[!snippet:is=true:then=_postfix]]]]. I agree with you that the MODX parser should be smart enough to figure this part out itself, but it seems like a different issue that this particular bug report.

yoleg commented 8 years ago

@whauben To keep pdoTools but disable the parser, you might try fiddling with the system settings. I think changing pdoParser.class and parser_class to modParser might disable the pdoTools parser.

yoleg commented 8 years ago

@whauben I think there is also the system setting parser_class_path, which I think you can just set to an empty string.

yoleg commented 8 years ago

@gadgetto Actually, this means that instead of grepping /core/components/, you could check the values of the parser_class and parser_class_path system settings in the old snapshot (and also check which plugins were enabled on which events).

gadgetto commented 8 years ago

@yoleg here is a DropBox link to the grep output: https://www.dropbox.com/s/ozr7lm30mtnwm0z/extends_mod.grep?dl=0

I dont have a snapshot available from before July 1st (before I installed pdoTools), sorry.

yoleg commented 8 years ago

@gadgetto Thanks. I don't see anything there besides pdoTools that extends the parser. Collections and SimpleCart both have custom resource classes, but unless the problem you had was limited to those resource types, they probably did not contribute to that.

I guess all I can do is wait to see if someone else can help me reproduce the problem on my laptop so that I can debug it.

nikonieminen commented 7 years ago

I'm quite certain that I have/had problems similar to what have been discussed here.

A project I'm working on stopped working as expected after upgrading from 2.4.2 to 2.4.3 and to newer versions.

I have now managed to experimentally upgrade to the latest 2.5.6 after commenting out the following lines in that version:

/core/model/modx/modparser.class.php:456-460

        $_restoreProcessingUncacheable = $this->_processingUncacheable;
        /* stop processing uncacheable tags so they are not cached in the cacheable content */
        if ($this->_processingUncacheable && $cacheable && $this->modx->getOption('parser_recurse_uncacheable', null, true)) {
            $this->_processingUncacheable = false;
        }

/core/model/modx/modparser.class.php $this->_processingUncacheable = $_restoreProcessingUncacheable;

and changing the /core/model/modxparser.class.php:1216 to if ($this->_output !== null || $this->modx->parser->isProcessingUncacheable()) {

Complete changed file from 2.5.6: modparser.class.php.txt

I realize this may be hard for others to reproduce, but I can confirm that these lines changed the caching somehow so that it caused problems for me.

OptimusCrime commented 6 years ago

@bezumkin is this problem solved with the parser changes in 2.6?

bezumkin commented 6 years ago

@OptimusCrime I can`t reproduce bug described in the start topic at 2.6. As far as can see there is no 100% repeatable bug to check.

2.6 changes only processing of those cached tags, that contains uncached inside, like

[[tag?param=`[[!othertag]]`]]

and makes them uncacheble too.

OptimusCrime commented 6 years ago

@matdave Is this still a problem? Is it solved in 2.6?

matdave commented 6 years ago

@OptimusCrime I rebuilt the code on all our old sites to properly uncache placeholders from uncached snippets, and do a better job of letting other snippets be cached. I haven't tried to reproduce this issue, because I've changed the way I write the code to accommodate.