Would be nice to have the URL and other fields of the feed to map.

D7 verion of #735530: Map feed Title and feed URL (not feed item title and url) (kept this separate as architecture changed so no plain backport)

CommentFileSizeAuthor
#11 map_feed_title_and_feed-1689374-11.patch3.34 KBNikolay Shapovalov
#11 interdiff_6-11.txt2.11 KBNikolay Shapovalov
#10 map_feed_title_and_feed-1689374-10.patch3.35 KBNikolay Shapovalov
#10 interdiff.txt2.12 KBNikolay Shapovalov
#6 map_feed_title_and_url-1689374-6.patch2.55 KBRoSk0
#2 0005-implemented-1689374-Map-feed-Title-and-feed-URL.patch2.88 KBgeek-merlin
#1 0001-implemented-1689374-Map-feed-Title-and-feed-URL.patch2.64 KBgeek-merlin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin commented
Status: Active » Needs review
FileSize
0001-implemented-1689374-Map-feed-Title-and-feed-URL.patch2.64 KB

and here's the implementation. only testing url thoroughly for now.

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin commented
FileSize
0005-implemented-1689374-Map-feed-Title-and-feed-URL.patch2.88 KB

here's the latest.

RoSk0’s picture

RoSk0
he/him
Primary language Russian
Location Wellington
CreditAttribution: RoSk0 commented

Great job!
Tested on 7.x-2.0-alpha7. Works as expected.

Ken Hawkins’s picture

Ken Hawkins CreditAttribution: Ken Hawkins commented
Status: Needs review » Reviewed & tested by the community

This also worked for me on 7.x-2.0-alpha7.

Being bold and setting to reviewed and tested.

twistor’s picture

twistor CreditAttribution: twistor commented
Status: Reviewed & tested by the community » Needs work

There are a few coding standard violations.

I can understand parent:title and feed_source:url, but importer name and description??

RoSk0’s picture

RoSk0
he/him
Primary language Russian
Location Wellington
CreditAttribution: RoSk0 commented
Status: Needs work » Needs review
FileSize
map_feed_title_and_url-1689374-6.patch2.55 KB

Description looks fine to me. Reapplied #2 patch at latest 7.x-2.x-dev. Fixed some code style issues.

twistor’s picture

twistor CreditAttribution: twistor commented

Sure, it looks fine. But at some point we have to ask ourselves what features are useful and necessary, and what's just cluttering things up. I've never even set a description for an importer.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

Patch in #6 works good for me.

fwitw I'm only using the title field but core fields I think would be useful so happy with the ones added by this patch.

favosys’s picture

favosys CreditAttribution: favosys commented

How is this supposed to work?

I tried patches #2 and #6 separately and then I go to:

mysite.com/admin/structure/feeds/my_feed/mapping

And then I click on Mapping in the node processor and I would expect to be able to map the feed title but nothing has changed, there are no new options.

Am I missing something here?

Nikolay Shapovalov’s picture

Nikolay Shapovalov
Primary language Russian
CreditAttribution: Nikolay Shapovalov as a volunteer commented
Issue summary: View changes
FileSize
interdiff.txt2.12 KB
map_feed_title_and_feed-1689374-10.patch3.35 KB

Am I missing something here?

Patch #6 attach new mapping sources only when it attached to contet_type.
I've fix this, also I change option name:
from Feed source: URL to Feed source: Source.

I have test this patch only with HTTP Fetcher, so please test it with files.

Please use patch from #11.

Nikolay Shapovalov’s picture

Nikolay Shapovalov
Primary language Russian
CreditAttribution: Nikolay Shapovalov as a volunteer commented
FileSize
interdiff_6-11.txt2.11 KB
map_feed_title_and_feed-1689374-11.patch3.34 KB
MegaChriz’s picture

MegaChriz CreditAttribution: MegaChriz as a volunteer commented
Status: Needs review » Needs work
Issue tags: +Need tests

I tested the patch in action and I found no issues with it. I tested with an importer attached to a content type and with the fetchers HTTP fetcher and File fetcher.

Code review:

  1. +++ b/plugins/FeedsParser.inc
@@ -141,16 +158,40 @@ abstract class FeedsParser extends FeedsPlugin {
+    // Node elements.
+    elseif (strpos($element_key, 'parent:') === 0 &&
+        $source->feed_nid &&
+        ($node = node_load($source->feed_nid))) {

    These lines could be improved a bit. According to the code standards, if control structure condition tend to get long it's better to prepare the conditions separately.
    https://www.drupal.org/coding-standards#linelength

  2. Since this patch adds new sources, these sources need test coverage. Create a test method for each source. I think for the source "feed_source:source" there should be a test that covers the case for HTTP fetcher and another test that covers the case for the File fetcher.