Silverstripe-framework: exclude acts like excludeAny not exclude"ALL" in 3.5.2

[sunnysideup]

Here is the php:

$list = $list
                ->filter(
                    array(
                        "CancelledByID" => 0
                    )
                )
                ->exclude(
                    array(
                        'ID' => array(1,2,3),
                        'StatusID' => array(2,4,5)
                    )
                );

the resulting mysql is:

SELECT DISTINCT Bla 
FROM "Order" 
WHERE 
    ("Order"."CancelledByID" = ?) AND 
    (
         ("Order"."ID" NOT IN (?, ?, ?)
    ) *** OR ****
         ("Order"."StatusID" NOT IN (?, ?, ?)
    )
) 
ORDER BY "Order"."LastEdited" DESC

From what I understand, and if I had to bet, I'd say I am wrong ;-))))))

* OR should be AND *.

i.e. I want to select all Orders, excluding Orders with a certain IDs and certain StatusIDs.

[dhensby]

@sunnysideup I think you're right - it's always confused me how we don't have excludeAny.

What happens if you do:

$list->exclude([
    'ID' => [1, 2, 3],
])->exclude([
    'StatusID' => [1, 2, 3],
]);

[sunnysideup]

Yeah - that is how I solved it... (call exclude two times).

Is there anyway we can fix this properly in 4.0? I would suggest to have an

Maybe we can do like this:

• exclude - keep as is, but deprecate in 5.0
• excludeAny replaces current exclude - basically mirrors filterAny
• excludeAll is basically what we currently expect exclude to be (exclude ALL of them).

I can see that you can also do it the other way around (any vs all), but to me that seems the best way forward.

[dhensby]

This should be fixed, yes. I don't agree with deprecating exclude we should just make it work as expected.

[sunnysideup]

can it still go into 4.0? What will happen to all the people who know exclude as the way it works now. It will be crazy confusing for them that it works the opposite way in 4.0???

[sunnysideup]

If it can go into 4.0 then if you tell me which branch to use, etc... then I am happy to code it (should only be a few lines - famous last words ;-)))

[dhensby]

Yes, it can go into 4. The master branch.

We'll put something in the upgrade notes. But exclude is clearly working contrary to expectations.

[tractorcow]

Could we just make exclude behave as exclude all (mirror of filter) and add excludeAny with the old behaviour of exclude, as the mirror to filterAny? That way we have the same method naming and behaviour between filter/exclude methods. We don't have a filterAll do we, so it feels odd to have excludeAll.

[sunnysideup]

This has actually caught me out a second time now in a project...

I made a start with a fix:

https://github.com/silverstripe/silverstripe-framework/compare/master...sunnysideup:patch-49

but I think it would be better if a core dev would take over as I have limited time to do this.

Thank you

[dhensby]

@andrewandante has done some great investigation into this and exclude is working completely as expected. He'll be opening a PR with some more tests to completely show it's working as expected.

Basically, exclude is conceptually quite difficult to get one's head around because you're having to write an inverse query. The query is saying ID NOT IN (?, ?, ?), the NOT part is what makes the OR necessary.

The query is essentially writing the exclude as an inverted filter which means the INs become NOT IN and the ANDs become OR...

[sunnysideup]

It appears to me that unless the code is changed, there are still the same issues. Is that right?

@dhensby, as you said:

I think you're right - it's always confused me how we don't have excludeAny.

Has anything been done to make it less confusing?

What I would expect is that if I would write:

->exclude( A, B, C)

then I would get less results than

->exclude(A)

but, from what I understand, it does not work that way - i.e. ->exclude( A, B, C) actually excludes less records than ->exclude(A).

What is more, if I write:

$list->exclude(A)->exclude(B)->exclude(C)

then I get a different result altogether from ->exclude( A, B, C)

"Technically" this may be right, but it definitely does not seem intuitive and it is likely that developers may miss this subtlety.

Am I missing something? If not, is there not a way to make it easier to understand?

[andrewandante]

If you write ->exclude( A, B, C) then yes, you do get fewer. It's when you are doing the mapping that it gets complicated.

As @tractorcow said, exclude() should work the same as filter(). That is, if you pass arrays, it needs to match _all_ to match the filter. The only way to get this in line is to change both methods, and that is likely more confusing. I think the default behaviour of

$list->filter([
  'Name' => ['Adam', 'Beatrice'],
  'Occupation' => ['Painter', 'Doctor']
]);

is intuitively that you want an item to have to match both of those criteria to match the filter - and as such, we should map exclude() to match, which is the current behaviour.

I think that _adding_ excludeAny() is half the battle - if you see that there are two methods, you are more likely to be able to figure out which is the one you want. Additionally, in the PR, I've updated the docblock (as you did) to recognise that it was misleading before, including examples in the excludeAny() method.

[tractorcow]

What I would expect is that if I would write:
->exclude( A, B, C)
then I would expect to get less results than
->exclude(A)

If you exclude more credentials (exclude items with A AND B AND C) then you should get more, because you're including items that may match only some of those credentials, not fewer. Hence exclude() is acting as excludeAll()

[tractorcow]

Another way to look at it is:

(!cond1 OR !cond2) === !(cond1 AND cond2) (exclude/excludeAll())
also
(!cond1 AND !cond2) === !(cond1 OR cond2) (excludeAny)

Notice how the conjunction flips without changing the meaning of the condition.

[dhensby]

@sunnysideup:

@dhensby, as you said:

I think you're right - it's always confused me how we don't have excludeAny.

Yes, I said I think because conceptually it's difficult at first glance to reconcile the logical SQL query needed.

However, it's clearly correct behaviour and it was just a bit difficult to fully grasp.

And I note what you said as well:

From what I understand, and if I had to bet, I'd say I am wrong ;-))))))

It seems we both were :)