Add support for link fields

Patch not working anymore, code in "feeds_et.module" has been changed in the mean while...

Attached is the same patch against current VCS 7.x-1.x HEAD. I also fixed a few coding style issues.

The patch works fine for me, it'd be great if we could get it committed.

I need to import link fields using feeds too; I'll see what I can do to help this patch along.

***

The patch still applies as of commit bf0d6d0.

***

Overall, the patch in #3 appears to use the API well. I do have a few small questions / coding-standards nit-picks:

1. diff --git a/feeds_et.mappers.inc b/feeds_et.mappers.inc
   index 8299b02..9ef7734 100644
   --- a/feeds_et.mappers.inc
   +++ b/feeds_et.mappers.inc
   @@ -45,6 +45,28 @@
    /**
   + * Callback for mapping link fields.
   + */
   +function _feeds_et_link_feeds_set_target($source, $entity, $target, $value) {
   

   I know the other callbacks don't do this, but for clarity, I would add...

   @see feeds_et_feeds_processor_targets_alter()
   @see \FeedsProcessor::mapToTarget()
   

   ...to the docblock, to indicate the functions responsible for adding this function to the list of callbacks, and the function that actually calls the callback respectively.

2. diff --git a/feeds_et.mappers.inc b/feeds_et.mappers.inc
   index 8299b02..9ef7734 100644
   --- a/feeds_et.mappers.inc
   +++ b/feeds_et.mappers.inc
   @@ -45,6 +45,28 @@
   +  foreach ($value as $k => $v) {
   +    if (empty($v)) {
   +      unset($k);
   +    }
   +    // @todo should we do more validation here?
   +  }
   

   I'd argue that this chunk of code is unnecessary, because the feeds_tamper already provides a "Filter empty items" (machine name: array_filter) tamper which does this, and can be turned on or off at the site builder's discretion depending on their needs.

   If this is absolutely necessary (i.e.: to prevent fatal errors), then there are two problems:

   1. This unsets the key variable inside the loop, but AFAICT does not unset the key-value pair from the array, which I think is the intention. I'd replace this with unset($value[$k]).
   2. You should remove the @todo comment... AFAICT, there isn't any other validation we should / could be doing, and adding @todo comments in a patch implies that either this patch is incomplete or that the module maintainer needs to do this in the future.

3. diff --git a/feeds_et.mappers.inc b/feeds_et.mappers.inc
   index 8299b02..9ef7734 100644
   --- a/feeds_et.mappers.inc
   +++ b/feeds_et.mappers.inc
   @@ -45,6 +45,28 @@
   +  // the second parameter is the target key (title or url) for links.
   +  $target_key = $target_pieces[1];
   

   The fact that "the second parameter is the target key" is re-stated in the code on the next line. It's more important that the target key can be either title or url for links.

   Coding standards say that there needs to be a blank line before this type of comment, and comments should start with a capital letter.

   I would add the blank line before the comment and re-word the comment to say "The target key will be either 'title' or 'url' for links."

4. diff --git a/feeds_et.module b/feeds_et.module
   index 0551e58..d658bb3 100644
   --- a/feeds_et.module
   +++ b/feeds_et.module
   @@ -21,16 +21,37 @@
   +      // Certain fields (with multiple targets) use field name:property.
   +      switch ($field_info['type']) {
   ...
   +        case 'date':
   +          $keys = array();
   +          $keys[] = $field_name . ':start';
   +          $keys[] = $field_name . ':end';
   +          break;
   +      }
   

   This patch is focused on adding support for link fields, not date fields. Maybe this change was part of another patch that crept into this one? If this was intentionally included in this patch (i.e.: throw in support for date fields because it's easy to do) then this issue title and summary should be updated to clarify this.

Issue housekeeping

I am working on this issue currently.

@Zalak and I discussed this in Slack, and because the changes in this patch provide a great basis for #1850930: Support multilingual File fields (which we also need done for our client and I was already working on), I'm going to continue working on this. Thus, by mutual agreement, I am reassigning this issue to myself.

***

I have attached a patch that fixes the issues I identified in #4.

Note that I removed the Date field key definitions because there is no corresponding _feeds_et_date_feeds_set_target() (i.e.: the callback for the mappings, set in _feeds_et_process_targets()). Also, the ET mappings do not show up (not sure if that is because Feeds can't find the callback, or some other reason).

Sorry for the delay on this. I seem to be able to choose in the "target configuration" column which language a mapping targets for multilingual file fields, so I'm not really sure there's any reason for me to leave this assigned to myself.