script declarations are duplicated when caching enabled

[stutteringp0et]

This was opened in joomlacode and immediately closed with a suggestion that the issue be opened here. After searching, I suppose that the original submitter just gave up.

See it here: http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=26551

Recreate the issue:

1. enable caching - either will do
2. use a module to add a script declaration ( JFactory::getDocument()->addScriptDeclaration("alert('hello');"); )
3. reload the page a few times.

You'll notice in source that you have more than one " alert('hello'); "

I've resorted to enabling checks in my extensions to determine if they've already run or not.

[adriansi7]

I have noticed the same issue today with my Joomla 1.7.3 . After doing some tests i found a workaround :

In libraries/joomla/cache/cache.php :

Under ( public static function setWorkarounds ) i have changed $loptions['nohead'] = 0 ; to 1

And now there are no more duplications but the head section doesn't get cached .

I really hope this issue will be fixed .

Thank you !

[stutteringp0et]

Trying to figure out how to set the nohead option without editing a library file.

[pascal06n]

Still present in joomla 2.5.4. For instance, it appears when you use the caption feature (img .caption) you'll get several script running so several captions.

[stutteringp0et]

I wonder if this can be fixed with a system plugin that scans through the scripts looking for (and removing) duplicates... Since the platform-developers haven't seen fit to even classify or comment on this issue, it must not be that important to them... Or maybe I pissed someone off and my report is being ignored (a distinct possibility - I can be abrasive)

[elinw]

I had a number of conversations with people at Joomla and Beyond with issues around javascript caching.
@adriansi7 If that particular fix works for you why don't you just fork and edit the file and send a pull request?

@stutteringp0et The fact is that the platform maintainers rarely read the issue list .... usually developers submitting issue reports would be working on code to correct the issue and if they weren't sure how they might post on the platform list. When their code was finished they would send it as a pull request but that has not happened in this case. If you want to have a general discussion of the topic the platform list is probably the way to go.

[pascal06n]

Thx elinw for this quick feedback. well what @adriansi7 proposed is not a fix but a good workaround. This issue remains open even though this workaround is part of next release. I haven't implemented it, but i'll do it soon.

I'm very new in the joomla debugging area. But every developper (as I am but in embedded firmwares) are not always taking into account feedback from "normal" people :) At least this problem seems easy to reproduce. I don't know the priority of the bug.

You said you had discussions with people at Joomla and Beyond with issues around caching : what was the result of your discussions ? especially on this issue ?

I'm just here for raising problems (especially mine:) , and helping joomla developper :)

[klas]

If head doesn't get cached any modules adding to the head will get unfunctional when they use cache.

This can be fairly easily solved by having few checks in document addScriptData / mergeHeadData methods to check for duplicates.

[LouisLandry]

So there are several things at play here. Firstly, @stutteringp0et my apologies for the non-responsiveness. As @elinw points out we've not been too good about keeping track of the issue list. Secondly, one of my major problems with the Joomla caching package is that it tries to do entirely too much. In doing so it creates a whole world of problems that are really challenging to work around. So challenging in fact that we have a actual class methods like setWorkarounds, etc.

Most of the maintainers (I can't speak for all) are looking for a more simplified approach to caching where we really only deal with get() and set() kinds of things, and leave the application logic to the application. Because of this I'm much less likely to make a change to an existing "legacy" section of the code and potentially introduce regressions. That being said I'd be happy to review a pull request if any of you find a relatively clean way of dealing with this problem. It just isn't going to be more of a priority than actually reworking the cache package itself.

I'm going to close the issue, and if any of you want to take a crack at a pull request to fix this please feel free. Again, I'd be happy to review it.

[liluxdev]

Hi guys i just fixed that: setWorkarounds wasn't doing the diff well, it used to do only array_diff to detect what a module added to the document head, but the script and the style object are exceptions that needs a string level diff to detect what was appended by a module.

This logic was already implemented in the JDocumentHTML->mergeHeadData() that is used to rebuild the head from the "cache pieces" so i think my fix matches the originally wanted behaivor of cache.php by you Joomla Developers.

So please review my pull request: https://github.com/joomla/joomla-cms/pull/2150

[Denitz]

Please check new patch in http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_id=8103&tracker_item_id=26551

It's based on Stefano's pull but is simpler and follows coding rules.

[liluxdev]

Thanks @Denitz, yes my contribute was just to identify the where the problem was (it was an hard debbugging session! ;) ) but it's written to be readable not optimized, so you Joomla! developers please optimize it as you want!

Anyway I've to give your a feedback your review:

        if (($now == 'script' || $now == 'style') && is_array($newvalue) && is_array($options['headerbefore'][$now]))
                        {
                            foreach ($newvalue as $type => $currentScriptStr)
                            {
                                if (isset($options['headerbefore'][$now][strtolower($type)]))
                                { 
                                    $oldScriptStr = $options['headerbefore'][$now][strtolower($type)];
                                    if ($oldScriptStr != $currentScriptStr)
                                    {
                                        // Save only the appended declaration.
                                        $newvalue[strtolower($type)] = JString::substr($currentScriptStr, JString::strlen($oldScriptStr));
                                    }
                                }
                            }
                        }

merging the first if conditions and negating my "happy" empty if block is super-correct (it's filled with a comment to explain just to explain you why you should put a negation here). Anyway for the rest I'm not sure your code can replace mine with the same effects (Anyway now I can be wrong we have to test it deeply) but if i remember well you can't replace this:

if (!stristr($OldScriptStr, $currentScriptStr)) {
         //save only the appended string in the cache entry for this module
          $diffString = str_replace($OldScriptStr, "", $currentScriptStr);
          $newvalue[strtolower($type)] = $diffString;
 }

with this:

if ($oldScriptStr != $currentScriptStr)
{
        // Save only the appended declaration.
        $newvalue[strtolower($type)] = JString::substr($currentScriptStr, JString::strlen($oldScriptStr));
}

in particular what I think is wrong is the != instead of the stristr surely you can use strstr or strpos but not != because if you debug you will se that $oldScriptStr can contain the current snippet but not equals it (as $oldScriptStr that is $options['headerbefore'][$now][strtolower($type)]; contains a bunch of cached inline scripts all merged in a string, this is the main issue!!!). Instead I'm sure you can replace my str_replace($a,"",$b) with something more optimized but not the substr at start! Anyway I can be wrong, time is past after my patch so please test your one deeply and if is correct go with that that is a lot computationally faster!

Update, I was wrong with these points, it can never happen, i was confused and created confusion: Denitz patch is working perfectly

[liluxdev]

Now I updated the PR applying some of your optimizations: https://github.com/joomla/joomla-cms/pull/2150/commits

[Denitz]

It's not correct to use stristr() because we can't rely on case-insensitive differences, it should be case-sensitive. I agree now that using != is not fine, so better use strpos:

if (JString::strpos($OldScriptStr, $currentScriptStr) !== false) {

It's not save to use str_replace() because it's not UTF friendly, I can suggest to use strtr() - it's multi-byte safe and fast:

$newvalue[strtolower($type)] = strtr($currentsnippet, array( $oldinlinebuffer => '' ));

[liluxdev]

@Denitz first of all many thanks!

Yes my stristr was absolutely a nonsense, and I didn't know that str_replace and standard strpos can give problems with encodings! So thank you I updated the code using your suggestions and i'll remember them for any PHP/Joomla coding in the future.

[liluxdev]

please use last commit: using strpos() !== false we inverted the original logic that was a check of kind "string NOT contains" (originally it was a !stristr) so now is strpos() ===false and it works correctly, tested in Joomla 2.5.16 (only the diff added after $newvalue line) and in Joomla 3.2, against an environment with the reproducible issue and it fixed it

[liluxdev]

Sorry all, I was wrong: the first @Denitz improved version is the perfect solution! My original code has potential issues and is less optimized, sorry if I created confusion but I was paranoically figuring out a situation that can never happen in the Joomla cache system (now I'm sure)! Now my patch uses first @Denitz code that is safer and more performant! So please use last commit. Thanks again @Denitz I apologize for the wrong point I issued.