prepareImportValue is wrong

[designermonkey]

I've been pulling my hair out for an entire day as to why I can't import multiple values into a Select Box Link via the XML Importer.

I remember having the discussion with @brendo on the forum (http://www.getsymphony.com/discuss/thread/45612/) as to how to do it, and yet it still just wouldn't work. Single values always worked, but never multiple values.

Take this

<items>
    <item>42</item>
    <item>handle</item>
    <item>String</item>
</items>

Using an expression of ./items/item should allow a developer to add these values to a multiple Select Box Link, yet it fails every time. It always concatenates the values into a string.

I never even thought that it was the SBL itself that was doing the dirty here, as it's not changed with this regard for sooo long, but I was wrong.

No matter what you pass the SBL's prepareImportValue function, using the getValue style, it always returns a flat string, even though the field itself is more than capable of ingesting an array.

I've hacked a change to it as follows.

    public function prepareImportValue($data, $mode, $entry_id = null)
    {
        $message = $status = null;
        $modes = (object) $this->getImportModes();

        if (!is_array($data)) {
            $data = array($data);
        }

        if ($mode === $modes->getValue) {
            if ($this->get('allow_multiple_selection') === 'no') {
                return implode('', $data);
            }

            return $data; // <-- I have changed this little line
        } elseif ($mode === $modes->getPostdata) {
            // Iterate over $data, and where the value is not an ID,
            // do a lookup for it!
            foreach ($data as $key => &$value) {
                if (!is_numeric($value) && !is_null($value)) {
                    $value = $this->fetchIDfromValue($value);
                }
            }

            return $this->processRawFieldData($data, $status, $message, true, $entry_id);
        }

        return null;
    }

Instead of (as it currently is) implode($data) being where I have changed it (as commented), I let it return an array!

Now the XML Importer finally works as expected. That's three years of me having to find hacks and convoluted methods of associating import data. One line.

@brendo can you clarify this change and let me know if it will break anything else? @nitriques could you test it too for me before I PR this change, I want to make sure that I don't break anything.

Also, this will need replicating into the Association Field if all is good.

[nitriques]

@designermonkey

In the integration branch, things looks different.

if($mode === $modes->getValue) {
    if ($this->get('allow_multiple_selection') === 'no') {
        $data = array(implode('', $data));
    }
    return implode($data);
}

I do not understand the implode -> array -> implode thing...

I am not saying that your patch is not valid tho. I just do not fully understand what's going on...

[nitriques]

Maybe we need to change two lines ? 😄

[designermonkey]

Yeah, that's what I've changed from. Sorry, I changed two lines.

        if ($mode === $modes->getValue) {
            if ($this->get('allow_multiple_selection') === 'no') {
                return implode('', $data); // <-- I changed this line
            }

            return $data; // <-- I have changed this little line too
        }

[nitriques]

Ok thanks, I just wanted to make sure I was testing the right stuff.
Will check this afternoon.

[nitriques]

But without testing, I does look better then before. 😄

[brendo]

As an extension author, would you expect getValue to return a string or an array? That's what you have to look at here. I'd expect a string and that's what the extension does:

            return array(
                'getValue' =>       ImportableField::STRING_VALUE,
                'getPostdata' =>    ImportableField::ARRAY_VALUE
            );

The XMLImporter is at fault here. The prepareImportValue function is used strangely as we're basically just using the first mode the extension offers. In the SBL case, that's going to be getValue, which isn't going to work. Ideally it would prefer the getPostdata method instead and fallback to the first mode otherwise.

As for this block, it's just personal preference. I usually like to return as early as possible so I'm not really sure why I did that way =\

if($mode === $modes->getValue) {
    if ($this->get('allow_multiple_selection') === 'no') {
        $data = array(implode('', $data));
    }
    return implode($data);
}

[designermonkey]

Ah, but the SBL is a special case with regard to values. It can be wither a single string, or an array of strings (as accepted by the field post data functions). IMO, this function should provide values as accepted by the field, which is therefore multiple values or a single value.

[designermonkey]

Otherwise, how can you ever process an array of values?

[designermonkey]

I can understand your point about the XML Importer, but what would we do to solve this from that perspective? Definitely don't want to go down the road of specifically naming array based fields, as that only causes headaches in compatibility issues for fields.

[brendo]

I can understand your point about the XML Importer, but what would we do to solve this from that perspective? Definitely don't want to go down the road of specifically naming array based fields, as that only causes headaches in compatibility issues for fields.

Fortunately, a quick projects search shows that most fields that implements the ImportableField interface already provides the getPostdata mode. This should be the preference, as it will call the field's own processRawFieldData method to massage the given value into something the field can deal with.

Fields that support a single value, or multiple values already have this support in the processRawFieldData method, so I don't really want to duplicate that logic.

[designermonkey]

Right, I'll have a look at breaking a local build then ;)