Posted by webchick on September 1, 2011 at 7:51am
9 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | aggregator.module |
| Category: | task |
| Priority: | critical |
| Assigned: | kathyh |
| Status: | closed (fixed) |
| Issue tags: | Needs change notification, Novice |
Issue Summary
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.
Change records for this issue
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| aggregator-load-order.patch | 2.85 KB | Idle | FAILED: [[SimpleTest]]: [MySQL] 32,767 pass(es), 140 fail(s), and 0 exception(es). | View details |
Comments
#1
The last submitted patch, aggregator-load-order.patch, failed testing.
#2
Meh. I don't actually care enough to fix whatever's causing test failures myself, but I bet it's something stupid.
Tagging "novice".
#3
Assigned to kathyh. Will take a look.
#4
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
#5
-function aggregator_load_feed removed from patch (left as *_load), uploading for test
#6
-re-upload (status was ignored)
#7
ok - google says I need to change status to needs review - let's see if this calls the test bot
#8
@kathyh: Thanks for providing the patch!
There are 3 functions that match
/aggregator_*_load/:aggregator_feed_load(),aggregator_category_load()andaggregator_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 ;-)
#9
@marvil07 - thanks for the feedback and help in #drupal - much appreciated.
Change was to remove menu wildcard loaders from the patch. Resubmitting.
#10
As mentioned on #8, IMHO this is ready now :-)
#11
Committed to 8.x. Thanks.
#12
The log indicates that this was committed to D8 and D7. The D7 commit must have been accidental.
#13
#14
Oh dear. :)
Rolled back in 7.x. Thanks for escalating, Eric_A, and thanks for the patch kathyh!!
#15
This needs a change notification, no?
#16
@Tor Arne Thune:
Yup. Follow your own link and create one; it's not hard. ;-)
#17
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.
#18
#19
For your review (this is my first change notice submitted... so let me know if I mucked something up).
http://drupal.org/node/1295398
#20
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 ;-)
#21
Rewrote the change notice. Only one function was renamed, although three were considered.
Also posted comments to 7.x and 8.x API pages.
#22
Readding change notice part of issue title so people realise that is all that is outstanding
#23
Huh? No; the change notice has been written.
#24
Automatically closed -- issue fixed for 2 weeks with no activity.