croxton / Stash

Stash allows you to stash text and snippets of code for reuse throughout your templates.
GNU General Public License v3.0
197 stars 20 forks source link

Low search compatibility / process="static" nocache cleanup bug #98

Closed timkelty closed 10 years ago

timkelty commented 10 years ago

Here's my tag:

{exp:stash:cache}
  {exp:low_search:form
    result_page="search/results"
    form_class="Search"
    loose_ends="right"
    form_id="search"
    required="keywords"
    secure="no"
  }
    ...
  {/exp:low_search:form}
{/exp:stash:cache}

First time the template is hit, everything is ok. Subsequest hits, {AID:Low_search:catch_search} (value of a hidden input created by low_search) doesn't get parsed.

Here's what's in the saved cache var:

type="hidden" name="XID" value="{XID_HASH}" />
<input
type="hidden" name="params" value="eyJyZXN1bHRfcGFnZSI6InNlYXJjaFwvcmVzdWx0cyIsInJlcXVpcmVkIjoia2V5d29yZHMifQ" />
<input
type="hidden" name="ACT" value="{AID:Low_search:catch_search}" />

Wrapping the low_search:form tag in stash:nocache tags fixes things (even though I don't really care about not caching it, hence the secure=no).

The problem is: I have some templates with process="static". It would seem in this case, the {stash:nocache} tags arent't removed. Interestingly, if I use {exp:stash:static} instead, the {stash:nocache} tags get "cleaned up", as I want them to be.

So this works:

{exp:stash:static}
{exp:stash:cache}
  {stash:nocache}
    {exp:low_search:form
      result_page="search/results"
      form_class="Search"
      loose_ends="right"
      form_id="search"
      required="keywords"
      secure="no"
    }
      <div class="Search-input">
        <label for="keywords" class="u-isHiddenVisually">Keywords</label>
        <input type="text" name="keywords" id="keywords" placeholder="Search">
      </div>
      <button id="search-submit" type="submit" class="Search-submit Icon Icon--magGlass">
        <span>Search</span>
      </button>
    {/exp:low_search:form}
  {/stash:nocache}
{/exp:stash:cache}

But this doesn't (meaning the low_search tags are parsed and cached ok, but the {stash:nocache} tags are left behind)

{exp:stash:cache process="static"}
  {stash:nocache}
    {exp:low_search:form
      result_page="search/results"
      form_class="Search"
      loose_ends="right"
      form_id="search"
      required="keywords"
      secure="no"
    }
      <div class="Search-input">
        <label for="keywords" class="u-isHiddenVisually">Keywords</label>
        <input type="text" name="keywords" id="keywords" placeholder="Search">
      </div>
      <button id="search-submit" type="submit" class="Search-submit Icon Icon--magGlass">
        <span>Search</span>
      </button>
    {/exp:low_search:form}
  {/stash:nocache}
{/exp:stash:cache}

Again, I don't really need to nocache these, that was just the only way I could get it to parse {AID:Low_search:catch_search}

Maybe related: https://getsatisfaction.com/low/topics/low_search_aid_tag_issues_on_production_environment_possible_stash_embed_conflict

croxton commented 10 years ago

You can't have any tags left in the output of a static cached page, because that url when subsequently viewed will be served as a plain HTML page and does not pass through PHP/CI/EE at all. {stash:nocache} tags are therefore ignored when you use static caching.

Two things remain unparsed in the form output: {XID_HASH} and the action id {AID:..}. The former should never be parsed, because it is a unique token generated for each form view (and it follows that if you use secure form tokens you cannot static-cache the tokens). The later is a simple ID that does not change, that could be parsed for the static cache:

In mod.stash.php the function save_output() just below this line:

$this->EE->TMPL->tagdata = $this->EE->TMPL->parse_globals($output); 

Add this:

 $this->EE->TMPL->tagdata = $this->EE->functions->insert_action_ids($this->EE->TMPL->tagdata);

Let me know if that works for you.

timkelty commented 10 years ago

That's the difference between exp:stash:cache process="static" and exp:stash:static, then? The latter seems to "clean up" any stray nocache tags, but the former leaves them, which is my problem.

In this case I don't care if the {XID_HASH} is cached because I'm bypassing secure form processing (using the secure="no") param. Apparently Low Search still puts the input in there, just doesn't use it.

croxton commented 10 years ago

In your first template the {stash:nocache} tags are removed by the {exp:stash:cache} tag pair when it parses it's tagdata, and then the rendered content (with the form unparsed) is returned to the template. This gets parsed and is then captured by {exp:stash:static}. Hence there are no nocache tags left in the output cached as a static page.

In your second the nocache tags are correctly ignored. If you remove them, make the addition I suggested above then you should find that the cached static output includes the rendered value of {AID:...}

timkelty commented 10 years ago

If I apply your patch, remove the stash:nocache tags, it only fixes it when process=static (since save_output() is only for static.

Without the process param (or process="end"), {AID:...} remains unparsed for subsequent loads after the first (it is properly parsed the first load).

croxton commented 10 years ago

Right, so when it makes the form Low Search must generate the AID as a global variable, and on subsequent views since Low Search doesn't run (because it's been rendered as html) that global doesn't get set, and so the placeholder is not replaced. So we need to parse it before caching. That's a bit trickier than for static caching but I can see how it can be done.

timkelty commented 10 years ago

Ok. For now I can leave it in the nocache tags and use exp:stash:static instead of process="static". Not ideal but it works.

timkelty commented 10 years ago

Probably oversimplifying it, but would it make sense to have stash:cache remove any stash:nocache tags from it's final output (when you have process="static"), since you'd never want to actually output those?

A bit of a roundabout solution, but it would fix the problem I'm having.

croxton commented 10 years ago

Possibly as a new clean_string() option, strip_stash_tags="yes"

timkelty commented 10 years ago

sounds better!

timkelty commented 10 years ago

Might be worth noting that for my case it would important to strip just the stash tags, and not the tagdata.

thisisjamessmith commented 10 years ago

Hi guys, thanks for your work investigating this. I'm facing the same issue, but need to keep secure forms on (not for Low Search but for another form which really needs it since it writes to the database). I've been trying various combinations of stash:nocache without much luck - any ideas?

croxton commented 10 years ago

James, just to be clear you can't have any dynamic content inside a static-cached page. It's impossible because the static html page generated bypasses php/ee entirely. If your form needs to be processed by EE you should use standard caching and escape the form with a {stash:nocache} pair.

thisisjamessmith commented 10 years ago

Hi Mark, thanks for the clarification. In this case, the search form is in a global header in a stash embed, but my exp:stash:cache tags are wrapping the main template content, so when using stash:nocache in the layout template I'm then encountering this issue http://devot-ee.com/add-ons/support/mustash/viewthread/12540 on pages that should not be cached... sorry for going round in circles!

croxton commented 10 years ago

Ah, right. good. I'll have a new parameter shortly to remove those which should clear that up for you on un-cached pages.

croxton commented 10 years ago

Hang on, on un-cached pages you won't necessarily have the content of your template wrapped by a stash tag, so a parameter wouldn't help in that case. You need to strip those tag pairs before final output somehow, even if Stash doesn't see get to parse the content. Right?

One possible solution would be to use a module tag pair {exp:stash:nocache} ...{/exp:stash:nocache} instead which returns it's tagdata by default. When inside a block of content parsed by Stash, Stash could look for these and parse them before anything else. Does that make sense?

croxton commented 10 years ago

It would be nice if EE had a final_output hook to do this kind of cleanup without elaborate workarounds.

thisisjamessmith commented 10 years ago

yep that makes sense.

thisisjamessmith commented 10 years ago

I thought that's what template_post_parse was for, but I guess that runs on every template

croxton commented 10 years ago

Please see feature/nocache branch https://github.com/croxton/Stash/tree/feature/nocache

I've added a tag which you can add anywhere to your template to remove placeholders from the final output, e.g. {exp:stash:cleanup strip="stash:nocache|something_else"}. As a bonus you can run any of the other methods in the _clean_string() function too, e.g. compress='y' to compress your final html output.

In theory this cleanup tag, if used, will always be called before {exp:stash:static} tag (or process="static") so will remove any placeholders you want from static cached pages too.

I went down this route instead of turning {stash:nocache} itself into a tag because the overhead of a tag is greater than a placeholder, and I thought it would be better to allow you to only run a cleanup where you need it.

With regards to parsing of Action IDs in non-static cached pages with {exp:stash:cache}, you can cause them to be parsed by using process="inline" so that the cache is created before EE's own later parsing functions are run. I may make that the default in future instead of 'end', not sure yet.

croxton commented 10 years ago

Also fun: use the _clean_string()methods in {exp:stash:cache}.

timkelty commented 10 years ago

Hmmm...Haven't' gotten the cleanup tag to work.

Template

{exp:stash:cleanup strip="stash:nocache"}
{exp:stash:cache process="static"}
  {stash:nocache}
    Please remove the nocache tags around me when I'm static!
  {/stash:nocache}
{/exp:stash:cache}

Output


{c0f14f51c4381c1007c9d66330ee945e235188595}

{stash:nocache}
Please remove the nocache tags around me when I'm static!
{/stash:nocache}
croxton commented 10 years ago

Did you replace the extension file too? You'd see that if you hadn't.

timkelty commented 10 years ago

Yep, Just triple checked.

Also uninstalled/reinstalled with no luck.

EE 2.7.2

timkelty commented 10 years ago

Interesting, when I log $save_output and $template (around line 414 in ext), they're different:

array (size=6)
  'method' => string 'save_output' (length=11)
  'tagproper' => string '{exp:stash:cache process="static"}' (length=34)
  'tagparams' => 
    array (size=2)
      'process' => string 'static' (length=6)
      'name' => string 'static' (length=6)
  'tagdata' => string '
  {stash:nocache}
    Please remove the nocache tags around me when I'm static!
  {/stash:nocache}
' (length=100)
  'priority' => string '999999' (length=6)
  'placeholder' => string '8dd7afc40d17058716a955f6ac924840198617860' (length=41)
string '{c0f14f51c4381c1007c9d66330ee945e1181092950}

{stash:nocache}
Please remove the nocache tags around me when I'm static!
{/stash:nocache}' (length=136)
croxton commented 10 years ago

Ok it looks like the static caching is being called before the cleanup tag, which is why it remains as a placeholder. It has a lower priority assigned in the post-processing order, so that really shouldn't be possible. Looking into it now.

croxton commented 10 years ago

No, I'm wrong the tags are being called in the right order (good) but the template is not being updated before being passed to the static caching. Not sure why.

croxton commented 10 years ago

Fixed it - please try latest commit to feature/nocache : c69c95a51

timkelty commented 10 years ago

Ok, that fix was better :).

So that solves my simplified example, in acutal application, here's the problem I'm having:

It appears that if you have a template with the cleanup tag, and nocache tags in that same template, it will still parse the the inner contents of the nocache tags, and remove the {nocache} from the output. However, if those are in a nested stash template, the inner contents remain unparsed.

Example:

Parent template

{exp:stash:cleanup strip="stash:nocache"}
{exp:stash:cache process="static"}
  {stash:embed name="layouts/default"}
{/exp:stash:cache}

layouts/default

  {stash:nocache}
    {exp:low_search:form
      result_page="search/results"
      form_class="Search"
      loose_ends="right"
      form_id="search"
      required="keywords"
      secure="no"
    }
      <div class="Search-input">
        <label for="keywords" class="u-isHiddenVisually">Keywords</label>
        <input type="text" name="keywords" id="keywords" placeholder="Search">
      </div>
      <button id="search-submit" type="submit" class="Search-submit Icon Icon--magGlass">
        <span>Search</span>
      </button>
    {/exp:low_search:form}
  {/stash:nocache}

Result

 {exp:low_search:form
    result_page="search/results"
    form_class="MobileNav-search"
    loose_ends="right"
    form_id="search-mobile"
    required="keywords"
    secure="no"
  }
    <div class="MobileNav-searchInput">
      <input type="text" name="keywords" id="keywords" placeholder="Search">
    </div>
    <button type="submit" class="MobileNav-searchSubmit Icon Icon--magGlass">
      <span>Search</span>
    </button>
  {/exp:low_search:form}

Ideas?

timkelty commented 10 years ago

adding process="start" fixes it in that example, though I'm not sure I'd be able to do that in my actual template setup without breaking other things...

croxton commented 10 years ago

OK, I've changed the tag to this:

{exp:stash:finish nocache="no"}

The nocache="no" parameter will disable {stash:nocache} escaping AND remove the tag pairs from the final template output (before caching), even if you have used a custom prefix anywhere (e.g. {my_prefix:nocache}). Tags wrapped by {stash:nocache} will then be parsed normally.

As before all the _clean_string() parameters can also be used to cleanup or compress the final output of the template prior to caching.

df437a94eefbf7f14cdf2e235adc7fc3f6b94e40

timkelty commented 10 years ago

Cool.

Heading off for vacation shortly, but I'll let you know when I give it a shot.

croxton commented 10 years ago

Implemented with c7b409bd29109805769c7296c4520707d18d888f

thisisjamessmith commented 10 years ago

Thanks Mark, this is working for me, and the compress/trim options are a great bonus!

The only thing that's a bit strange is that a developer now has to remember to add this extra tag to all non-cacheable templates - would be more intuitive if it just happened by default and then got overridden when using caching, but appreciate that's probably not at all straight-forward if even possible.

timkelty commented 10 years ago

Finally got around to testing this, and it works like a charm.

Totally solved all my problems!