Orm: We need to add isset to this key

[AndreyMashukov]

Feature Request

| Q | A
|------------ | ------
| New Feature | yes
| RFC | no
| BC Break | /no

Summary

We need to add isset to this key

and in enable too. A can make it

[Ocramius]

@AndreyMashukov a failing test is required FIRST.

[delirehberi]

I review this file and run a test I think getFilter method already validate at line Doctrine/ORM/Query/FilterCollection.php:135. I still add a test for re-validate it. I send pull request, you can close my pull-request if not necessary test for this problem.

[AndreyMashukov]

I will make it on weekend.

[AndreyMashukov]

types declarations was added for missing types

[AndreyMashukov]

And we never can to get line with unset with not enabled filter, because getFilter function checks filter, but we need to have test for it! :)

[AndreyMashukov]

oh, it's need for latest version (type hinting)

[Ocramius]

See @delirehberi's patch: he already sent a test case.

I'd rather say that the exception is correct and to be expected there.

[AndreyMashukov]

@Ocramius It's good, we need more tests

[Ocramius]

@AndreyMashukov I integrated the test in master, but the test is passing without change, so it does not reproduce the issue in this ticket.

[AndreyMashukov]

@Ocramius I can't understand why we need this Exception? I think it will be simple if we ignore disabling if filter was not enable. I think it's better way. Then we will not need to check filter before disabling

[AndreyMashukov]

@Ocramius, something like this:

    /**
     * Disables a filter.
     *
     * @param string $name Name of the filter.
     */
    public function disable($name)
    {
        if (!$this->isEnabled($name)) {
            return;
        }

        unset($this->enabledFilters[$name]);

        // Now the filter collection is dirty
        $this->filtersState = self::FILTERS_STATE_DIRTY;
    }

It will be simple to use outside

[Ocramius]

First, it is a BC break to remove the exception, as other code may rely on it, second it is correct that you shouldn't blindly enable/disable stuff that is not there, as these filters are not just a collection, but alter the behaviour of everything in the ORM.

[AndreyMashukov]

@Ocramius it's only about disabling

[AndreyMashukov]

Yes, we need to check before enable, but why we need to check before disabling?

[AndreyMashukov]

If not enabled - do nothing. but always before disable I need to check it, why?!