Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Aron Novak’s picture

Component: aggregator.module » documentation

only doc, nothing else.
It affects D7 as well, the patch applies cleanly there as well. So backport is not needed, but 7.x and 8.x are both affected.

jhodgdon’s picture

Status: Needs review » Needs work
Issue tags: +Needs documentation updates, +Needs backport to D7

It would be good if this was updated to follow our documentation standards too. This list of keys should be formatted as a list, following this format:
http://drupal.org/node/1354#lists

Also, in order to review this patch, I would need to verify that the documentation change is correct. The only example in core I can see where this hook is implemented doesn't even include ->items in the return value. Can you point me to where I can see this being implemented or used with lower-case array keys? Or the issue where this was changed and the API documentation wasn't? Also, if this was a Drupal 6/7 change, we need to document it on the module update page.

Aron Novak’s picture

Status: Needs work » Needs review
FileSize
2.1 KB

About the correctness of the change, see: http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/modules/ag...
For example in aggregator_parse_feed():

elseif (!empty($item['summary'])) {
  $item['description'] = $item['summary'];
}
elseif (!empty($item['content'])) {
  $item['description'] = $item['content'];
}

That code is called when http://api.drupal.org/api/drupal/modules--aggregator--aggregator.parser.... is invoked:

if (aggregator_parse_feed($feed->source_string, $feed)) {

The requested improvements of the patch are done, so it's NR again.

Aron Novak’s picture

It was a change for Drupal 7: http://drupalcode.org/project/drupal.git/commitdiff/04662d1328bf0dbc9e04...
But before Drupal 7, the architecture of Aggregator wasn't pluggable, so it's not an update (D6->D7) issue.

Also, others are discovered this problem: http://api.drupal.org/api/drupal/modules--aggregator--aggregator.api.php...

jhodgdon’s picture

Status: Needs review » Needs work

Better! A few suggestions:

a)

+ *   An stdClass object containing:

This should be
A stdClass object with the following properties:

b)

+ *   - description - A string to use for the description of the feed 
+ *   - image - A string to use for the image for the feed
+ *   - etag - A string to use for the value of feed's entity tag header field

List items should be set off by : not - as in:
* - description: A string ...
And they should end in a period. Please check the list formatting guidelines on http://drupal.org/node/1354 where this is described.

c) It would be good if this said whether these strings need to be translated.

d) How is the string used as an image -- is it the filepath?

e) Actually, in general I think "A string to use for" should be left out of all of these. So it would be:
* - description: The translated, human-readable description of the field
etc.

f) Should I already know what an "entity tag header" is? I don't...

g)

+ *   - modified - A unix timestamp of the feed's last modified header
  *     field

Should be "The Unix timestamp when the feed was last modified" maybe?

h)

+ *   - items - An array of feed items, the common format for a
+ *     single feed item is an associative array containing:

Grammatically, this is essentially a comma-splice. Should have a . instead of , after items.

i) The comments above apply to the nested list in items also.

Aron Novak’s picture

Status: Needs work » Needs review
FileSize
2.37 KB

Thank you for the suggestions, we can improve the doc text further alongside fixing its the bogus caps thing :)
"translated" - i don't think we should mention it. The parser is parsing an external data file, of course the human-readable texts are in that language what the XML file provides.

jhodgdon’s picture

Status: Needs review » Needs work

Actually, mentioning translated is not a bad idea. Some Drupal functions expect translated text, and a few do not. I don't think that being explicit is bad?

Looking at the current patch -- it needs proof-reading:

a)

+ *   - image: A full <a><img /></a> tag what displays the image of the feed,
+ *     src is an external URL, the link points to the source site. Use l()
+ *     and theme('image') for the rendering.

This needs to be rewritten as an English sentence, rather than a mix of English and HTML. Also, "what" should be "that".

b) respone should be response in the next item, and can you explain what an "ETag" is? I would have no idea.

c) Please re-wrap the lines as close to 80 characters as possible without going over, everywhere in the patch.

d)

+ *     - title: The human-readable title of a feed item.

a -> the

e)

+ *     - guid: The RSS/Atom global unique identifier. This field, together
+ *       with link is utilized tobe able to identify already aggregated items.

- link needs a comma after it
- tobe should be "to be"
- Does it really require both guid and link to identify if an item has already been identified? That seems wrong.

f)

+ *     - link: The original URL of the feed item. Usually it points
+ *       to a specific path within a website.
 

I think the second sentence could be omitted? Doesn't seem to provide information.

Aron Novak’s picture

One quick reply:

- Does it really require both guid and link to identify if an item has already been identified? That seems wrong.

See http://api.drupal.org/api/drupal/modules--aggregator--aggregator.process... .
It uses either guid or link. Maybe it has to be reworded with mentioning 'fallback'

jhodgdon’s picture

Rewording mentioning fallback sounds like a good idea.

Aron Novak’s picture

Status: Needs work » Needs review
FileSize
2.4 KB

"mentioning translated is not a bad idea" - for me, it would mean that the string passed somehow the drupal translation system, which is not the case. Neither we can expect that the text is in a specific language. But you can convince me, is there a place in drupal docs where we say that a text what comes from an external system or from the user, and we have no idea what language it uses and it's not processed by translation modules, and there, we say it's 'translated' human-readable text.

I updated the rest, i tried to be as precise as possible, but i can't say 100% surely that everything is grammatically and stylistically.
So anyone, feel free to improve it.

"can you explain what an "ETag" is?" - I added a link to the RFC. I think saying that it's a field of the HTTP header is really enough. If someone wants to know more, now it's possible to follow the link.

I'm not entirely sure that all the text is stylistically and grammatically correct, i just wanted to fix an error in the docs, not rewriting the whole section :), anyways, anyone is welcomed to do some further language improvements.

jhodgdon’s picture

Status: Needs review » Needs work

The patch will not be marked RTBC (at least by me, and hopefully by anyone else either) unless it is clearly written and free of grammar issues, sorry! If you could put in the grammar fixes I pointed out in the last review, that would be helpful.

Agreed on the other points.

Aron Novak’s picture

Status: Needs work » Needs review
FileSize
2.4 KB

One "what->that" replacement is done in the top of the previous changes (my bad, one point of your notes was skipped). This is the end of my contribution here. If nobody will improve it further, a trivial documentation error won't be fixed, it seems.

Aron Novak’s picture

Status: Needs review » Needs work

oups, marking it as NW

jhodgdon’s picture

Assigned: Aron Novak » Unassigned

I am not in favor of fixing trivial documentation errors in ways that don't follow our standards, sorry! So I'm sad that you don't want to make a patch that will comply. I will go ahead an un-assign this so someone else can work on it.

jhodgdon’s picture

At this point, it's probably a good Novice project to redo the above patch so the doc complies with our doc standards.

kathyh’s picture

Assigned: Unassigned » kathyh

taking a look

jhodgdon’s picture

kathyh: thanks! The standards are at http://drupal.org/node/1354 if you haven't already figured that out. :)

kathyh’s picture

Status: Needs work » Needs review
FileSize
2.88 KB

Header documentation updated per review of above comments and node 1354

Aron Novak’s picture

Status: Needs review » Needs work

Nice work!
One little thing: the etag is nothing to do with HTML, it comes from the HTTP header.
Also, under items:
"timestamp: The UNIX timestamp when the feed was last published."
This one is about the feed item, not the feed.

kathyh’s picture

Status: Needs work » Needs review
FileSize
2.9 KB

Thanks for the feedback - much appreciated. Attached is an update based on comment #20.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! Looks pretty good... One problem though. You cannot have a paragraph break in the middle of a @param or @return, or the API module will assume that the new paragraph is part of the main function description instead of the @param/@return.

Also, the line wrapping in the list area is a bit funky. Please move words up onto previous lines if they would fit in 80 characters. And there is at least one place where there is an extra space after a . (please use just one space, although it violates everything we learned in 8th grade typing class way back when. :) ).

kathyh’s picture

Status: Needs work » Needs review
FileSize
2.91 KB

Updates per #22 and D8 /core dir change

jhodgdon’s picture

Status: Needs review » Needs work
Issue tags: -Novice

The formatting of this patch looks good now, thanks!

I'm not quite sure about the content.

Looking at aggregator_aggregator_parse() and aggregator_parse_feed(), I see:
- 'image' appears to be the image URL, not an HTML tag.
- There's also a 'link' field for the feed - that is not in the documentation, should it be?
- It looks like in an item, 'description' is either the full body text of the item or a summary (depending on what the feed put in there). I don't think "human-readable description" is the right documentation of that.

kathyh’s picture

Status: Needs work » Needs review
FileSize
2.96 KB

Updated per #24. Agreed with 'link' field - added.

jhodgdon’s picture

Status: Needs review » Needs work

That's better. :)

I realized I'm still slightly uncomfortable with how the first part of this reads:

    * @param $feed
[ - lines omitted]
+ *   The $feed object describes the resource to be parsed. The
+ *   $feed->source_string contains the raw feed data which is parsed to expose
+ *   it to other modules as an array of data items in $feed->items. A stdClass
+ *   with the following properties:

Normally, after @param $feed, we wouldn't start by saying "The $feed object...", since that is implied. And I don't think we need "The" with $feed->source_string. And... The way this is written, it's not clear that the properties described are to be added to $feed by this function, rather than expected to be part of the input $feed. So... How about rewriting this something like:

@param $feed
An object describing the resource to be parsed: $feed->source_string contains the raw feed data. The hook implementation should parse this data and add the following properties to the $feed object:

kathyh’s picture

Status: Needs work » Needs review
FileSize
2.92 KB

Updated per #26.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Great! Thanks for all the work on this. I think we now have some actually good documentation of this hook. :)

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks. Committed/pushed to 8.x, will need a re-roll for 7.x.

xjm’s picture

Status: Patch (to be ported) » Reviewed & tested by the community
FileSize
2.9 KB

Here's that backport. Just a reroll of documentation fixes, so this should be RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks!

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