This was an annoying glitch in the aggregator API.

Other functions go aggregator_[verb]_[thing being verbed], but for whatever reason load doesn't. Let's fix that.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, aggregator-load-order.patch, failed testing.

webchick’s picture

Issue tags: +Novice

Meh. I don't actually care enough to fix whatever's causing test failures myself, but I bet it's something stupid.

Tagging "novice".

kathyh’s picture

Assigned: Unassigned » kathyh

Assigned to kathyh. Will take a look.

kathyh’s picture

Status: Reviewed & tested by the community » Needs work

Novice notes:
GET ISSUE
-Select Issue from queue that is tagged with "Novice":
http://drupal.org/project/issues/search/drupal?issue_tags=Novice
-Assigned it to myself

IF IT COMES WITH A PATCH - APPLY PATCH
-downloaded new *AMP stack to get the php version for 8.x and then installed d8 site
-downloaded greasemonkey to better REVIEW the patch in the issue http://drupal.org/project/dreditor
-get a copy of drupal8 from git:
git clone http://git.drupal.org/project/drupal.git drupal8 (where drupal8 is the dir for the repo)
git checkout -b local 8.x
-to use the patch (from http://drupal.org/patch) downloaded cygwin
-install patch module and utils/cygutils (to get the unix2dos utility)
with the patch module to run: patch -p1 < patchfile

FIX THE ISSUE
-now that I have the patch, looking at test dir in the aggregator dir to see which test fail and why (e.g. fixing the issue)
-ask questions in #drupal on IRC http://webchat.freenode.net/
-ask questions in #drupal-contribute - see http://groups.drupal.org/node/175539

CREATE THE NEW PATCH
-after done will look at Making a Drupal Patch with GIT http://drupal.org/node/707484 - run unix2dos on files
git config --global core.autocrlf true
git config --global core.safecrlf true
git diff > aggregator-load-order-3.patch

UPLOAD PATCH AND TEST
-switch the status to "needs review" and upload the patch. This will wake up the test-bot... eventually.
-if it passes, then leave status as "need review" and wait for RTBC

kathyh’s picture

-function aggregator_load_feed removed from patch (left as *_load), uploading for test

kathyh’s picture

-re-upload (status was ignored)

kathyh’s picture

Status: Needs work » Needs review
FileSize
2.47 KB

ok - google says I need to change status to needs review - let's see if this calls the test bot

marvil07’s picture

Status: Needs review » Needs work

@kathyh: Thanks for providing the patch!

There are 3 functions that match /aggregator_*_load/: aggregator_feed_load(), aggregator_category_load() and aggregator_feed_items_load().

The problem with renaming the first two functions is that they both are menu wildcard loaders, and sadly there is not a way to set manually the callback name for them, so they should not be removed.
Another possibility is to declare the right-named functions, and there call the menu wildcard loader functions manually, but that seems even more confuse to me.

--- a/modules/aggregator/aggregator.module
+++ b/modules/aggregator/aggregator.module
@@ -684,7 +684,7 @@ function aggregator_feed_load($fid) {
  * @return
  *   An associative array describing the category.
  */
-function aggregator_category_load($cid) {
+function aggregator_load_category($cid) {
   $categories = &drupal_static(__FUNCTION__);
   if (!isset($categories[$cid])) {
     $categories[$cid] = db_query('SELECT * FROM {aggregator_category} WHERE cid = :cid', array(':cid' => $cid))->fetchAssoc();

@kathyh: Based on the reasoning above, this hunk need to be removed, and after it, I guess this issue will be RTBC ;-)

kathyh’s picture

Status: Needs work » Needs review
FileSize
1.95 KB

@marvil07 - thanks for the feedback and help in #drupal - much appreciated.
Change was to remove menu wildcard loaders from the patch. Resubmitting.

marvil07’s picture

Status: Needs review » Reviewed & tested by the community

As mentioned on #8, IMHO this is ready now :-)

Dries’s picture

Status: Needs work » Fixed

Committed to 8.x. Thanks.

Eric_A’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Reviewed & tested by the community

The log indicates that this was committed to D8 and D7. The D7 commit must have been accidental.

Eric_A’s picture

Priority: Minor » Critical
webchick’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Fixed

Oh dear. :)

Rolled back in 7.x. Thanks for escalating, Eric_A, and thanks for the patch kathyh!!

Tor Arne Thune’s picture

Issue tags: +Needs change record

This needs a change notification, no?

pillarsdotnet’s picture

@Tor Arne Thune:

This needs a change notification, no?

Yup. Follow your own link and create one; it's not hard. ;-)

Tor Arne Thune’s picture

Well, seeing as the issue is marked Novice, I was hoping to give a hint to someone stumbling upon the issue that has never done it before.

webchick’s picture

Title: s/aggregator_*_load/aggregator_load_*/g » API change notice for s/aggregator_*_load/aggregator_load_*/g
Status: Fixed » Active
rickmanelius’s picture

Status: Active » Needs review

For your review (this is my first change notice submitted... so let me know if I mucked something up).
http://drupal.org/node/1295398

marvil07’s picture

It look fine :-), maybe(because target is developers for this change notice) mentioning that some functions matching the pattern mentioned on this issue title end up not changing because they are menu wildcard loaders(see comment 8 for more information) is relevant. Otherwise I guess it's ok, but I'm not an expert on change notices ;-)

pillarsdotnet’s picture

Title: API change notice for s/aggregator_*_load/aggregator_load_*/g » Rename aggregator_feed_items_load() to aggregator_load_feed_items().

Rewrote the change notice. Only one function was renamed, although three were considered.

Also posted comments to 7.x and 8.x API pages.

marcingy’s picture

Title: Rename aggregator_feed_items_load() to aggregator_load_feed_items(). » API change notice: Rename aggregator_feed_items_load() to aggregator_load_feed_items().

Readding change notice part of issue title so people realise that is all that is outstanding

pillarsdotnet’s picture

Title: API change notice: Rename aggregator_feed_items_load() to aggregator_load_feed_items(). » Rename aggregator_feed_items_load() to aggregator_load_feed_items().
Status: Needs review » Fixed

Huh? No; the change notice has been written.

Status: Fixed » Closed (fixed)

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

xjm’s picture

Priority: Critical » Normal
Issue tags: -Novice, -Needs change record