spatie / laravel-query-builder

Easily build Eloquent queries from API requests
https://spatie.be/docs/laravel-query-builder
MIT License
4k stars 392 forks source link

Unset default of AllowedFilter after it was set once, which was possible with default(null) before nullable filters where implemented in 5.6.0 #902

Closed patrickrobrecht closed 7 months ago

patrickrobrecht commented 10 months ago

The changes from #895 aren't backwards compatible.

AllowedFilter::scope('test')->default('myDefaultValue')->default(null) does not unset the default value anymore.

Background: We have a static allowedFilters method in a model, such that we can use the same set of AllowedFilters in multiple controllers:

In TestModel:

public static function allowedFilters(): array
{
        return [
                AllowedFilter::scope('test')->default('myDefaultValue'),
                // ...
        ];
}

We can use the following in multiple controllers:

$testModels = QueryBuilder::for(TestModel::class)
        ->allowedFilters(TestModel::allowedFilters())

Removing one of the default values from the array allowedFilters() when using the filter worked with Laravel Query Builder 5.5.0, but not with 5.6.0:

$allowedFilters = [];
foreach (TestModel::allowedFilters() as $allowedFilter) {
        $allowedFilter->default(null);
        $allowedFilters[] = $allowedFilter;
}
$testModels = QueryBuilder::for(TestModel::class)
        ->allowedFilters($allowedFilters);

This PR introduces a new unsetDefault() method which could be used instead of the default(null) call in the example above. This behavior is not fully backwards compatible as one should be able to expect according to Semantic Versioning used by this project.

patrickrobrecht commented 8 months ago

I've updated https://github.com/spatie/laravel-query-builder/pull/902 to introduce a new method unsetDefault() which provides the same behavior as default(null) did before merging #895. Of course, there are tests for the new method as well.

@AlexVanderbist @Nielsvanpach Could you have a look on that?

This may be an edge case, but we may want to add some hint to the UPGRADING.md file.

AlexVanderbist commented 7 months ago

Hey, sorry about breaking backwards compatibility. This approach looks good to me, so I'll merge it in and add a disclaimer in the UPGRADING.md. Thanks!