HTTPFetcher autodiscovery

Scott:

I hate to say it because I see all the work you've done and I'd LOVE to have auto discovery back (it worked in FeedAPI), but:

auto discover needs to go into libraries/http_fetcher.inc . I *really* want to keep non Feeds specific (integrational) functionality outside of the plugins and in libraries, this is functionality that could potentially even live outside of Drupal!

I'm on IRC if you have more questions.

#4 It *is* a monster :-) I've pulled it over basically 1:1 from FeedAPI, FeedAPI used it from Aggregation (if I remember correctly), so it's oold. Would love to rewrite it if I had some time.

That would mean the batch importer would need to know if it i should auto discover.

I'm not sure whether I follow. Couldn't http_request auto detect a feed, download it when found and return its data?

#7: I follow now. You'd like to essentially avoid doing auto discovery on every import, but rather you'd like to resolve the actual feed URL once and then keep using this actual feed URL rather than the original URL - correct?

Where do you plan to store the resolved URL? Would you replace the original URL with the resolved URL?

And I think there is a reason why FeedsImportBatch is one master class instead of two separate ones. Care to enlighten me?

Only historical reasons. I agree that it should be broken into separate classes, just didn't have the time to do it. A @todo would be warranted.

Need more time to review the patch.

Just rerolled the patch against HEAD. Not tested.

The patch uses now a checkbox instead of radios which is in line with the choice of the PuSH UI on the same page.

I've vastly simplified the part in FeedsHTTPFetcher and I'd like to discuss whether this covers your use case.

Essentially, the fetcher attempts to resolve the URL on validate if auto_detect_feed is true. What's different is that it modifies the URL once and forever. This keeps things much simpler for the rest of the stack (namely the fetch() method) as there doesn't be an awareness of URLs that need to be resolved before using them. Also edge cases like OPML import where existing URLs are determined by simply looking at the source field of the feeds_source table remain functional.

If the fetcher cannot resolve the URL it fails silently. This is in line with the existing behavior of Feeds where there is no warning or error issued if the feed that is added cannot be downloaded or is not of a specific format (the only error you get is that the title cannot be resolved). URL (input) validation is something that we should keep separate from this patch if possible.

What are your thoughts? I'm available to discuss this on IRC if you'd like to.

I did not review much of http_fetcher.inc so far.

So I just thought more about this. I think we should only resolve on feed submission (specifically on sourceFormValidate()). It simplifies much and it's not too confusing for the user. Further, resolving shouldn't present a hard problem for the simplefeeds upgrade script. The feeds API should be available when upgrading, so you simply do a $url = http_request_get_common_syndication($url) for every migrated feed - right?

What I think a good solution would be is use this function on the node-add page. When entering a HTML field, it could detect this, get the first application/rss+xml element, return to the page with a message: "@url is not a valid link. Did you mean @discoveredlink"?

"@url is not a valid link. Did you mean @discoveredlink"?

I'd rather not bother a user with this... If this is desired behavior in a site build, it can be added simply by adding an additional validation step to the node form - right?

Problem with that is that the autodiscovery really is an educated guess method. For example, my Twitter has the links:

<link rel="alternate" href="http://twitter.com/statuses/friends_timeline.rss" title="Your Twitter Timeline" type="application/rss+xml" /> 
<link rel="alternate" href="http://twitter.com/statuses/replies.rss" title="Your Twitter @nielsz Mentions" type="application/rss+xml" /> 

In this case, the first is the one we probably want, but you can never be sure. So an interface letting the user making the final choice could be a solution.

A other interface could be a ajax call wich displays:

did you mean:
1. Your Twitter Timeline [yes]
2. Your Twitter @nielsz Mentions? [yes]

#22: Good point. Only reservation I'd have would be that I don't want to give users choices like "News as RSS" or "News as Atom". But I guess we could handle that.

I would argue that we should first land the "educated guess" approach that Scott started and then deal with an improved UI for auto discovery in a separate issue.

How does this sound?

The "improved UI" would mean that if the detection mechanism finds equal link entries it escalates them to the user. I'd like to avoid AHAH or AJAX, ideally we can run detection on form validation and then either present the same node form with a choice between the found links or a second form. This mechanism needs to work not only on node forms but on standalone forms, too.

Thoughts?

Yes, auto-discovery to take the first URL it finds would be a great step forward. Should it replace what the user entered? That seems to be ok since that would dovetail into a new interface (more on that in a sec). Otherwise, I would propose 2 url fields: user_entered and auto_discovered, but that complicates things.

On autodiscovery, a 2nd patch could then say: more than one feed found, which do you want. If no feed found, it can alert the user and they can't proceed. This would address both an improved experience and prevent bad feeds from being entered. Another patch/issue for this would be needed.

Should it replace what the user entered?

Yes. (see #19)

Let's shoot for a two patch approach and go forward with Scott's patch first. Ball is in my court to take a closer look at #16 then.

I just tested #16 and found that it needed slight reordering in hook_nodeapi() to make the validation hook fire before we are looking for a title.

Then I tried this feed:

http://news.google.com/news/search?aq=f&pz=1&cf=all&ned=en_pk&hl=en&q=dr...

to test auto discovery. The feed URLs detected by the http_request library are not HTML decoded and hence don't work.

We should add some tests that verify HTML documents that we expect to work. The documents to test against should be included in feeds.

Uff. Did you try if you could use form_set_value() in configFormValidate() if you pass $form into configFormValidate() ? If we can do that, this would be ideal. This would essentially require refactoring all instances of configFormValidate() and sourceFormValidate() which wouldn't be too bad.

If we would also need to pass in the full $form_state array into configFormValidate() we'd have to refactor all instances of this method to expect the full $form_state and not the part which is the configurable's own. I'd rather go with a custom element_validate callback in this case.

Hint, look at feeds_config_form_validate() in FeedsConfigurable.inc to see how the validate methods are being invoked.

#31: does title detection work when an HTML page is given?

A problem with this approach is that $source_config in saveSource() is piercing the current paradigm where we don't mix setting with saving methods. Hm... I need to do more thinking/experimenting with this issue, I really want to get this right.

I won't be in the coming week, I hope this patch works for you in the meantime, I'll get back to it in the week of the 15th.

Subscribing

We have a winner.

- Rerolled after no newline cleanup commit broke http_fetcher.inc patch - st00pid CSV :p
- HTTP auto discovery is executed on FeedsHTTPFetcher::sourceFormValidate() now. #740962-5 introduces a fix where changes to $values array in FPlugin::sourceFormValidate() are carried over to subsequent stages by a little workaround in feeds_nodeapi() - the standalone form should just work. Details on #740962.

From my point of view this is RTBC once #740962-5: FileFetcher Attached to Feed Node, Upload Field Not Saving File Path is in.

Scott: could you give this patch a spin and see whether it's working for you? You'll have to apply the patch on #740962-5.

Hm, I guess I shouldn't roll patches so late. The autodiscovery in the save function is completely unnecessary.

[edit: this patch still requires #740962-5: FileFetcher Attached to Feed Node, Upload Field Not Saving File Path to be in place.]

Committed. This is an awesome feature.

http://drupal.org/cvs?commit=348266

Thanks, Will for committing. Thank you Scott for your patience and persistence to see this through.