Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Attached is a patch making minor adjustments to the text in Aggregator module. Mostly just fiddling, there's no big problems here.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | aggregator_text_5.patch | 11.43 KB | keith.smith |
| #8 | aggregator_text_4.patch | 11.21 KB | keith.smith |
| #5 | aggregator_text_3.patch | 11 KB | keith.smith |
| #4 | aggregator_text_2.patch | 11 KB | keith.smith |
| #1 | aggregator_text.patch | 8.91 KB | catch |
Comments
Comment #1
catchEverything looked fine to me, except one t() which had single quotes and an escaped apostrophe instead of double quotes and no escaping. Nitpicking, so rather than set it to needs work I've attached a patch with the change and RTBCed.
Comment #2
Gábor HojtsyIt is not a requirement to have a cron entry point to cron.php actually. You can use poormanscron, or even run cron by hand, if you wish so (eg. in the short initial period you get to know whether the module fits you). It would definitely be better to have some good substitute for this admittedly quite unfriendly 'requires crontab' notes though.
Comment #3
keith.smith CreditAttribution: keith.smith commentedGood point. Thanks for the review. I'll see if I can adjust this in some way.
Comment #4
keith.smith CreditAttribution: keith.smith commentedAttached is the text, which standardizes on a cron message: A correctly configured Cron maintenance task is required to...., where the Cron maintenance task is a link to the /admin/reports/status page (where there is a section for Cron maintenance tasks). If this is an acceptable mechanism for doing this, I'll roll a patch to modify the other similar references to cron in core.
Note that this patch changes instances of "News aggregator" to "Feed aggregator", EXCEPT in the permissions, which still refer to 'administer news feeds' and 'access news feeds'. I'm not sure that I have the fu to change these and provide an update path without help, but I can find a similar example if these need to be changed as well.
I made this change because "News reader" has always sounded odd to me: I understand the origin, but it seems strange especially if the feed items do not represent "news" in the strict sense. Wikipedia says this:
Also, please note:
-- Breadcrumbs have an issue in aggregator, but that is present without this patch. The patch in http://drupal.org/node/170309 cures it.
-- On the aggregator/categories page, I receive the following notices:
for each item displayed. An issue may already exist for this, I have not yet searched. This notice occurs whether the patch is applied or not.
Comment #5
keith.smith CreditAttribution: keith.smith commentedChanged a string delimited [unnecessarily] by " to ', per coding standards.
Comment #6
catchThe cron text is much better. Sending people to status seems sane as well - since they can see if they have such a thing as a "correctly configured cron maintenance task" then only have to find out what it is if it's not working.
So back to RTBC.
Comment #7
Gábor HojtsyA major weaknesses of the text which is not solved by this patch is that RDF and Atom support is mentioned quite late (ie. only on the feed addition page). Otherwise RSS is used which can easily make people think this is only an RSS aggregator. I'd use "syncidation format (such as RSS, RDF and Atom)" instead of simply RSS. I am not attached to this exact wording, but I'd like to see this fixed with the text updates as well. Otherwise all seem to be fine. Thanks!
Comment #8
keith.smith CreditAttribution: keith.smith commentedHopefully the third (or fourth) time will be the trick.
I expanded the discussion in the initial paragraph of the aggregator module help text to include RSS, RDF and Atom formats (and linking to what I thought to be the most authoritative page for each, after a casual search).
New patch attached.
Comment #9
Gábor Hojtsyadmin/content/aggregator still only talks about RSS. Otherwise all looks good as far as I see.
Comment #10
keith.smith CreditAttribution: keith.smith commented*sigh*
Obviously, I've been doing a poor job of reviewing these of late. Thank you for your very good eye for detail!
I've adjusted the text at admin/content/aggregator, borrowing RSS, RDF and Atom text from the help page.
Comment #11
Gábor HojtsyGreat, this one committed now.
It would indeed be great to standardize on this cron information note/link at other places where cron is mentioned (as a requirement). Please inform me about the issue number for this once submitted :) Thanks for your awesome work in making Drupal more understandable!
Comment #12
keith.smith CreditAttribution: keith.smith commentedThanks, Gábor -- you're just swell!
The "Cron maintenance task" issue is http://drupal.org/node/197730, but no patch yet. :( I'll work on it this afternoon, hopefully.
Comment #13
(not verified) CreditAttribution: commentedAutomatically closed -- issue fixed for two weeks with no activity.