Apply filter even if the operator is not provided

[VincentLanglet]

Subject

Bug reported #6590

I am targeting this branch, because bug fix.

Changelog

### Fixed
- Apply filter even if the operator is not provided.

[core23]

May be extract some methods here?

[VincentLanglet]

IMHO there is too much private method here.
Testing applyFilter wasn't easy.

Creating one method for a one line check seems overkill.
I already use one time variable for improving the readability.

[core23]

IMHO there is too much private method here.

That's an other problem of the class. The class / method does too much stuff.

Creating one method for a one line check seems overkill.

Not, if you look at the NPath complexity here...

[VincentLanglet]

Not, if you look at the NPath complexity here...

I don't see why it would improve the complexity.
What method do you want to extract ?

[greg0ire]

I think @core23 probably means to have a hasType and hasValue method, or just one method with an extra 'value'|'type' parameter

[VincentLanglet]

Yes but it's basically a method

private function hasValue($key1, $key2)
{
    return isset($this->values[$key1][$key2]) && '' !== $this->values[$key1][$key2];
}

used this way

if ($this->hasValue($filterFormName, 'value') || $this->hasValue($filterFormName, 'type')) {
     $filter->apply($this->query, $data[$filterFormName]);
}

I don't understand how it improved the complexity. I just feel like it's loosing context.

I changed the code for

$value = $this->values[$filterFormName]['value'] ?? '';
$type = $this->values[$filterFormName]['type'] ?? '';

if ('' !== $value || '' !== $type) {
    $filter->apply($this->query, $data[$filterFormName]);
}

Isn't it better ?

[greg0ire]

That looks good enough to me 👍

[franmomu]

My 2 cents, maybe giving some semantic to the method, something like:

foreach ($this->getFilters() as $name => $filter) {
    $this->values[$name] = $this->values[$name] ?? null;

    if ($this->canFilterBeApplied($filter, $this->values)) {
        $filter->apply($this->query, $data[$filterFormName]);
    }
}

Maybe there should be a method in FilterInterface to check if a filter can be applied or not.

[VincentLanglet]

I'm more concern about the line

$this->values[$name] = $this->values[$name] ?? null;

Which I don't understand.

[VincentLanglet]

But the purpose of this PR is to fix a bug, so I would avoid to introduce a new method in the interface or to remove a line I don't understand here, in order to quickly release a 3.80.1 version.