From the README:

This module provides further integration between the Entity reference and
Feeds modules, allowing the user to map values to fields and properties
of referenced entities. The mapper will automatically create the target
entity if it does not exist.

Mapping depth is limited to one.

Screeshot of the mapping admin inteface: http://drupal.org/files/project-images/entityreferencefeeds.png
(Sorry about the Swedish admin interface, will try to upload a better image)

The target description is on the format: "field_name (bundle_name: bundle_field_name)"

One could argue that this module could instead be merged with the Entity Reference module, but I think it's better of as a separate module since the use cases are probably rather limited, and the extra functionality might confuse inexperienced users.

Sandbox page: http://drupal.org/sandbox/gnucifer/1862074
Git url: http://git.drupal.org/sandbox/gnucifer/1862074.git
Automatic review: http://ventral.org/pareview/httpgitdrupalorgsandboxgnucifer1862074git

Reviews of other projects

http://drupal.org/node/1858872#comment-6827166
http://drupal.org/node/1751752#comment-6827150
http://drupal.org/node/1703260#comment-6747588

Comments

gnucifer’s picture

gnucifer commented
Issue summary: View changes

Clarification

gnucifer’s picture

gnucifer commented
Issue summary: View changes

grammar

dasRicardo’s picture

dasRicardo commented

I can not find any install and require block in the README.txt

gnucifer’s picture

gnucifer commented

Is this really mandated? It seems redundant. The install step is simply to enable the module. For requirements there is the .info file.

dasRicardo’s picture

dasRicardo commented

Hmmm, you write this on your project page:

Installation
For requirements and installation instructions, refer to the included README.txt file.

It's only a tipp and not more,

gnucifer’s picture

gnucifer commented

Ahh, good catch. You are right, I should remove that statement.

gnucifer’s picture

gnucifer commented
Issue summary: View changes

grammar

gnucifer’s picture

gnucifer commented
Issue tags: +PAreview: review bonus

Reviews:
http://drupal.org/node/1858872#comment-6827166
http://drupal.org/node/1751752#comment-6827150
http://drupal.org/node/1703260#comment-6747588

dydave’s picture

dydave commented
Status: Needs review » Needs work

Automated Review: http://ventral.org/pareview/httpgitdrupalorgsandboxgnucifer1862074git

Review of the 7.x-1.x branch:

Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project.

FILE: .../all/modules/pareview_temp/test_candidate/entityreference_feeds.install
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
7 | ERROR | Missing function doc comment
--------------------------------------------------------------------------------

FILE: ...s/all/modules/pareview_temp/test_candidate/entityreference_feeds.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 4 WARNING(S) AFFECTING 4 LINE(S)
--------------------------------------------------------------------------------
68 | WARNING | Line exceeds 80 characters; contains 89 characters
69 | WARNING | Line exceeds 80 characters; contains 87 characters
196 | WARNING | Line exceeds 80 characters; contains 89 characters
264 | WARNING | Line exceeds 80 characters; contains 90 characters
--------------------------------------------------------------------------------

Source: http://ventral.org/pareview - PAReview.sh online service


Manual Review:
t() function calls seem fine. Coding also seemed fine for the code I have seen, but I haven't taken an in-depth testing of each functions. So this is probably something on which you may want to get more feedback.


So far, that's all I could find, but I will keep looking and certainly let you know if I can find anything else.
I hope these comments will help you to improve this nice module.

Cheers!

gnucifer’s picture

gnucifer commented
Status: Needs work » Needs review

Thanks for the review! Fixed the "Missing function doc comment". I knew about the long line numbers before, not sure how to solve it though. If I split the strings and concatenate I will (correctly) get the error: "t"-function may not be concatenated. So not sure if this is solvable? How can break the string up for formatting reasons without actually breaking any lines in the string?

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus

manual review:

  1. coder sniffer complains about some comment lines that are over 80 characters.
  2. "drupal_alter('feeds_processor_targets', $entity_targets, $target_type, $info['bundle']);": Hooks that are provided by a module should be documented in MODULENAME.api.php, see http://drupal.org/node/161085#api_php

But otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

gnucifer’s picture

gnucifer commented

Hi and thanks for the review. About the issues:

1. I read somewhere that an acceptable exception for exceeding 80 characters is when using the t-function, (as i described above concatenation is not allowed for arguments to t, so that will result in another error.

2. I don't expose any new hooks in this module, the drupal_alter-invokation is a feeds-hook, (hook_feeds_processor_targets_alter), that I need to invoke in order for this module to do it's job.

kclarkson’s picture

kclarkson commented
Status: Reviewed & tested by the community » Needs review

Hate to be a downer but it looks like the feeds integration is going into entity references itself.

I just did a large data import that included various references with the following patch to entity references View patch #1616680: Handle more mapping keys in Feeds integration

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Postponed (maintainer needs more info)

@gnucifer: could you check out the patch?

Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition.

Please set this back to RTBC if it is determined that your feature cannot live in the entity reference module.

gnucifer’s picture

gnucifer commented
Status: Postponed (maintainer needs more info) » Needs review

This module exposes the fields of the entity beeing referenced for mapping, it also creates them in place. So it differs quite a lot from the patch in http://drupal.org/node/1616680. I think it's best that both mappers are available for the user to choose from, since they serve different needs.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Fixed

Since this was RTBC already and there were no other objections ...

Thanks for your contribution, gnucifer!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

gnucifer’s picture

gnucifer commented

Ok, thanks to all!

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

review bonus