Make range filter work with just min or max

[ychanan]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no

Currently the range filter only works if both min and max values are provided. This PR makes it work if just one of those two values is provided.

[dpfaffenbauer]

@breakone please review

[breakone]

@ychanan please check the RangeRenderer too. Till now, from and to values are mandatory.

[dkarlovi]

@breakone not understand what you mean here. If you check \CoreShop\Component\Index\Condition\RangeCondition, it actually allows both min and max to be nullable.

The RangeRenderer you've linked is indeed buggy since it doesn't account for either min or max to be null. That one should be fixed in a separate PR, it's unrelated to this change since it's already broken. :)

[breakone]

@dkarlovi true, but for simplicity let's do it in one PR.

[dkarlovi]

@breakone when you say "let's", does that mean somebody else will do it in this PR?

[dpfaffenbauer]

@dkarlovi no, @ychanan should do it in this PR.

We should do it right...

[dkarlovi]

OK, I guess I understand. I don't see this as "right" though.

This is guaranteed to make contributions more difficult though: to fix a bug, you first need to fix all the other bugs. In this case it's easy enough to fix, but in general I don't see it as a good practice to enforce, it's then much easier to upstream bugfixes at all.

[dpfaffenbauer]

@dkarlovi This PR introduces a bug somewhere else, so either we fix it completely or we leave it.

The change in this PR effectively creates this SQL Query:

from >= null AND to <= null

[dkarlovi]

@dpfaffenbauer OK, we'll fix it.

I can see this PR is mislabeled though, it's a bugfix since currently Coreshop just silently ignores the filter if both values are not set.

About this:

This PR introduces a bug somewhere else

This is incorrect: this PR fixes a bug, the one I described in the first sentence. Fixing this bug makes the other, already present, separate bug visible.

To confirm, run this with current CS beta 3:

$mocker = new \PHPUnit\Framework\MockObject\Generator();
$connection = $mocker->getMock(\Doctrine\DBAL\Connection::class, callOriginalConstructor: false);
$connection
    ->method('quoteIdentifier')
    ->with('test')
    ->willReturn('test');
$worker = $mocker->getMock(\CoreShop\Component\Index\Worker\WorkerInterface::class);

// condition with only a min, without a max
$condition = new CoreShop\Component\Index\Condition\RangeCondition('test', 1, null);
$renderer = new \CoreShop\Bundle\IndexBundle\Condition\Mysql\RangeRenderer($connection);
var_dump($renderer->render($worker, $condition));

Output

string(22) "test >= 1 AND test <= "

To sum up:

• there's two separate bugs here
• the PR is currently fixing one of them
• by fixing it, it's highlighting the other one, it's not "introducing" it, it's already there and the second bug relies on the first bug to not show up, it's basically "it's not a bug, it's a feature!"
• the second bug should be fixed regardless of the outcome of this PR, it could/should be fixed completely separately, it should have been caught by static analysis
• mandating each bugfix to first fix all the bugs which get highlighted by the new bugfix will make it really difficult and discouraging to do any bugfixing, significantly stifling new collaborations on the project

I'm writing all this not because of this PR specifically (where it's easy enough to fix both bugs), more about the opportunity for collaboration in the future which needs to be taken into account here because the project could use more help and these types of decisions about collaboration shouldn't be taken lightly IMO.

[dpfaffenbauer]

@dkarlovi will you work on that?

[dkarlovi]

Sure, I'll fix it Monday.

[dpfaffenbauer]

closed due to inactivity