thephpleague / pipeline

League\Pipeline
https://pipeline.thephpleague.com
MIT License
959 stars 76 forks source link

Remove InterruptibleProcessor and Rename FingersCrossedProcessor #25

Closed GreatOwl closed 8 years ago

GreatOwl commented 8 years ago

I would like to mention a criticism of recent decisions on this awesome package, and request input from others.

The recent addition of the additional processors I feel was kind of messy compared to the resistance to bloat this project has taken in the recent past. I think the addition of the Processor interface was a fantastic idea; however, I feel the implementation was overreaching. The InterruptibleProcessor should have been an implementation detail of someone's project utilizing pipelines. Taking this approach would allow the library to resist the bloat of trying to predict/satisfy every users desired implementation. I hate BC breaks, but this library is less than 1.0, is this something the maintainers would consider allowing modification of?

To go along with this, since the FingersCrossedProcessor is the default Processor and the one we have all loved so much that we have been using this package Pre-1.0, doesn't it seem more fitting that its named with the respect it deserves? Such as Processor or DefaultProcessor.

Of course I mean no disrespect to the author of the changes, because the code was thoughtful and well written.

I will happily be providing more of my input in the PRs on this project in the future because I am passionate about the potential of this library, but I am certainly interested in other people's thoughts on this issue.

frankdejonge commented 8 years ago

@GreatOwl sorry for the (very) late response. First of all, thank you for the comments. I've thought quite some time how such a component should be named. I went with "fingers crossed" mostly because Monolog has a similar handler type, which I thought was kind of funny. I also like the fact that you almost feel like you should think about what happens when things don't go according to plan, like a try/catch block around the execution. However, I am totally open to suggestions. Names like DefaultProcessor don't really cover the load because it doesn't describe what that kind of processor does. With "fingers crossed" you can assume there's no error handling or checks involved, which is something I like about that name.

GreatOwl commented 8 years ago

I think that is a fair assessment of your decision.

From my position I see the opportunity for being able to apply a functional pattern with this library, modifying state as each stage completes. The original implementation was wonderful in my opinion because of its use of top level functions instead of a foreach during execution.

The usage of the foreach is logical however for those dealing with a traditional stack, and gathers no opposition.

so I will willingly close this, and request your continued attention to: https://github.com/thephpleague/pipeline/issues/24

Thanks @frankdejonge

frankdejonge commented 8 years ago

@GreatOwl converting the array_reduce into a foreach was unfortunate, but PHP's handling of exceptions was nipping it in the butt. When an exception is thrown it's unrecoverable unless handled inside the stage, which is not always desired. This made me have to "downgrade" the actual implementation, I share your grief.