Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

elgg_get_loaded_js() mangles order when priority is the same (Trac #3355) #3355

Closed
elgg-gitbot opened this issue Feb 16, 2013 · 16 comments
Closed

elgg_get_loaded_js() mangles order when priority is the same (Trac #3355) #3355

elgg-gitbot opened this issue Feb 16, 2013 · 16 comments
Labels
engine
Milestone
Elgg 1.8.0

Comments

@elgg-gitbot
Copy link

Original ticket http://trac.elgg.org/ticket/3355 on 41284286-08-03 by brettp, assigned to unknown.

Elgg version: Github Master

I couldn't quickly duplicate this in the unit test, but it's happening on my install in practice. Here is the loaded js in elgg_get_loaded_external_files() before the usort():

array
  0 => 
    object(stdClass)[svn:43]
      public 'loaded' => boolean true
      public 'url' => string 'http://elgg.trunk.com/vendors/jquery/jquery-1.5.min.js' (length=54)
      public 'priority' => int 1
      public 'location' => string 'head' (length=4)
  1 => 
    object(stdClass)[svn:42]
      public 'loaded' => boolean true
      public 'url' => string 'http://elgg.trunk.com/vendors/jquery/jquery-ui-1.8.9.min.js' (length=59)
      public 'priority' => int 2
      public 'location' => string 'head' (length=4)
  2 => 
    object(stdClass)[svn:40]
      public 'loaded' => boolean true
      public 'url' => string 'http://elgg.trunk.com/vendors/jquery/jquery.form.js' (length=51)
      public 'priority' => int 500
      public 'location' => string 'head' (length=4)
  9 => 
    object(stdClass)[svn:1039]
      public 'loaded' => boolean true
      public 'url' => string 'http://elgg.trunk.com/js/tgscalendar/fullcalendar.min.1302729273.js' (length=67)
      public 'priority' => int 500
      public 'location' => string 'head' (length=4)
  10 => 
    object(stdClass)[svn:1040]
      public 'loaded' => boolean true
      public 'url' => string 'http://elgg.trunk.com/js/tgscalendar/gcal.1302729273.js' (length=55)
      public 'priority' => int 500
      public 'location' => string 'head' (length=4)
  11 => 
    object(stdClass)[svn:1041]
      public 'loaded' => boolean true
      public 'url' => string 'http://elgg.trunk.com/js/tgscalendar/tgscalendar.1302729273.js' (length=62)
      public 'priority' => int 500
      public 'location' => string 'head' (length=4)
  18 => 
    object(stdClass)[svn:1192]
      public 'loaded' => boolean true
      public 'url' => string 'http://elgg.trunk.com/js/elgg.1302729273.js' (length=43)
      public 'priority' => int 10
      public 'location' => string 'head' (length=4)

And here it is after the usort:

array
  0 => 
    object(stdClass)[svn:43]
      public 'loaded' => boolean true
      public 'url' => string 'http://elgg.trunk.com/vendors/jquery/jquery-1.5.min.js' (length=54)
      public 'priority' => int 1
      public 'location' => string 'head' (length=4)
  1 => 
    object(stdClass)[svn:42]
      public 'loaded' => boolean true
      public 'url' => string 'http://elgg.trunk.com/vendors/jquery/jquery-ui-1.8.9.min.js' (length=59)
      public 'priority' => int 2
      public 'location' => string 'head' (length=4)
  2 => 
    object(stdClass)[svn:1192]
      public 'loaded' => boolean true
      public 'url' => string 'http://elgg.trunk.com/js/elgg.1302729273.js' (length=43)
      public 'priority' => int 10
      public 'location' => string 'head' (length=4)
  3 => 
    object(stdClass)[svn:1040]
      public 'loaded' => boolean true
      public 'url' => string 'http://elgg.trunk.com/js/tgscalendar/gcal.1302729273.js' (length=55)
      public 'priority' => int 500
      public 'location' => string 'head' (length=4)
  4 => 
    object(stdClass)[svn:40]
      public 'loaded' => boolean true
      public 'url' => string 'http://elgg.trunk.com/vendors/jquery/jquery.form.js' (length=51)
      public 'priority' => int 500
      public 'location' => string 'head' (length=4)
  5 => 
    object(stdClass)[svn:1041]
      public 'loaded' => boolean true
      public 'url' => string 'http://elgg.trunk.com/js/tgscalendar/tgscalendar.1302729273.js' (length=62)
      public 'priority' => int 500
      public 'location' => string 'head' (length=4)
  6 => 
    object(stdClass)[svn:1039]
      public 'loaded' => boolean true
      public 'url' => string 'http://elgg.trunk.com/js/tgscalendar/fullcalendar.min.1302729273.js' (length=67)
      public 'priority' => int 500
      public 'location' => string 'head' (length=4)

It looks like we'll need to add more info for the usort to include load order. The solution is currently to manually specify priority for js that requires load order.

(Set at high because this is very problematic and a pain to debug if loading jquery plugins that require a specific order.)
@elgg-gitbot
Copy link
Author

cash wrote on 41284306-01-06

If JS plugins requires a certain order, they should use the priority value (that's why it is there after all).

The problem is that usort does not preserve original order for items that have the same sort value. This is occurring in both menu items and js/css registration. We could try to adding more logic to the sort function that checks original index for ties.

@elgg-gitbot
Copy link
Author

brettp wrote on 41284320-08-27

Other systems (views, hooks, events) maintain registration order without needing to be explicit so I assumed this one would. That's the problem.

Instead of adding more logic to the sort function, what if we keep a static variable in the registration function that increments if the priority isn't explicitly set. More efficient than additional comparisons?

@elgg-gitbot
Copy link
Author

trac user lie2815 wrote on 41299418-07-02

So should this be fixed? If so, I'd like to do it.

@elgg-gitbot
Copy link
Author

brettp wrote on 41299436-02-25

lie2815 - Yes, this should be fixed. Feel free to take care of it :)

@elgg-gitbot
Copy link
Author

trac user lie2815 wrote on 41299454-10-25

Okay, will do.

@elgg-gitbot
Copy link
Author

trac user lie2815 wrote on 41300233-05-17

Here's your pull request:
#25

@elgg-gitbot
Copy link
Author

trac user tomv wrote on 41384225-11-24

Maybe stupid thought... but an idea: before doing the usort, extend the array with an extra sequential number. And then usort on the combination of this number and the priority ;)

@elgg-gitbot
Copy link
Author

trac user tomv wrote on 41384226-03-20

Maybe stupid thought... but an idea: before doing the usort, extend the array with an extra sequential number. And then usort on the combination of this number and the priority (if priority equal, than compare sequential number in usort function). In that way you don't need extra field.

@elgg-gitbot
Copy link
Author

trac user tomv wrote on 41384232-02-21

Maybe stupid thought... but an idea: before doing the usort, extend the array with an extra sequential number. And then usort on the combination of this number and the priority (if priority equal, than compare sequential number in usort function). In that way you don't need extra field.

@elgg-gitbot
Copy link
Author

trac user tomv wrote on 41384287-01-21

Hm, the 3x posting was unintentional. But, decided to write a piece of code (without testing!) as a suggestion:

function elgg_get_loaded_external_files($type, $location) {
    global $CONFIG;

    if (isset($CONFIG->externals) && isset($CONFIG->externals[$type])) {
        $items = array_values($CONFIG->externals[$type]);

        $callback = "return \$v->loaded == true && \$v->location == '$location';";
        $items = array_filter($items, create_function('$v', $callback));
        if ($items) {
            $i = 0;
            foreach ($items as $value){
                $value->sequential = $i;
                $i += 1;
            }
            usort($items, elgg_external_file_arry_sort );
            array_walk($items, create_function('&$v,$k', '$v = $v->url;'));
        }
        return $items;
    }
    return array();
}

function elgg_external_file_arry_sort($a,$b){
    if ($a->priority == $b->priority) {
        return $a->sequential >= $b->sequential;
    } else {
        return $a->priority >= $b->priority;
    }
}

@elgg-gitbot
Copy link
Author

cash wrote on 41398236-07-14

I'm thinking that a better approach is solving this in the storage rather than on the sort side of things. Make the ordering explicit like it is for plugin hooks and views.

@elgg-gitbot
Copy link
Author

ewinslow wrote on 41398602-01-24

Yea, I agree with Cash. It should be implemented the same way other subsystems are. Seems like we need a data structure for this (like I did for the javascript).

@elgg-gitbot
Copy link
Author

trac user tomv wrote on 41399838-02-11

Fully agree. This was quick and dirty bug fix but seems that is not needed...

@elgg-gitbot
Copy link
Author

cash wrote on 41497335-08-14

I'm thinking a dual index (priority and name).

@elgg-gitbot
Copy link
Author

trac user Brett Profitt wrote on 41639358-09-17

Refs #3355. Added ElggPriorityList.
Changeset: 8a2559a

@elgg-gitbot
Copy link
Author

trac user Brett Profitt wrote on 41645397-12-02

Fixes #3355. Added ElggPriorityList. Adapted the externals system to use it.
Changeset: 5285471

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
engine
Milestone
Elgg 1.8.0
Development

No branches or pull requests
1 participant
@elgg-gitbot