Bruno17 / MIGX

MIGX for revo 2.2 and above
84 stars 78 forks source link

issue with modx operators after editing existing migx feilds #252

Open scorchsoft opened 8 years ago

scorchsoft commented 8 years ago

If you add a new feild to a migx TV within form tabs, and you try to run an opperator on that new feild then migx fails to display or process those chunks correctly. You have to manually open every single migx item on every single page and re-save it.

For example if we went started with this and added a few resources that use the migx TV and have a few rows set for it:

[
    {
      "formname":"block-general",
      "formtabs":[
        {
            "caption":"Our Work Items", 
            "fields": [

                {   
                    "field":"feild01",
                    "caption":"Our Work Title",
                    "inputTV":"migxInput"
                }
            ]
        }
      ]
    }
] 

And then at some point in the future we change it to add feild02:

[
    {
      "formname":"block-general",
      "formtabs":[
        {
            "caption":"Our Work Items", 
            "fields": [
                {   
                    "field":"feild01",
                    "caption":"Our Work Title",
                    "inputTV":"migxInput"
                },
                {   
                    "field":"feild02",
                    "caption":"Our Work Title",
                    "inputTV":"migxInput"
                }
            ]
        }
      ]
    }
] 

Next imagine we have a chunk that getImageList is calling a chunk that used to look like this:

<div>
   <h1>[[+feild01]]</h1>
</div>

and then we change it to add a modx operator on our new feild02:

<div>
   <h1>[[+feild01]]</h1>
   [[+feild02:isnot=``:then=`
      <span>[[+feild02]]</span>
   `]]
</div>

The changed version fails to render properly when the chunk has been changed to perform an operation on the new feild. You have to then go in and re-save every item in the list within the migx TV on every page to fix the problem (which it looks like creates the database records for feild02, which is why this fixes it).

What looks to be happening is that because the record has been added, then MigX has been altered, the database record for 'feild02' was never stored against each item. So when you try and run an operator on it, it doesn't know what to do because it cannot even find a blank record.

What should happen instead: if an operator is attempted to be run on a truly non-existent database record then it should detect that and treat it as if it is happening on an empty string.

scorchsoft commented 8 years ago

please excuse my mis-spelling of "field", you should get the gist however.

Bruno17 commented 8 years ago

this has nothing todo with MIGX. An not existing placeholder doesn't get parsed by the MODX-parser and will stay as unparsed tag, at the time, when the ouput-filter gets parsed. Try, if you get somewhere, with an else. If not, you will need to check against the placeholder, too. Maybe with a custom-outputfilter or a snippet.

scorchsoft commented 8 years ago

Hi Bruno, thanks for your response.

I would agree with you but it's the output that I'm getting that is suspect (and suggests otherwise).

So assuming no field02 is set for example you would expect something like this:

<div>
   <h1>[[+feild01]]</h1>
   [[+feild02:isnot=``:then=`
      <span>[[+feild02]]</span>
   `]]
</div>

To appear like this right?: Feild content [[+feild02:isnot=`:then=[[+feild02]]`]]

or maybe just like this: Feild content

Whereas what actually happens is that the whole chunk fails to render completely, including HTML outside of the tag. Note that I don't get this behaviour if I reference a newly created plain resource TV (e.g.

 [[*pageField:isnot=``:then=`<span>[[*pageField]]</span>`]] 

) Which implies the issue is to do with migx's processes handling this slightly differently.

The effect of this is that it isn't practical on live sites to ever change the migx formtabs taxonomy if you plan on using operators on the new variables. Which for large sites is very restrictive and potentially very damaging.

Bruno17 commented 8 years ago

hmm... I took your example

<div>
   <h1>[[+feild01]]</h1>
   [[+feild02:isnot=``:then=`
      <span>[[+feild02]]</span>
   `]]
</div>

use it in a chunk as tpl for getImageList on a MIGX-TV, which doesn't have that field. But can't reproduce you issue.

What I get is:

   <h1></h1>

</div><div>
   <h1></h1>

</div><div>
   <h1></h1>

</div><div>
   <h1></h1>

</div><div>
   <h1></h1>
scorchsoft commented 8 years ago

Hi Bruno,

I've just accidentally responded in the wrong place. See my response here: https://github.com/Bruno17/MIGX/issues/211

Thanks Andrew

P.s. If you fix this I will owe you a beer.

Bruno17 commented 8 years ago

maybe, you have to much nested output-filters. what happens with:

            [[[[*inheritWorkFrom:isnot=``:then=`
                getImageList? 
                  &tvname=`OurWork`
                  &tpl=`tpl.workItem`
                  &docid=`[[*inheritWorkFrom]]`
            `:else=`
                getImageList? 
                  &tvname=`OurWork`
                  &tpl=`tpl.workItem`
                  &docid=`[[*id]]`
            `]]]]
scorchsoft commented 8 years ago

Ok I've changed it to that: http://fluence.world/case-studies-and-success-stories

and it outputs the following html:

<div class="image-hover-block">
                  <h4>JW - Cookham Wood</h4>
                  <p>“JW was assessed at Entry Level 1 for literacy. During our sessions we built up a good rap-port and he used [Rapid English] exercises to tell "his story" and talk about his family, which was extremely beneficial. When he left he passed his Functional Skills Level 1 in Literacy.”</p>
</div>

which looks like it may be correct now. What do the extra square brackets do?

scorchsoft commented 8 years ago

Actually scratch that, it is still outputting tag code:

    <div class="image-hover-block">
                  <h4>JW - Cookham Wood</h4>
                  <p>“JW was assessed at Entry Level 1 for literacy. During our sessions we built up a good rap-port and he used [Rapid English] exercises to tell "his story" and talk about his family, which was extremely beneficial. When he left he passed his Functional Skills Level 1 in Literacy.”</p>

                </div>
           </div>

            ]]

(see brackets at the bottom)

scorchsoft commented 8 years ago

in the bit that should be output based on the rules:

  <div class="single-work-block [[+block_size]]">
            [[+block_size:is=`size-1-1`:then=`<img src="[[+block_image:phpthumbof=`w=380&h=374&zc=1`]]" alt="Our Work [[+block_title]] " class="img-responsive">`]]
            [[+block_size:is=`size-1-2`:then=`<img src="[[+block_image:phpthumbof=`w=760&h=374&zc=1`]]" alt="Our Work [[+block_title]] " class="img-responsive">`]]
            [[+block_size:is=`size-2-1`:then=`<img src="[[+block_image:phpthumbof=`w=380&h=748&zc=1`]]" alt="Our Work [[+block_title]] " class="img-responsive">`]]
            [[+block_size:is=`size-2-2`:then=`<img src="[[+block_image:phpthumbof=`w=760&h=748&zc=1`]]" alt="Our Work [[+block_title]] " class="img-responsive">`]]

            [[+image_title:isnot=``:then=`
                <div class="image-title">
                  [[+image_title]]
                </div>
            `]]

            [[+block_textarea:isnot=``:then=`
                <div class="image-hover-block">
                  <h4>[[+block_title]]</h4>
                  [[+block_textarea]]

                  [[+button_link:isnot=``:then=`
                      <a class="btn dark-btn" href="[[+button_link]]">[[+button_text]]</a>
                  `]]
                </div>
            `]]

           </div>

you would expect to see

<div class="image-title">

somewhere, but it is nowhere to be found, it's like its being deleted due to tag parsing failing. Then the closing ]] output on page for some reason.

latest TPL: latest tpl.txt

Note that this only seems to happen when block_size is not set in the data on the page because it has been added after an entry has been populated. If I were to go into the migx tv on this page and save it then it would work.

Edit: I've just duplicated the record and edited and saved it to the bottom of the migx tv list on this page for you to see what I mean: http://fluence.world/case-studies-and-success-stories screencapture-fluence-world-case-studies-and-success-stories-1456508423670

The second item outputs it's HTML fine because it has the relevant variables set:

<div class="single-work-block size-1-1">
            <img src="/assets/components/phpthumbof/cache/JW%20–%20Cookham%20Wood.66acac706771c1ec837ec4078535eaf916.jpg" alt="Our Work JW - Cookham Wood222 " class="img-responsive">

                <div class="image-hover-block">
                  <h4>JW - Cookham Wood222</h4>
                  <p>“JW was assessed at Entry Level 1 for literacy. During our sessions we built up a good rap-port and he used [Rapid English] exercises to tell "his story" and talk about his family, which was extremely beneficial. When he left he passed his Functional Skills Level 1 in Literacy.”</p>
</div>

So it does look like it is a parsing bug with nested operators when handling values that are empty. It seems to fail to parse and handle the outputting as it normally would do outside of migx.

Bruno17 commented 8 years ago

I still think, you are overusing conditional output-filters and that's the reason for your issues. Try to avoid them, where possible. For example this isn't a good idea:

[[+block_size:is=`size-1-1`:then=`<img src="[[+block_image:phpthumbof=`w=380&h=374&zc=1`]]" alt="Our Work [[+block_title]] " class="img-responsive">`]]
            [[+block_size:is=`size-1-2`:then=`<img src="[[+block_image:phpthumbof=`w=760&h=374&zc=1`]]" alt="Our Work [[+block_title]] " class="img-responsive">`]]
            [[+block_size:is=`size-2-1`:then=`<img src="[[+block_image:phpthumbof=`w=380&h=748&zc=1`]]" alt="Our Work [[+block_title]] " class="img-responsive">`]]
            [[+block_size:is=`size-2-2`:then=`<img src="[[+block_image:phpthumbof=`w=760&h=748&zc=1`]]" alt="Our Work [[+block_title]] " class="img-responsive">`]]

this will run phpthumbof 4 times, to show only one image. And I don't know, which conditionals you are using outside of that part. You never should have big code-blogs inside conditional output-filters. There are much better ways to handle this stuff.

Bruno17 commented 8 years ago

see also this one: https://modx.com/blog/2012/09/14/tags-as-the-result-or-how-conditionals-are-like-mosquitoes/

scorchsoft commented 8 years ago

Oh wow, what the hell. That is horrible, I did not realise it did that. So absolutely everything in my execution hierarchy is running all at once. Very nasty indeed, and I can see why this may cause problems.

I will rewrite this next week and see if the technique you (and the article) have suggested fix my woes. Though it does make sense that it should.

Bruno17 commented 8 years ago

this part, for example could be done absolute without conditionals:

[[+block_size:is=`size-1-1`:then=`<img src="[[+block_image:phpthumbof=`w=380&h=374&zc=1`]]" alt="Our Work [[+block_title]] " class="img-responsive">`]]

if you would use different chunks for that, like

[[$block_image_[[+block_size]]]]

the chunkname would be block_image_size-1-2 and so on.

maybe, you will need to pass the placeholders to the chunk, like that:

[[$block_image_[[+block_size]]? &block_image=`[[+block_image]]` &block_title=`[[+block_title]]` &block_size=`[[+block_size]]`]]
scorchsoft commented 8 years ago

Hi Bruno,

This has fixed the problem. Something is definitely dodgy with so many nested conditions, though as you say this was a bad implementation approach anyway.

Thanks again for your help. Let me know if I can thank you in any way.

Andrew