Rename FeedsSource->feed_nid to FeedsSource->id and FeedsSource->id to FeedsSource->importer_id.

Alternatively we could rename

FeedsSource->feed_nid to FeedsSource->source_id and keep FeedsSource->id the way it is.

I prefer the former as it 'id' without further specification looks like the id of the object it is found on. Not so the case right now, where FeedsSource->id points to the importer id.

Comments

alex_b’s picture

Forgot to mention: feed_nid should go completely as a source can be attached to anything, not just nodes. (UI does not allow this yet, but I am successfully attaching sources to users).

alex_b’s picture

Issue tags: +beta blocker
alex_b’s picture

Issue tags: -beta blocker

This is postponed to Drupal 7.

zhangtaihao’s picture

Coming from #917678: Refactor code to use arbitrary types of feeds source in addition to nodes.

If I were to give this a shot for D6, would you look at it?

zhangtaihao’s picture

Just in case your answer is yes, would it be possible to set up a branch point for, say, DRUPAL-6--2?

alex_b’s picture

This is a lot of work to get done for D6 and would require a major version move for Feeds. If you're prepared to bite off a large piece of refactoring work and seeing through an upgrade path for it, let's talk.

My gut is telling me that you may get away with a custom solution to the use case you'll need to solve. To be very clear: from an API level it is possible to 'attach' a Feeds source to anything with a serial id. (users, comments, terms, etc.) - there is just no UI for it and you will have to live with the fact that 'feed_nid' is going to reference a user id, comment id, term id.

I'm available on IRC this further.

zhangtaihao’s picture

I'm in Australia so there's probably quite a slim chance we can find each other on IRC (I just tried).

Would you mind terribly if you import DRUPAL-6--1 onto the github repo as a branch so I can fork it and work from there (or you might include me)?

alex_b’s picture

Great call. There you go, just merged in changes from CVS DRUPAL-6--1 right now: http://github.com/lxbarth/Feeds/tree/DRUPAL-6--1

zhangtaihao’s picture

Thanks. I've forked my own DRUPAL-6--2 branch.

alex_b’s picture

I think it's worth revisiting the naming conventions here.

CTools is using 'name' instead of 'id'. Drupal core uses 'machine_name' - I think that's too long. I like 'name' better than 'id' as it is closer to Ctools, D7 and it suggests that the content is a string and not a serial id.

Further, we should refrain from using 'id' as nowhere in Drupal we find an 'id' key but rather keys like 'nid', 'cid', 'fid', etc.

So here's my proposal:

// Rename $id to $name
FeedsConfigurable::$id => FeedsConfigurable::$name
FeedsImporter::$id => FeedsImporter::$name
// At some point in the future: FeedsImporter::$config['name'] => FeedsImporter::$config['title']
FeedsSource::$id => FeedsSource::$name
// + Every other class derived from FeedsConfigurable

// Rename $feed_nid to $sid.
FeedsSource::$feed_nid => FeedsSource::$sid
zhangtaihao’s picture

Your proposal seems logical. I will roll those changes in.

I should mention that it won't matter too much what we rename anything to, since there would have to be an upgrade path tailored for any renaming anyway.

Also, I've reopened #917678: Refactor code to use arbitrary types of feeds source in addition to nodes so we can focus on the renaming issue here and keep other refactoring matters in that issue.

EDIT:

I'm not sure how the versioning thing works in d.o, but is it possible to create a new version (or branch) for this project here, i.e. 6.x-2.x, without uploading a release? I feel uncomfortable writing about this issue and that as 1.x issues.

zhangtaihao’s picture

Version: 6.x-1.x-dev » 7.x-2.x-dev

Changing this issue to D7 2.x.

zhangtaihao’s picture

Version: 7.x-2.x-dev » 6.x-1.x-dev

I'm sorry for spamming this issue. I think I should still give this a try from the D6 end.

zhangtaihao’s picture

Everywhere I turn I run into the vast amount of functionality tied into the Node API, especially CRUD mechanisms. There is also significant integrated functionality for other modules, e.g. OG, Locale, Taxonomy, etc. It's not worth doing for D6 just yet. I'm now thinking what you did in #3.

However, if I'm not mistaken, 7.x-2.x is still tightly coupled to the Node API (and for that matter depend on nodes). Might there be a chance to refactor it to use the Entity API for CRUD instead? Integrated functionality can at some point be shifted down to Node subclasses of everything. I'm guessing this wouldn't be feasible for 7.x-2.x as that branch is obviously the poster child for D7 (and thus needs to be released as quickly as possible).

Perhaps in 7.x-3.x?

alex_b’s picture

Title: Rename FeedsSource->feed_nid to FeedsSource->id and FeedsSource->id to FeedsSource->importer_id » Rename FeedsSource->feed_nid to FeedsSource->sid and FeedsSource->id to FeedsSource->name

Renaming according to #10

alex_b’s picture

Title: Rename FeedsSource->feed_nid to FeedsSource->sid and FeedsSource->id to FeedsSource->name » Rename FeedsSource->feed_nid to FeedsSource->sid and FeedsImporter->id to FeedsImporter->name

This is better.

alex_b’s picture

Might there be a chance to refactor it to use the Entity API for CRUD instead?

Yes, this could be done. One thing I'd like to avoid however, is to use the Field API for attaching a Feeds importer to an entity. Reasons: Field API has a swappable storage back end that introduces all kinds of complications when it comes to accessing data as you can't assume that it's sitting in your SQL database.

This would be all possible in 7.x 2.x as long as it is still an alpha branch.

kenorb’s picture

Issue summary: View changes
Status: Active » Closed (outdated)

Closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.