#1183440: Multilingual Feeds - Make field import language-aware has demonstrated that out mapping configuration API is less than stellar and should be significantly enhanced.

We should, in a backwards compatible way, allow multiple form and summary callbacks to be defined. We should also add either a mechanism for default values, or a process callback so that the configuration values can be prepared before mapping.

CommentFileSizeAuthor
#42 feeds-extend-mapping-api-2333029-42.patch54.12 KBtwistor
#42 interdiff.txt3.91 KBtwistor
#39 feeds-extend-mapping-api-2333029-39.patch51.36 KBtwistor
#39 interdiff.txt6.16 KBtwistor
#38 feeds-extend-mapping-api-2333029-37.patch45.76 KBtwistor
#38 interdiff.txt562 bytestwistor
#36 feeds-extend-mapping-api-2333029-36.patch45.75 KBtwistor
#36 interdiff.txt4.9 KBtwistor
#34 feeds-extend-mapping-api-2333029-34.patch45.54 KBtwistor
#34 interdiff.txt1.51 KBtwistor
#31 feeds-extend-mapping-api-2333029-30.patch45.04 KBtwistor
#29 feeds-extend-mapping-api-2333029-29.patch44.92 KBtwistor
#28 feeds-extend-mapping-api-2333029-28.patch39.52 KBtwistor
#26 feeds-extend-mapping-api-2333029-26.patch38.95 KBtwistor
#23 feeds-extend-mapping-api-2333029-23.patch38.96 KBtwistor
#22 feeds-extend-mapping-api-2333029-22.patch38.67 KBtwistor
#18 feeds-extend-mapping-api-2333029-18.patch31.72 KBtwistor
#18 interdiff.txt664 bytestwistor
#16 feeds-extend-mapping-api-2333029-16.patch31.8 KBtwistor
#15 feeds-extend-mapping-api-2333029-15.patch0 bytestwistor
#14 feeds-extend-mapping-api-2333029-14.patch31.57 KBtwistor
#14 interdiff.txt9.35 KBtwistor
#13 feeds-extend-mapping-api-2333029-13.patch22.99 KBtwistor
#13 interdiff.txt1.43 KBtwistor
#12 feeds-extend-mapping-api-2333029-12.patch22.83 KBtwistor
#12 interdiff.txt2.82 KBtwistor
#10 feeds-extend-mapping-api-2333029-10.patch20.6 KBtwistor
#10 interdiff.txt1.5 KBtwistor
#8 feeds-extend-mapping-api-2333029-8.patch20.56 KBtwistor
#8 interdiff.txt7.03 KBtwistor
#7 feeds-extend-mapping-api-2333029-7.patch18.04 KBtwistor
#7 interdiff.txt2.12 KBtwistor
#4 feeds-extend-mapping-api-2333029-3.patch16.66 KBtwistor
#2 feeds-extend-mapping-api-2333029-2.patch5.39 KBtwistor
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

twistor’s picture

twistor’s picture

Status: Active » Needs review
FileSize
5.39 KB

Let's see the damage.

MegaChriz’s picture

Not much damage yet! I hope to look at the patch in a few days. Or is the patch just a start/incomplete?

twistor’s picture

twistor’s picture

There's plenty more to come.

I still need to:

  • Update the API docs.
  • Add the process callback.
twistor’s picture

Well that was surprising!

I should have most of the functionality wrapped up today.

Another @todo in this patch: We need a way to ensure hook_feeds_processor_targets() is invoked. There will be a gap where other processors won't be calling module_invoke_all('feeds_processor_targets'), but still calling drupal_alter('feeds_processor_targets').

I'm thinking we can set a global flag that tracks if module_invoke_all('feeds_processor_targets') has been called, and if it hasn't we can call it from feeds_feeds_processor_targets_alter(). It's not very pretty, and we should probably post patches to the relevant processor modules, but we should have a hack for the mean time.

twistor’s picture

Alrighty, this is what I was thinking about with the preprocess business.
I'm not sure if it makes sense yet.

twistor’s picture

Status: Needs review » Needs work

The last submitted patch, 8: feeds-extend-mapping-api-2333029-8.patch, failed testing.

twistor’s picture

Status: Needs work » Needs review
FileSize
1.5 KB
20.6 KB

Status: Needs review » Needs work

The last submitted patch, 10: feeds-extend-mapping-api-2333029-10.patch, failed testing.

twistor’s picture

Status: Needs work » Needs review
FileSize
2.82 KB
22.83 KB

Whew! I would have been worried if I didn't get any failures.

twistor’s picture

This is just to prove to myself that the backwards compatibility works.

twistor’s picture

I might be getting a little carried away, but I'm feeling better about things.

twistor’s picture

twistor’s picture

I think that means I should quit for today.

Status: Needs review » Needs work

The last submitted patch, 16: feeds-extend-mapping-api-2333029-16.patch, failed testing.

twistor’s picture

Status: Needs work » Needs review
FileSize
664 bytes
31.72 KB

Status: Needs review » Needs work

The last submitted patch, 18: feeds-extend-mapping-api-2333029-18.patch, failed testing.

twistor’s picture

Status: Needs work » Needs review
FileSize
38.67 KB

Re-roll.

twistor’s picture

The last submitted patch, 22: feeds-extend-mapping-api-2333029-22.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 23: feeds-extend-mapping-api-2333029-23.patch, failed testing.

twistor’s picture

Status: Needs work » Needs review
FileSize
38.95 KB

Status: Needs review » Needs work

The last submitted patch, 26: feeds-extend-mapping-api-2333029-26.patch, failed testing.

twistor’s picture

Status: Needs work » Needs review
FileSize
39.52 KB

This found a bug.

We need to bump our required features version to 2.x.

twistor’s picture

I've added some tests.

I figured out a way to make this perfectly backwards compatible with processors that don't invoke hook_feeds_targets. It's hacky, but it works.

I think this is ready to go. I've been running tests on non-upgraded feeds modules I have lying around. Only one problem in feeds_ex, but that was an existing bug.

Status: Needs review » Needs work

The last submitted patch, 29: feeds-extend-mapping-api-2333029-29.patch, failed testing.

twistor’s picture

FileSize
45.04 KB

Just need to add 'preprocess_callbacks' => array('feeds_tests_preprocess_callback'), to test_unique target.

I think it's ready!

MegaChriz’s picture

Status: Needs work » Needs review

Great! I hope to take a look at this patch next Thursday. I remember from last time, back in September last year, I had a hard time to review it.

MegaChriz’s picture

Status: Needs review » Needs work

This patch looks like it brings a lot of improvements and clean-ups. Great work!

Here is what I could find so far after looking at the code.

  1. +++ b/plugins/FeedsProcessor.inc
    @@ -873,13 +951,13 @@ abstract class FeedsProcessor extends FeedsPlugin {
    -        foreach ($targets[$this->id][$target]['unique_callbacks'] as $callback) {
    -          if (is_callable($callback) && $entity_id = call_user_func_array($callback, array($source, $this->entityType(), $this->bundle(), $target, $value))) {
    +        foreach ($targets[$target]['unique_callbacks'] as $callback) {
    +          if ($entity_id = $callback($source, $this->entityType(), $this->bundle(), $target, $value)) {
    

    I think you should leave the call_user_func_array() here, because a callback can also be defined as an array if the callback points to a class method, for example:

    $callback = array('MyClass', 'myCallbackMethod');
    

    See http://php.net/manual/en/language.types.callable.php

  2. +++ b/feeds_ui/feeds_ui.admin.inc
    @@ -634,12 +634,10 @@ function feeds_ui_mapping_settings_form($form, $form_state, $i, $mapping, $targe
    -    // Build the form.
    -    if (isset($target['form_callback'])) {
    -      $settings_form = call_user_func($target['form_callback'], $mapping, $target, $form, $form_state);
    -    }
    -    else {
    -      $settings_form = array();
    +    $settings_form = array();
    +
    +    foreach ($target['form_callbacks'] as $callback) {
    +      $settings_form += $callback($mapping, $target, $form, $form_state);
    
    @@ -664,13 +662,15 @@ function feeds_ui_mapping_settings_form($form, $form_state, $i, $mapping, $targe
    -    if (isset($target['summary_callback'])) {
    -      $summary = call_user_func($target['summary_callback'], $mapping, $target, $form, $form_state);
    -    }
    -    else {
    -      $summary = '';
    +    $summary = array();
    +
    +    foreach ($target['summary_callbacks'] as $callback) {
    +      $summary[] = $callback($mapping, $target, $form, $form_state);
    

    Same story here. A callback can be a class method.

  3. +++ b/feeds.feeds.inc
    @@ -0,0 +1,86 @@
    +  // Filter out any incorect callbaks. Do it here so it only has to be done
    

    Typos:

    • "incorect" => "incorrect"
    • "callbaks" => "callbacks"
  4. Documentation for the new hook hook_feeds_processor_targets() is missing in feeds.api.php.
  5. I see targets can now have "preprocess_callbacks", but in feeds.api.php it is not documented that a target can have such a callback, what it does and what the signature for the callback is. Add an example function called my_module_preprocess_callback() and an example target to feeds.api.php.
  6. For the tests, I think it would be a good idea to add a test target with two summary callbacks and two form callbacks. This way the behaviour of having multiple callbacks can be covered. Now all test targets in feeds_tests.module that have summary/form callbacks defined have only one summary/form callback.
twistor’s picture

Status: Needs work » Needs review
FileSize
1.51 KB
45.54 KB

A quick test about the callback thing.

On my machince, $callback = [$obj, 'method']; $callback($arg1, $arg2); works fine, but I seem to remember there being a PHP version issue. I can't seem to track down anything, so throwing up a quick test to verify.

Status: Needs review » Needs work

The last submitted patch, 34: feeds-extend-mapping-api-2333029-34.patch, failed testing.

twistor’s picture

Status: Needs work » Needs review
FileSize
4.9 KB
45.75 KB

Documentation fixes up next.

Status: Needs review » Needs work

The last submitted patch, 36: feeds-extend-mapping-api-2333029-36.patch, failed testing.

twistor’s picture

Status: Needs work » Needs review
FileSize
562 bytes
45.76 KB

Bahhh

twistor’s picture

twistor’s picture

If this passes, I'm going to commit it. Any problems found can be followups.

twistor’s picture

twistor’s picture

opps. Forgot the extra test coverage for the second form/summary callbacks.

  • twistor committed 11c68ef on 7.x-2.x
    Issue #2333029 by twistor, MegaChriz: Extend mapping API to allow for...
twistor’s picture

Assigned: twistor » Unassigned
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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