Change in EE controller (ee.php) in 2.2.1 will probably break some addons using json (incorrectly?)

Bjorn,

It does look like EE is removing line breaks and changing code that uses curly brackets. For example, this:

if ( typeof target !== "object" && !jQuery.isFunction(target) ) {
    target = {};
}

Is changed into this:

if ( typeof target !== "object" && !jQuery.isFunction(target) ) {
    target = {};}

Still functional, but definitely changed. I started looking around and this was the block that was removing the line breaks within Template::parse():

// Remove whitespace from variables.
// This helps prevent errors, particularly if PHP is used in a template
$this->template = preg_replace("/".LD."\s*(\S+)\s*".RD."/U", LD."\\1".RD, $this->template);

Since it doesn’t affect how it works, I’m going to leave this alone.

I also looked into the headers and compared them to Google’s hosted copy of jquery. The only difference was they were specifying a charset and we weren’t. I changed it to specify the charset and that didn’t change it either.

So I kept digging and I finally came across this line in the minified version of jQuery:

{embed:!0,object:"clsid:D27CDB6E-AE6D-11cf-96B8-444553540000",applet:!0}

That is what’s causing the problem. Right now the devs are talking it over, but I’d like to hear your take on this. Would you want to see a static Javascript template that doesn’t change anything, it just serves static content as Javascript? Any other ideas?

Wes

Nice research, Wes 😊

Would you want to see a static Javascript template that doesn’t change anything, it just serves static content as Javascript? Any other ideas?

I guess the ultimate solution would be for the EE template parser not to parse non-EE tags. The {embed} in this specific case almost looks like an EE tag, true - but it’s missing the = and also contain other chars which would not be found in a valid embed tag. So it shouldn’t be a “hit” for the regexp performing this specific task.

IMO, the second best solution would be a static solution, where the .js files are not parsed. In my case, I’d love it if this was a per template setting, maybe even default set to static. Yes, if so people need to be notified of this so that when upgrading it does not break their templates if they use EE tags in a .js file. Alternatively a setting that I could set per template in a theme install script (default_content.php / or theme_preferences.php).

I realise solution 1) would be difficult, it might require rewriting the template parser quite a bit, would require a bunch of testing, and when finished you’d still be uncertain if it works in all cases. Solution 2) is def. the most secure bet .. although also a bit of a let down to say “hey, JS might not work in the EE templates so if you want to be sure go with the static js template” — one of the issues here is that this changes per release, so a js template that works fine in 2.2.1 might not work in the next release.

I guess the ultimate solution would be for the EE template parser not to parse non-EE tags. The {embed} in this specific case almost looks like an EE tag, true - but it’s missing the = and also contain other chars which would not be found in a valid embed tag. So it shouldn’t be a “hit” for the regexp performing this specific task.

I’ll see what I can do here, as this does make the most sense.

IMO, the second best solution would be a static solution, where the .js files are not parsed. In my case, I’d love it if this was a per template setting, maybe even default set to static. Yes, if so people need to be notified of this so that when upgrading it does not break their templates if they use EE tags in a .js file. Alternatively a setting that I could set per template in a theme install script (default_content.php / or theme_preferences.php).

I think we might end up doing this if only because it’ll speed things up. If you have a file like a minified jquery library, it would make sense not to send it through the full parser and serve it as a static file.

one of the issues here is that this changes per release, so a js template that works fine in 2.2.1 might not work in the next release.

Has it been that jittery on you, changing from minor point versions, or is it larger releases where this changes? Also, is it more of a question of jQuery changing or us changing? I’d expect it’s more jQuery then us. In fact, if you look at jQuery 1.3.2 you won’t find that occurrence of

{embed:...

Wes

Sounds great, Wes! May the force be with you :-D

Has it been that jittery on you, changing from minor point versions, or is it larger releases where this changes? Also, is it more of a question of jQuery changing or us changing? I’d expect it’s more jQuery then us.

Well, normally when I build websites I load jQuery directly from Google so this was just used as an example. Most of the problems I’ve experienced when upgrading have been related to inline javascript in templates, e.g. as a specific example I see a push I did about a month ago where I changed this:

if(is_https)
{
    google.load('search', '1', {"language" : "en"});
}

to this:

if(!is_https)
{
    var langObj = new Object();
    langObj.language = 'en';
    google.load('search', '1', langObj);
}

.. because EE was eating the {"language":"en"} .. that might have been the other issue though - but anyway this was something that seemed to change from the previous version as it worked before the upgrade.

Just following this, I keep thinking that putting a block on parsing as well as ‘cleanup’ variable-blanking of anything within

<script></script>

tags will give a great deal of normally operating safety, plus a quickly implementable developer fix if something tries to slip through other protections.

• normal embedded Javascript/JSON or other scripts will automatically be taken care of
• anything else that turns out to cause a problem can be rapidly protected.

Doesn’t it make sense?

Clive,

Your code didn’t come up, mind putting it up so I can see what you’re talking about? Also, when you say ‘putting a block on parsing as well as ‘cleanup’ variable-blanking’, do you mean not doing those two things?

Wes

Yes, I just noticed. Well, let’s try this.

Between (s-cript)(/s-cript) tags.

Where you replace parens with corner brackets, and elide the dashes.

(s-cript) can contain normal arguments, naturally, and still be accepted for this purpose.

Ahh, I see, so anything between script tags wouldn’t be parsed? I have to say I disagree with that idea. What if someone had information that was stored in a Channel and used by Flash? They would then need to output any variables to pass into Flash using script tags, so it would need to parse the EE tags with the script tags.

Wes

Second part:

• yes, I mean not to do EE parsing within script tags.
• yes, I particularly mean not to do the “let’s zap what look like unparsed EE variables” cleanup which is separate from parsing, done just before final emit of parsed code. Used to be in EE_Output; think it might have been moved to Output class recently.

On second point, in other words, do not do the cleanup on script-tag-enclosed text, whatever the state of debug= or other hidden EE config.

Sorry it has to be so convoluted, but that’s the state of things that I have in a prior life once complained about. Life, however, goes on… 😉

C.

Alright, I’ll see what I can do then. If it’s a simple fix I’m all for it. If it makes the template parser slower, it’ll most likely be shelved.

Ahh, I see, so anything between script tags wouldn’t be parsed? I have to say I disagree with that idea. What if someone had information that was stored in a Channel and used by Flash? They would then need to output any variables to pass into Flash using script tags, so it would need to parse the EE tags with the script tags. Wes

Ok, I had lightly thought about that sort of thing at the moment, and with your example would agree after all that there are necessary cases.

When we discuss it (!), then these sort of things come out of course. I hadn’t realized that there were situations about such as that nearly-EE-embed tag which Bjorn’s example has thrown up, which has involved EE parsing itself in these don’t-zap-Javascript problems.

I think you are discussing not parsing included files as a workaround for the near-EE-tag problem, until the parser can be more accurate.

In that case, I’d think that end-of-parse-unsubstituted-variable-zapping ought to be similarly not done on included files.

Then the use of script/script tags should be recognized only for preventing the end-of-parse-unsubstituted-variable-zapping.

It does get complicated when the parser itself gets involved, not a doubt. I guess I would see it fixed.

Also by this point, I think I would just remove that end-of-parse-unsubstituted-variable-zapping code. It’s about one line, the ill-fated regex, and I don’t think there’s supposed to be any state where it’s called as of 2.2.2 and no-debug-zero. Except, it will be called, on all legacy sites that upgrade having debug-zero set. That’s the big recurring problem you want to kill, if you listen to others, I think.

Kill that problem, and you’re left with the question of parser accuracy. That’s a clean fix then, and should not be so involved.

What have I missed?

Caught your intermediate note, Wes, thanks.

Note that my final suggestion, just to get rid of the apparently-unparsed-eevars code, greatly simplifies and definitely will not negatively impact performsnce.

That is all 😉

Back later to see if any further.

Regards, Clive

Yup, the zapping of unparsed-tags is a different issue and not done by the template parser, but in EE.php .. I also think it should be removed, unless there are very good reasons for keeping it.

Yup, the zapping of unparsed-tags is a different issue and not done by the template parser, but in EE.php .. I also think it should be removed, unless there are very good reasons for keeping it.

Well, the best reason to use this is when you’re using your entries tag pair for two channels and you just have all of the fields there. Some fields will exist for one channel and some will exist for the other and you only want to see the valid fields. Obviously you can also use if statements, but there are a good number of sites that already use this behavior (and a handful of others that depend on the tags being removed) so for the time being it’ll remain this way. Plus, with the next minor version of ExpressionEngine, you’ll have the ability to override this behavior if you so choose.

I’ve already explained this to Clive and also let him know why it’s not changing until a major version bump.