Just a place holder. Since it's finally been fixed, I'll try to get a conversion going soon.

There's also a split patch by Crell that needs to be settle first.

CommentFileSizeAuthor
#4 agg_1.patch31.73 KBeaton
#1 tplify_aggregator_1.patch31.73 KBdvessel

Comments

dvessel’s picture

dvessel commented
Title: tplify aggregator module » tplify aggregator module and remove hard coded markup.
Category: task » bug
Status: Active » Needs review
StatusFileSize
new tplify_aggregator_1.patch31.73 KB

This one was really difficult. Every bit of html or the feeds markup was moved into themable functions. The feed functions were not converted into templates, only theme functions. I'd say that's a bug.

A couple of functions were changed and moved around. My head is hurting so I'm not in the best state to try and explain. I tested it throughly and it's darn solid.

Please review.

senpai’s picture

senpai commented

I have the patch installed, and am looking for something to test, but I can't figure out what the original problem was. Help?

senpai’s picture

senpai commented
Assigned: Unassigned » senpai

Assigning it to myself so i can test the theming functions.

eaton’s picture

eaton commented
StatusFileSize
new agg_1.patch31.73 KB

an /em had been swapped with an em tag, resulting in eighty-jillion instances of italicization. Other than that, I ran it through its paces and it appears to be fine.

dvessel’s picture

dvessel commented

This one was really difficult. Every bit of html or the feeds markup was moved into themable functions. The feed functions were not converted into templates, only theme functions. I'd say that's a bug.

That didn't sound right so to clarify, the rss and ompl feed functions were moved into themable functions and not to templates. So, the hard codedness was a bug and this patch fixes it.

The other parts for generating the feeds pulled its data from a form. Very hackish so a new function "aggregator_feed_items_load" was created just to generate the feeds outside the form function.

A wrapper template is also used. All output is wrapped in this. Previously a div with the id of "aggregator" was repeated in many functions and hard coded.

A handful of functions were renamed for clarity, this should not affect anything else.

And thanks Eaton, Senpai for reviewing.

dvessel’s picture

dvessel commented

Any objections on the changes? I'm so tempted to RTBC this..

senpai’s picture

senpai commented

No theming issues to report. CSS changes, node.tpl changes, and such all work as advertised. I did, however, see a couple of PHP notices while adding new feeds to aggregator module during my testing with this tplify_aggregator_1.patch applied.

# notice: Undefined index: fid in /Websites/drupalHEAD/modules/aggregator/aggregator.module on line 561.
# notice: Undefined index: fid in /Websites/drupalHEAD/modules/aggregator/aggregator.module on line 565.
# notice: Undefined index: fid in /Websites/drupalHEAD/modules/aggregator/aggregator.module on line 568.

Should this become a new issue for the module?

senpai’s picture

senpai commented
Status: Needs review » Reviewed & tested by the community

Checked again on a fully up-to-date HEAD, and no Notices, so I'm RTBC this one.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

Thanks for the patch, committed!

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)