when using js aggregator
og panels breaks:
- go to panel_content
- click some [+] to insert new pane
result: browser goes back to og_panels page
expected: popup to select new pane

Comments

geek-merlin’s picture

Project: Organic Groups » Javascript Aggregator
Version: 5.x-6.1 » 5.x-1.2
Component: og_panels.module » Code

this seems to be a aggregator bug.

geek-merlin’s picture

Status: Active » Fixed

and upgrading to 1.2 solves this for me.

geek-merlin’s picture

Status: Fixed » Needs review

NO, not solved. there is an issue.

in panels/js/display_editor.js there is a comment

/** add content button **/

which contains extra stars.
this will be replaced with a star, which breaks js code.

the offending code is in line 72 of javascript_aggregator.module

          $data = preg_replace('<
            /\*([^*\\\\]|\*(?!/))+\*/    # Remove comments
            >x', '\1', $data);

removing the replacement parameter (which does not make sense to me) solves the issue here.

          $data = preg_replace('<
            /\*([^*\\\\]|\*(?!/))+\*/    # Remove comments
            >x', '', $data);
geek-merlin’s picture

Memo:
this is the regexp in a more readable form.

          $data = preg_replace('<
            /\*
              (
                [^*\\\\]  // NOT star or backslash
              |
                \*(?!/)  // star but no slash afterwards
              )+
            \*/
            >x', '', $data); # Remove comments

(what does the backslash do there?)

i wonder why not just this

          $data = preg_replace('<
            /\*
              .*?
            \*/
            >x', '', $data); # Remove comments
derjochenmeyer’s picture

Hey aexl, thanks a lot for posting the code...

i am a total ignorant when it comes to regular expressions... that might explain it... i used this code because someone suggested it... do you think it makes sense to use your version and release a new version? i know enough to tell that your version looks convincing... but im not into that enough to tell the first version makes absolutely no sense... if you know what i mean... i am a regex pedestrian ;-)

geek-merlin’s picture

OK good to know that the things i dont understand contain no secret magic wisdom (but we can of course never be sure ;-)

just sat down a while and thought about it.
then i studiesd some other code. (here is some nice code from the wise python folks.

conclusion:
comment removal always needs a minimal parser, to first rule out string literals!

so we might
a) port the code from the link above
b) backport js aggregator code from drupal 6
c) or live without comment removal until d6

for the moment i choose c)

;-) aexl

geek-merlin’s picture

Title: JS Aggregation breaks og panels » JS Aggregator comment removal breaks javascript
derjochenmeyer’s picture

hm ... i tend to do the same (c) and leave the module as it is ... the comment removal minifies the js files only by 1-2% ... so i dont use the comment removal myself ... i think the main benefit is to reduce sever requests from 10+ in ideal cases to 1 ...

geek-merlin’s picture

i think there should be a new release which strips off that functionality or at least has a strong warning.
what do you think?

derjochenmeyer’s picture

Status: Needs review » Fixed

its done, i released a new version a second ago ... thanks aexl, for your feedback and your help ... i made the changes suggested in comment 4 (http://drupal.org/node/247384#comment-812587) and added a slight warning that this option might cause problems.

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.