Let components store feedback

[ddeboer]

Thanks to @AydinHassan (#93) the workflow now returns a result object with exceptions caught during process(). I think it would be useful to make it possible for components to add non-exception feedback about their actions too. For instance, a converter could send out a notification that some empty fields were filled with default values.

How should we go about this?

1. Construct the Result object at the beginning of process() and pass that to each component. The interfaces then change to convert($item, Result $result), filter($item, Result $result) etc. We then add a addFeedback() and getFeedbackCollection() (or something similar) to Result. The Result class could automatically add the current row number to the feedback item, something which the filters and converters currently have no access to.
2. Downside to the above solution is that the $result object may well be unused in many components. So perhaps adding a separate ReporterInterface is better. If components implement that they must add an setResult(Result $result) method. In their convert($item)/filter($item) etc. methods they could then access $this->result to add feedback. In this case, the workflow must take care of calling setResult() at the beginning of process().

@Baachi @AydinHassan Which do you prefer? Do you see any alternatives?

[AydinHassan]

I definitely prefer the second solution and I like the idea of consumers being able to swap about the Result object for their own - but as noted before the ResultInterface will be pretty large. Obviously we would provide an default implementation. Or are you thinking to not allow swapping of the Result?

Another thing to consider is how do you differentiate which converter/filter added a message - I can likely see a situation where you might want to know which converter/filter added the message.

[ddeboer]

Yes, in the end the Result should be interchangeable. Perhaps we can have two interfaces: a simple ResultInterface and a AdvancedResultInterface which has start/end time, count etc. Alternatively, we could have two objects: the current Result and a new Feedback class.

[AydinHassan]

Will come back to this - on holiday for a few days now :airplane: :beers:

[ddeboer]

All right, have fun!

[Baachi]

@ddeboer I really like the idea with the reporter. I'm currently work on this library, to made it more customizable. My idea is, to add a middleware layer inside the Workflow class. This made it really easy for a developer, to customize the behaviour.

But i would rather let filter/converters throw exception and let a Reporter collect them. If the import/export is finished, the developer should acced to this object to retrieve this information.

[ddeboer]

@Baachi Yep, we’re already throwing exceptions and collecting them through the Result object. However, I’m here talking more about feedback that isn’t really an exception, but just some info that the user may want to know, for instance: empty value converted to default value X, value Y appended with -2 to make it unique etc. If the converter in question threw an exception, the record wouldn’t be imported. But I want have those records imported, just with some slight changes.

Looking forward to seeing how you’re customizing the lib.

[Baachi]

Ah okay, but this sounds like a logger. Why not inject an logger to each converter, this wouldn't introduce an BC break.

[ddeboer]

Somewhat like a logger, yes, but with two key differences:

• it should be reset after each call to process(), otherwise the import results are polluted with results from previous imports; this is not an issue when running a small import from a web request but it is in my case, with a long-running RabbitMQ consumer that processes multiple imports
• it should store well-structured data, so not just log message strings, but a combination of row number, column, message (e.g.).

[Baachi]

Yes sounds cool. I would prefer your second solution. Create a ReporterInterface and implement it in the needed converter/filters. I think its the best solution, because not each filter/converter are able to log something.

[seydu]

It is necessary to have a detailed report of problems encountered, specially if we want to help the user find and correct them. Knowing the cell (row and column numbers) would help a lot

[gnat42]

Hmm I didn't see this thread when I submitted issue #239

[seydu]

I was about to discuss the possibility of submitting a PR on this. I was thinking of wrapping the rows in an array like object. This would give much more flexibility. But I am worried about two things: performance BC breaks.

[seydu]

I added a way to let all participants be able to generate detailed reports: Reporter Feature At first It seemed to me that it would be straightforward. For it to work I had to introduce changes in the result class, step aggregator and step classes. Besides that I had to either subclass the converters or wrap them in adapters. There was no way to reuse them directly (they throw exceptions when the encounter unexpected values) I could have added more default converters that are aware of the reporter feature.

But I did not feel this is right because it introduces too many changes. I ended up putting these converters in my project.

I think that to be able to use the reporter interactively it must allow a detailed reporting feature ! So users can pinpoint their errors.

The workflow could act like a mediator: it will receive all objects it needs or we will let it create them in separate methods so that subclasses can provide their own instances by overriding these methods (or provide factories).

I would suggest to wrap the items (row in a CSV reader) in an object that implements a minimal interface. This object would work like a bridge. Workflow subclasses could provide an instance that carries things like a message collector. Then we will not use the return values to decide if an item should be skipped, This information will be registered in the Item object by participants (like converters and validators). This will allow later on to delegate the skip or use decision making if it becomes complex

The only drawback I see (besides the BC breaks that are expected if we decide to go this way) is a performance hit. But if we keep these extra objects light this will not be noticeable.