Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Passing an empty string to an existing value doesn't overwrite that value #797

Closed
jan-dh opened this issue Feb 3, 2021 · 22 comments
Closed
Labels

Comments

@jan-dh
Copy link

jan-dh commented Feb 3, 2021

Description

When the data in your API gets updated so a field has a new value of an empty string but it previously had a proper value, Feed-me simple ignores it and there is no way to update the entry to reflect the new data. I know values that are set to null are being ignored by Feed-me, but an empty string is a valid value and shouldn't be ignored.

Think this is where this behaviour is implemented:

// Parse the just the element attributes first. We use these in our field contexts, and need a fully-prepped element
foreach ($feed['fieldMapping'] as $fieldHandle => $fieldInfo) {
if (Hash::get($fieldInfo, 'attribute')) {
$attributeValue = $this->_service->parseAttribute($feedData, $fieldHandle, $fieldInfo);
if ($attributeValue !== null) {
$attributeData[$fieldHandle] = $attributeValue;
}
}
}

The code suggests it only checks on null but empty strings are parsed somewhere I believe to be set to a value of null if they are empty.

Maybe there are arguments to be made that sometimes you would like this behaviour, but it would be very handy to be able to set this as a settings option and not a default you have to hack your way around. At the moment the only way around this we found is by doing something like this:

   Event::on(
            Entry::class,
            Element::EVENT_BEFORE_SAVE,

Then looping over all the fields and do the checking ourselves, based on a keyword EMPTY, setting that value manually.

Steps to reproduce

  1. Set up a feed and map a data value to a field
  2. Update the feed with that same data value now set as an empty string

Additional info

  • Craft version: 3.5.17.1
  • PHP version: 7.4
  • Plugins & versions: Feed-me 4.3.4
@jan-dh jan-dh added the bug label Feb 3, 2021
@darinlarimore
Copy link
Contributor

We are experiencing this same issue. An empty string in the import data results in no changes to the field's existing data.

@grokcodile
Copy link

Same issue here. Subsequent importing of a JSON feed where a value for one field is changed from a defined value to null or "" (blank) results in the previous value remaining and not being updated with the new null or "" (blank) value.

@nickdunn
Copy link

Same here.

@dgsiegel
Copy link

An easier solution would be to use the afterParseField event which gets called after a field's data has been parsed. There you can just overwrite the parsed value. You can check the original values in $e->feedData. You might even add some more logic to differentiate between really empty and just an empty string. But the safer version would be to use a keyword like @jan-dh suggests, e.g.:

use craft\feedme\events\FieldEvent;
use craft\feedme\services\Fields;
use yii\base\Event;

Event::on(Fields::class, Fields::EVENT_AFTER_PARSE_FIELD, function(FieldEvent $e) {
  if ($e->parsedValue === "EMPTY") {
    $e->parsedValue = "";
  }
});

floatingbits added a commit to floatingbits/feed-me that referenced this issue Dec 7, 2021
@floatingbits
Copy link

I got the same issue.
I don't see the reason why empty strings would be treated as "empty" (as in "ignore me") in the first place.
dgsiegel's solution wouldn't solve the problem in my case, since - if I don't get it wrong - every empty value would be overwritten by an empty string. And I don't wanna go through the data in the event since that might get complicated.

We are using a forked version of the plugin anyway and our solution is modifying the DataHelper-class. I'm providing a pull request soon.

@darinlarimore
Copy link
Contributor

I elaborated on this, and added a settings field so this is opt-in functionality!

Screen Shot 2022-01-19 at 4 37 45 PM

@grokcodile
Copy link

When will this feature be available as an upgrade from the Craft CMS plugin store?

@mdoorschodt
Copy link

I've been waiting for months for this feature and my clients are getting more and more impatient... this is an issue that really needs fiksing. Is there a workaround in the meantime?

@nickdunn
Copy link

nickdunn commented May 9, 2022

I had to take a pretty crappy approach of changing the feed I was importing to output "None" instead of nulls or blank strings, and check for this value in my template to treat as a blank.

@mdoorschodt
Copy link

can someone please fix this issue? we have a lot of problems due to this not being properly handled

@jan-dh
Copy link
Author

jan-dh commented Jun 13, 2022

According to what I heard on one of the talks of dotAll (I believe 🤔) I think Brandon said they were looking into not investing a lot more time into Feed-me in the future, but looking into a new approach etc. for content migration in Craft 4. Haven't seen an update on this since last year though. Basically. I wouldn't count on this to ever get fixed.

@floatingbits
Copy link

@mdoorschodt
Did you see my pull request? Maybe you can integrate that on your site, e.g. by forking the repo and using my code?

@mdoorschodt
Copy link

@mdoorschodt Did you see my pull request? Maybe you can integrate that on your site, e.g. by forking the repo and using my code?

yes i did. not a big fan of forking, as updating/upgrading will be an issue later on. I just don't get it that merging a bugfix is such a problem

@pixelmachine
Copy link

Also related #723

@angrybrad @olivierbon could we get this looked at? Would close at least 3 issues and 2 pull requests.

@pixelmachine
Copy link

And #854 #680

So would close 5+ issues.

@mdoorschodt
Copy link

mdoorschodt commented Oct 11, 2022 via email

@siffring
Copy link

siffring commented Dec 7, 2022

+1 for this feature

@pixelmachine
Copy link

@brianjhanson not sure if this is on your radar but taking a look at this issue would be great!

@angrybrad
Copy link
Member

Resolved in #1228

@brandonkelly
Copy link
Member

Feed Me 4.6.0 (Craft 3) and 5.1.0 (Craft 4) have been released with the new “Set Empty Values” setting, which resolves this (via #1228) 🎉

@darinlarimore
Copy link
Contributor

Happy St Pattie's day everyone!

@dgsiegel
Copy link

@brandonkelly unfortunately this change only partially fixes this issue for us. It seems that the PR makes no difference between an empty value and a non-existent value. Ie, let's say you want to import this XML via Feedme:

<?xml version="1.0" encoding="UTF-8"?>
<root>
   <object>
      <ID>12345</ID>
      <TITLE>Some title</TITLE>
      <FOO>foobar</FOO>
    </object>
    […]
</root>

Now let's say FOO is an optional field and can be empty. If the entry exists like in the example above, both removing FOO from the XML as well as an empty FOO will clear out the original value, whereas removing it only in the latter example would be correct:

<?xml version="1.0" encoding="UTF-8"?>
<root>
   <object>
      <ID>12345</ID>
      <TITLE>Some title</TITLE>
    </object>
    […]
</root>
<?xml version="1.0" encoding="UTF-8"?>
<root>
   <object>
      <ID>12345</ID>
      <TITLE>Some title</TITLE>
      <FOO></FOO>
    </object>
    […]
</root>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests