SimplePie 1.x

I'd like to take a stab at fixing this bug if its ok with everyone. Since this will be the first time using svn I'd like to make sure I do it correctly. I've installed TortoiseSVN, checked out the SP trunk, made the changes as I described above, and ran the unit tests. All the unit tests show the same result before and after my patch. I also ran some specific tests related to this bug and all passed. Is there anything else I should do? Do I just do a commit on simplepie.inc now?

proposed patch:

Index: simplepie.inc
===================================================================
--- simplepie.inc    (revision 973)
+++ simplepie.inc    (working copy)
@@ -2933,7 +2933,7 @@
         {
             if (!empty($this->multifeed_objects))
             {
-                $this->data['items'] = SimplePie::merge_items($this->multifeed_objects, $start, $end, $this->item_limit);
+                $this->data['items'] = $this->merge_items($this->multifeed_objects, $start, $end, $this->item_limit);
             }
             else
             {
@@ -3061,7 +3061,15 @@
             $item = null;
             if ($do_sort)
             {
-                usort($items, array('SimplePie', 'sort_items'));
+                // if called by multifeed, use $this, else, use SimplePie
+                if(isset($this))
+                {
+                    usort($items, array($this, 'sort_items'));               
+                }
+                else
+                {
+                    usort($items, array('SimplePie', 'sort_items'));
+                }
             }

SimplePie::merge_items() is a static method — we shouldn't be calling it dynamically. We can't change it to a dynamic method without breaking API compat., thus it must remain as is. If you want to add a way to over-ride it, it needs to be done some other way.

But doesn't the patch handle the dynamic/static problem by checking for a null $this?

- usort($items, array('SimplePie', 'sort_items'));
+ // if called by multifeed, use $this, else, use SimplePie
+ * if(isset($this))*
+ {
+ usort($items, array($this, 'sort_items'));
+ }
+ else
+ {
+ usort($items, array('SimplePie', 'sort_items'));
+ }

Yeah now that I think about it, it has to be done another way. Although it does work, it's not a good long term solution.

Take a look at how the other set_*_class() methods are implemented. SimplePie_Misc should be the default class, but people can override which class to use by setting $feed->set_misc_class('SimplePie_Misc_Custom');

That's not the dynamic/static problem: the problem is that on PHP 5 calling a non-static method dynamically causes an E_STRICT (and we should be able to just add the static keyword to what is defined as being static in comments to remove all those errors).

If you don't mind, I'll take a stab at this on the weekend. So much to do, so little time...

No problem, I could use the help. I was thinking the best solution would be to just duplicate merge_items, call it merge_items_internal and change SimplePie::sort_items call to $this->sort_items. Then change the static call to SimplePie::merge_items when processing multifeeds to $this->merge_items_internal. This would enable sort_items to be overridden for multifeeds, while at the same time keeping the API intact since the static merge_items method will still be there. What do you think?

Assigning this to me, I'll get around to it at some point.

This isn't an urgent bug fix, and so shouldn't be 1.1.2.

What-say we move the sorting method to SimplePie_Misc and wait for #98?