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.

Sign up for GitHub

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

Jump to bottom

Remove return type dependency of filter step #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove return type dependency of filter step #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion spec/Step/FilterStepSpec.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ function it_processes_and_filters_an_item(Step $step)
return false;
})->shouldReturn($this);

$this->process(
$this->shouldThrow('Port\Steps\Exception\FilterException')->duringProcess(
$item,
function($item) use ($step, $next) {
return $step->process($item, $next);
Expand Down
15 changes: 15 additions & 0 deletions src/Exception/FilterException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<?php

namespace Port\Steps\Exception;

use Port\Exception;

/**
* Thrown when the processing of the pipeline should be stopped
*
* @author Márk Sági-Kazár <[email protected]>
*/
class FilterException extends \Exception implements Exception
{

}
3 changes: 2 additions & 1 deletion src/Step/FilterStep.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Port\Steps\Step;

use Port\Steps\Exception\FilterException;
use Port\Steps\Step;

/**
Expand Down Expand Up @@ -39,7 +40,7 @@ public function process($item, callable $next)
{
foreach (clone $this->filters as $filter) {
if (false === call_user_func($filter, $item)) {
return false;
throw new FilterException();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a good idea, this will decrease the performance a lot.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be happy with any alternative which does not mess with return values.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is, i agree if we goes with good design this should throw an exception but if we decide to focus on performance this shouldn't be modified.

Is there any alternative to throw an exception? I really like good software design, but performance is also importance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure, I can't think of any clean solution, only ugly ones (with variables passed by reference at some point)

}
}

Expand Down
7 changes: 4 additions & 3 deletions src/StepAggregator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use Port\Workflow;
use Port\Writer;
use Port\Steps\Exception\BreakException;
use Port\Steps\Exception\FilterException;
use Psr\Log\LoggerInterface;
use Psr\Log\LoggerAwareInterface;
use Psr\Log\LoggerAwareTrait;
Expand Down Expand Up @@ -114,9 +115,9 @@ public function process()
// Read all items
foreach ($this->reader as $index => $item) {
try {
if (false === $pipeline($item)) {
continue;
}
$pipeline($item);
} catch(FilterException $e) {
continue;
} catch(BreakException $e) {
break;
} catch(Exception $e) {
Expand Down