In response to the changes in the tamper module introduced in Multiple value handling.

The tamper change has not currently been merged, but the intention is to do this soon.

CommentFileSizeAuthor
#6 interdiff-2948569-3-6.txt2.78 KBmegachriz
#6 feeds_tamper-multi-value-handling-2948569-6.patch4 KBmegachriz
#3 interdiff-2948569-2-3.txt10.55 KBmegachriz
#3 feeds_tamper-multi-value-handling-2948569-3.patch11.68 KBmegachriz
#2 feeds-tamper-multi-value-handling-2948569-2.patch1.89 KBericgsmith

Comments

ericgsmith created an issue. See original summary.

ericgsmith’s picture

ericgsmith commented
Status: Active » Needs review
StatusFileSize
new feeds-tamper-multi-value-handling-2948569-2.patch1.89 KB

This logic is basically a copy paste from your test in ChainedTamperTest.

The only change is that I've added an explicit check for an array instead of assuming at first it will be scalar. My assumption for this is that if the copy plugin can work as it currently can, we could have an array as a source value.

megachriz’s picture

megachriz
Primary language Dutch
commented
StatusFileSize
new feeds_tamper-multi-value-handling-2948569-3.patch11.68 KB
new interdiff-2948569-2-3.txt10.55 KB

Here is an automated test. I took ideas from MigrateExecutableTest. Assumes #2948543: Make all plugins configurable and #2937719: Multiple value handling to be committed. I added throwing an exception to the process. That one still needs work: it needs to be a more specific exception, it should not stop manipulating the rest of the items and Feeds should somehow report an error for that item. If that becomes too much to handle in this issue, throwing an exception could be handled in a follow-up.

Maybe a part of the test should be handled in an other issue first to make this one less complex and less dependent. Maybe I will do so tomorrow.

Status: Needs review » Needs work

The last submitted patch, 3: feeds_tamper-multi-value-handling-2948569-3.patch, failed testing. View results

ericgsmith’s picture

ericgsmith commented

Just a thought there is also pending tickets to throw the skip exception classes in tammper, would it be easier to handle all exceptions in a new issue?#

megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Needs work » Needs review
StatusFileSize
new feeds_tamper-multi-value-handling-2948569-6.patch4 KB
new interdiff-2948569-3-6.txt2.78 KB

Thanks for committing #2937719: Multiple value handling!

Yes, I think exception handling would better to do in a follow-up.

I've created a spin-off issue for basic test coverage for the FeedsSubscriber class: #2950522: Add basic test coverage for FeedsSubscriber. Since it passed tests and I verified that the class works on runtime, I committed my patch there.

So here is a new patch, excluding the exception bit.

  • MegaChriz committed d60d802 on 8.x-2.x authored by ericgsmith 
    Issue #2948569 by MegaChriz, ericgsmith: Handle multivalue changes from...
megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Needs review » Fixed

Committed #6.

megachriz’s picture

megachriz
Primary language Dutch
commented

I've opened a follow-up: #2951116: Throw an exception if a Tamper plugin doesn't return an array when it should.

Status: Fixed » Closed (fixed)

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