Hi Aron, I list out my observations and questions below:

1) Parser state of the art and tests

I don't have a grip on where we stand with the new parser in comparison the existing aggregator parser. The existing parser bears many many patches that fixed past problems and I'm a little concerned that the syndication_parser will require similar fixes. What do you think? How close are we to implementing unit tests with simple pie and universal feed parser test feeds? How do we look like with RDF? Is there anything that you would like me to explore?

2) The feed aggregator menu (/aggregator) should be part of aggregator_light - I fixed that.

3) add hook_parser/processor('info')

See my comment in the aggregator content type admin form - we should have a 'info' $op that pulls a human readable name and a description for forms etc. from processors and parsers.

4) We should think about merging aggregator_light into aggregator. It's a very small part of functionality that could live entirely (except the aggregator API callbacks) in an .inc file. The advantage is that users don't have to turn on an extra module. Right now, on the module page, aggregator light looks at first glance like an _alternative_ to aggregator. The other advantage would be, that we always _know_ there would be at least one processor there that we could default to.

5) create a default content type - I know this is on your radar - check out how installation profiles do this.

6) on admin/build/node-type/[type]/aggregator page : use field set for processors, use field set for parsers and add help text to the top of it.

7) aggregator.module: while(_aggregator_cron_time() || !$ready) { -> this looks broken

8) Curiosity: how and where is aggregator_admin.inc included?

9) Why do we not use module_invoke_all() in aggregator_feed_save() or in aggregator_feed_retrieve()?

10) aggregator_light_item's schema contains link but no guid, why?

11) for the aggregator_feed table, let's use the schema of the old aggregator...

12) aggregator_light_cron() - could expiry be done in one query?

if ($expiry = variable_get('aggregator_light_clear_' . $type->type, 9676800)){
	DELETE aggregator_light_item ali FROM aggregator_light_item ali JOIN node n ON n.nid = ali.nid WHERE n.type = 'type' AND ali.timestamp > $expiry

13) How close is the new aggregator to the old one? I compared theming templates and saw in aggregator-light-feed-source.tpl.php that $source_icon and $source_image is missing, otherwise we look very close. Can you give an overview on where there are differences between the new and the old aggregator and where not?

14) let's store arrival and published date with aggregator_light items -> drop timestamp, introduce published and downloaded

On a more general note, I think we should try to use the old aggregator's conventions, namings, schemas, templating wherever possible. I think it's perfectly ok that we gutted the thing and replaced it by a simpler and cleaner core, but for every thing we change we should have a good reason.

15) overrideable deduping is still open- this conversation is on the previous review: http://drupal.org/node/277823 - just making a note here.

16) Could you post a list with what's outstanding on the issue tracker? That would be very helpful for me. Thank you.

Nice job again. I like how this is going :)

Comments

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented

1)
The syndication parser now is the union of the best things of feedapi's common_synd and the aggregation's latest parser.
I've not started yet to write the test file for the parser. I have a serious concern about these tests: universal feed parser is a big piece of code. It's rather complex stuff and that's why it could handle almost every well- and malformed stuff.
I don't think that such perfection could be a purpose for the core parser. feedparser.py is 121K. Syndication Parser is now 13K. Syndication Parser cannot provide such parsing precision as feedparser or simplepie, because we want small and slim syndication parser module.
I suggest best-effort viewpoint while concepting about the testing of syndication parser. I'd like to see a tons of real-world feeds in the test suite and add a feed when a problem appears and make the test case to catch that specific bug.

2)
Ok, thanks, right.

3)
Disagree. Reason: core already stores this value. The core should not store redundant stuff. The core should offer a function for accessing to these values. I'm going to provide a patch what makes possible to retrieve these values, i hope it will be accepted :)

4)+
I think this step makes sense. Before the action, let others to tell their opinions.

5)
Ok, thanks for the notice again.

6)
Good point, it makes the form more user-friendly.

7)
Do you mean that the _aggregator_cron_time() function is broken? Because the cron stops nicely, so the condition works good in at least one case (when the feeds are all refreshed). The only possibility is that the _aggregator_cron_time() is broken. i'll double check. The logical condition seems to be good: exit if the time is elapsed or we do not have more feeds to refresh.

8)
drupal_function_exists() does the job.
You do not need to include stuffs at all. In D7, the wonderful registry knows everything about your functions and takes care about including.

9)
One things: variable passing by reference. The key point of the new structure: modules see each others modification in a chain

10)
Because _currently_ deduping is based on link only. It will change. (overrideable deduping)

11)
There are differences... For example: hash, nid. no description. I don't really know what do you mean here. You maybe want the missing fields back: image, etag. The etag is missing for the same purpose as 10).

12)
Yes, it should be one query per node-type.

13)
Do you mean aggregator or aggregator_light? Aggregator_light is very close to the old aggregator. Differences: feed support is replaced by nodes, taxonomy support is added. That's all. But the aggregator module has almost no common parts with the old aggregator.

14)
ok

General note: at the aggregator_light exactly this happened. Almost everything remained the same as the old aggregator.

16)
Here is the todo list: http://drupal.org/node/282570

alex_b’s picture

alex_b commented

1) I agree that it is not realistic to strive to the level of perfection of ufeedparser right now. but the test suite won't hurt, right? That way we know where the problem lies.

3) Where does core store this? If this is on a per module level: don't forget that it is possible to write a module that offers a parser and a processor or that actually offers something completely unrelated to aggregation, but offers a parser on the side... I'm thinking that 'info' should ideally provide a title and a description of the parser/processor exposed. The title and the description would be used on on/off checkboxes.

4) Go ahead on this one if you like.

7) Don't you need to use & in the condition? "If there are still feeds to process AND time is not out"?

8) So module_invoke() is history?

9) But aren't we reimplementing module_invoke_all? http://api.drupal.org/api/function/module_invoke_all/7

10) Let's keep GUID nevertheless, also in the spirit of 14)

11) Let's do the same order. Let's only drop columns we're not using for a good reason anymore (I'm assuming etag is one of them) but let's not drop features like e. g. image.

13) This answers my question. If you would do 4) we could have even the same template file names like before, right?

16) Thank you.

Thanks for your answers.!

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented

1)
Ok, i'll try to adopt that test suite.

3)
This data is in the .info file. I don't think that this is realistic that one module is parser and processor at the same time.

7)
No, definitely not. If the time is over, this means a hard limit!

8)
It seems.

9)
You're right. Objects are passed by reference.

10)
Yes, of course.
13)
Yes.

alex_b’s picture

alex_b commented

3) we discussed this one over IM - we can't exclude the possibility of one module implementing parser and processor, $op = 'info' it is...

7) Ok, cool. Just flew over code and thought it looked wrong.

I'll leave this ticket open as the tasks at hand are still in the works.

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
Status: Active » Fixed

The issues are fixed or included in review3.

Anonymous’s picture

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

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