Improve handling of empty data

Just pushing a WIP - not sure I got all the tests.

I think after working with feeds again after a long time - we should be doing this.

I think skipping on empty should solve a lot of related issues.

I suspect we might also want to add follow up issues to relax some of the strictness / casting in other plugins - but I think for now we should make sure we are being consistent and easier to use with empty data.

Setting to needs work to cleanup test failures and coding standards

I was way too eager changing everything - a bit to revert / cleanup in the branch

Alright, reverted my over eager changes.

We would need additional tests to confirm this behaviour + document its intentions somewhere.

Are we in agreement @MegaChriz on this approach?

It might be a better idea for me to fix tests & commit the related issues first given I am sure on those issues, then come back to this parent issue to review implementing in the plugins I've changed here for consistency reasons. I'll try make the Friday feeds meetup to discuss.

I've update the MR with a bunch of tests, and tweaks to avoid calls to empty in situations where null is sufficient and safer.

If I get time, it could do with some tests around some of these plugins.

I'm also going to open a seperate issue as I was not sure if we should be removing the exception thrown in the Hash plugin.

Added some more tests for empty strings and cleaned up the comment on the null value tests.

I was unable to finish executing feeds-import because some data came in as NULL, triggering the exception and interrupting the batch import. I took the MR diff and applied it as a composer patch, it worked perfectly for me!

Thank you!

This worked well. Thanks for the patch.

Worth noting that a related issue was down to bogus whitespace at the end of the file, just after the final closing tag. The XML parser didn't seem to like that either.

Unfortunately this no longer applies to the latest dev, so setting to "Needs work" to fix the merge conflicts.

Ok, I've updated the MR and I think this is ready to go again.

This is well on its way! Good work!

1. I've gone through all Tamper plugins that we have and I think that Aggregate and WordCount (relatively new plugins) should also do something about handling empty data.
2. The Math plugin throws an exception on an empty string. Perhaps we need to return an empty string here too?
3. For test coverage, I think it would be better if testing with null and an empty string is added to TamperPluginTestBase instead. This way, we'll see how each plugin reacts on that input and we can override the tests for plugins as necessary. I'm working on this one.
4. Some plugins treat the integer 0 (like ArrayFilter) as empty and return it as is. Other plugins mark it as invalid input data. More of this below.
5. The Keyword Filter plugin treats the integer 0 as empty. I think that this is wrong. You may want to filter out items whose value is zero. I wonder if Keyword Filter should abort early at all.
6. FindReplace and FindReplaceRegex accept integers and floats as valid input, but FindReplaceMultiline doesn't. Maybe fixing that is out of scope for this issue.
7. We may need additional tests for how plugins deal with 0 (as integer or float) and with boolean FALSE. But we could handle that as well in a follow-up, as handling that too makes the scope of this issue bigger.

I've made the following changes:

• Tests testWithNullValue() and testWithEmptyString() are moved to the base class and tests for certain plugins are overridden as necessary.
• Instead of assertEquals() for testing with null values or empty strings, assertSame() is used.
• Empty value handling for the Aggregate plugin is added. It returns NULL in case of an empty array, unless the used function is 'count' or 'sum'.
• Empty value handling for the WordCount plugin is added. NULL is returned in case of a null value. In case of an empty string, the number of words should be zero. The WordCount actually returned 1 in this case, so additional code is added to fix that.
• The Math plugin now treats null and empty strings as zero. Test coverage is added for empty data.
• The Keyword Filter doesn't abort in case of empty values, but it does cast the input data to a string.

Unfortunately the MR is broken against HEAD. Setting to "Needs work" so that it can be rebased (needs manual rebase; /rebase in GitLab was not sufficient).

The merge conflict should be resolved. Conflict was caused by additional test methods in EncodeTest.

Tests are passing again. 🙂

Thanks!

I've been using this for about a year on a site which imports about 20 different entities using Tamper, about 10,000 pieces of content, and everything is working great. My code isn't really affected by the changes in #20, so I will not set to RTBC, but I can say that this has made importing a lot easier for me because there are lots of null values that are now handled correctly.

@ptmkenny
Thanks for testing! What do you think of the changes that I made for Aggregate, WordCount, Math and KeywordFilter? For example, do you think it is okay that the Math plugin handles an empty value as if it were a zero? Because this ensures that the Math plugin produces a number. It does error in case the plugin is configured to divide by the source value, as in math dividing by zero is not allowed.

@megachriz Are there any cases where the user would not want the calculation to be performed if the value was NULL? For example, if the field is required, that might be a safe assumption. But if the field is not required, I can imagine that a developer might want to do a calculation that only does something if the field has a numeric value (if it is NULL, don't do the calculation).

Zero is a problem because it could mean "no value" or it could mean "the value is 0"; I have been working on a somewhat related issue in the Field Encrypt module, which uses 0 as a placeholder value for integer fields, but it was been decided to use NULL instead as the placeholder because 0 could represent an important value. ( 📌 We only encrypt fields if they don't match the placeholder value Active )

I haven't thought through all the implications of this, but my initial thought is that this could be a little dangerous.