From cce863e358f54e9b78aacaa32fb84f1fcd2978a9 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Wed, 1 May 2024 11:16:49 +0100 Subject: [PATCH 01/11] Fix querying entry statuses --- src/Entries/EntryQueryBuilder.php | 64 +++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/src/Entries/EntryQueryBuilder.php b/src/Entries/EntryQueryBuilder.php index d2edf759..592a8b1c 100644 --- a/src/Entries/EntryQueryBuilder.php +++ b/src/Entries/EntryQueryBuilder.php @@ -16,6 +16,8 @@ class EntryQueryBuilder extends EloquentQueryBuilder implements QueryBuilder private $selectedQueryColumns; + private const STATUSES = ['published', 'draft', 'scheduled', 'expired']; + const COLUMNS = [ 'id', 'site', 'origin_id', 'published', 'slug', 'uri', 'date', 'collection', 'created_at', 'updated_at', 'order', 'blueprint', @@ -161,4 +163,66 @@ public function with($relations, $callback = null) { return $this; } + + public function whereStatus(string $status) + { + if (! in_array($status, self::STATUSES)) { + throw new \Exception("Invalid status [$status]"); + } + + if ($status === 'draft') { + return $this->where('published', false); + } + + $this->where('published', true); + + return $this->where(fn ($query) => $this + ->getCollectionsForStatus() + ->each(fn ($collection) => $query->orWhere(fn ($q) => $this->addCollectionStatusLogicToQuery($q, $status, $collection)))); + } + + private function getCollectionsForStatus() + { + // Since we have to add nested queries for each collection, if collections have been provided, + // we'll use those to avoid the need for adding unnecessary query clauses. + + $wheres = collect($this->builder->getQuery()->wheres); + + if ($wheres->where('column', 'collection')->isEmpty()) { + return Collection::all(); + } + + return $wheres->where('column', 'collection')->pluck('value')->map(fn ($handle) => Collection::find($handle)); + } + + private function addCollectionStatusLogicToQuery($query, $status, $collection) + { + $query->where('collection', $collection->handle()); + + if ($collection->futureDateBehavior() === 'public' && $collection->pastDateBehavior() === 'public') { + if ($status === 'scheduled' || $status === 'expired') { + $query->where('date', 'invalid'); // intentionally trigger no results. + } + } + + if ($collection->futureDateBehavior() === 'private') { + $status === 'scheduled' + ? $query->where('date', '>', now()) + : $query->where('date', '<', now()); + + if ($status === 'expired') { + $query->where('date', 'invalid'); // intentionally trigger no results. + } + } + + if ($collection->pastDateBehavior() === 'private') { + $status === 'expired' + ? $query->where('date', '<', now()) + : $query->where('date', '>', now()); + + if ($status === 'scheduled') { + $query->where('date', 'invalid'); // intentionally trigger no results. + } + } + } } From 26d9e18853d49120a1c60cc4e22a81bc4110b688 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Wed, 1 May 2024 11:19:06 +0100 Subject: [PATCH 02/11] Throw exceptions when querying `status` column --- src/Entries/EntryQueryBuilder.php | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/Entries/EntryQueryBuilder.php b/src/Entries/EntryQueryBuilder.php index 592a8b1c..4bb7fcc3 100644 --- a/src/Entries/EntryQueryBuilder.php +++ b/src/Entries/EntryQueryBuilder.php @@ -164,6 +164,24 @@ public function with($relations, $callback = null) return $this; } + public function where($column, $operator = null, $value = null, $boolean = 'and') + { + if ($column === 'status') { + trigger_error('Filtering by status is deprecated. Use whereStatus() instead.', E_USER_DEPRECATED); + } + + return parent::where($column, $operator, $value, $boolean); + } + + public function whereIn($column, $values, $boolean = 'and') + { + if ($column === 'status') { + trigger_error('Filtering by status is deprecated. Use whereStatus() instead.', E_USER_DEPRECATED); + } + + return parent::whereIn($column, $values, $boolean); + } + public function whereStatus(string $status) { if (! in_array($status, self::STATUSES)) { From 7cb5740119a484e54e09c8fc7f0b42fd27c939b4 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Wed, 1 May 2024 11:43:45 +0100 Subject: [PATCH 03/11] Add tests --- tests/Data/Entries/EntryQueryBuilderTest.php | 92 ++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/tests/Data/Entries/EntryQueryBuilderTest.php b/tests/Data/Entries/EntryQueryBuilderTest.php index 9d08e574..3de87730 100644 --- a/tests/Data/Entries/EntryQueryBuilderTest.php +++ b/tests/Data/Entries/EntryQueryBuilderTest.php @@ -794,4 +794,96 @@ public function entries_can_be_ordered_by_an_date_json_field() $this->assertCount(3, $entries); $this->assertEquals(['Post 2', 'Post 1', 'Post 3'], $entries->map->title->all()); } + + /** @test */ + public function filtering_using_where_status_column_writes_deprecation_log() + { + $this->withoutDeprecationHandling(); + $this->expectException(\ErrorException::class); + $this->expectExceptionMessage('Filtering by status is deprecated. Use whereStatus() instead.'); + + $this->createDummyCollectionAndEntries(); + + Entry::query()->where('collection', 'posts')->where('status', 'published')->get(); + } + + /** @test */ + public function filtering_using_whereIn_status_column_writes_deprecation_log() + { + $this->withoutDeprecationHandling(); + $this->expectException(\ErrorException::class); + $this->expectExceptionMessage('Filtering by status is deprecated. Use whereStatus() instead.'); + + $this->createDummyCollectionAndEntries(); + + Entry::query()->where('collection', 'posts')->whereIn('status', ['published'])->get(); + } + + /** @test */ + public function filtering_by_unexpected_status_throws_exception() + { + $this->expectExceptionMessage('Invalid status [foo]'); + + Entry::query()->whereStatus('foo')->get(); + } + + /** + * @test + * + * @dataProvider filterByStatusProvider + */ + public function it_filters_by_status($status, $expected) + { + Collection::make('pages')->dated(false)->save(); + EntryFactory::collection('pages')->slug('page')->published(true)->create(); + EntryFactory::collection('pages')->slug('page-draft')->published(false)->create(); + + Collection::make('blog')->dated(true)->futureDateBehavior('private')->pastDateBehavior('public')->save(); + EntryFactory::collection('blog')->slug('blog-future')->published(true)->date(now()->addDay())->create(); + EntryFactory::collection('blog')->slug('blog-future-draft')->published(false)->date(now()->addDay())->create(); + EntryFactory::collection('blog')->slug('blog-past')->published(true)->date(now()->subDay())->create(); + EntryFactory::collection('blog')->slug('blog-past-draft')->published(false)->date(now()->subDay())->create(); + + Collection::make('events')->dated(true)->futureDateBehavior('public')->pastDateBehavior('private')->save(); + EntryFactory::collection('events')->slug('event-future')->published(true)->date(now()->addDay())->create(); + EntryFactory::collection('events')->slug('event-future-draft')->published(false)->date(now()->addDay())->create(); + EntryFactory::collection('events')->slug('event-past')->published(true)->date(now()->subDay())->create(); + EntryFactory::collection('events')->slug('event-past-draft')->published(false)->date(now()->subDay())->create(); + + Collection::make('calendar')->dated(true)->futureDateBehavior('public')->pastDateBehavior('public')->save(); + EntryFactory::collection('calendar')->slug('calendar-future')->published(true)->date(now()->addDay())->create(); + EntryFactory::collection('calendar')->slug('calendar-future-draft')->published(false)->date(now()->addDay())->create(); + EntryFactory::collection('calendar')->slug('calendar-past')->published(true)->date(now()->subDay())->create(); + EntryFactory::collection('calendar')->slug('calendar-past-draft')->published(false)->date(now()->subDay())->create(); + + $this->assertEquals($expected, Entry::query()->whereStatus($status)->get()->map->slug()->all()); + } + + public static function filterByStatusProvider() + { + return [ + 'draft' => ['draft', [ + 'page-draft', + 'blog-future-draft', + 'blog-past-draft', + 'event-future-draft', + 'event-past-draft', + 'calendar-future-draft', + 'calendar-past-draft', + ]], + 'published' => ['published', [ + 'page', + 'blog-past', + 'event-future', + 'calendar-future', + 'calendar-past', + ]], + 'scheduled' => ['scheduled', [ + 'blog-future', + ]], + 'expired' => ['expired', [ + 'event-past', + ]], + ]; + } } From 727271fad28d12b095087d6ae16c05ec8cd1669d Mon Sep 17 00:00:00 2001 From: Ryan Mitchell Date: Wed, 1 May 2024 12:27:08 +0100 Subject: [PATCH 04/11] Fix blueprint bug in EntryFactory --- tests/Factories/EntryFactory.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/Factories/EntryFactory.php b/tests/Factories/EntryFactory.php index 01a03d49..536b531d 100644 --- a/tests/Factories/EntryFactory.php +++ b/tests/Factories/EntryFactory.php @@ -89,9 +89,12 @@ public function origin($origin) public function make() { + $collection = $this->createCollection(); + $entry = Entry::make() ->locale($this->locale) - ->collection($this->createCollection()) + ->collection($collection) + ->blueprint($collection->entryBlueprint()) ->slug($this->slug) ->data($this->data) ->origin($this->origin) From 41e1ee851e6ce986e241d2a31f4bebe8484e2255 Mon Sep 17 00:00:00 2001 From: Jason Varga Date: Thu, 2 May 2024 23:02:02 -0400 Subject: [PATCH 05/11] use trait --- src/Entries/EntryQueryBuilder.php | 72 ++++++++++--------------------- 1 file changed, 22 insertions(+), 50 deletions(-) diff --git a/src/Entries/EntryQueryBuilder.php b/src/Entries/EntryQueryBuilder.php index 4bb7fcc3..7319623e 100644 --- a/src/Entries/EntryQueryBuilder.php +++ b/src/Entries/EntryQueryBuilder.php @@ -8,11 +8,13 @@ use Statamic\Facades\Collection; use Statamic\Facades\Entry; use Statamic\Query\EloquentQueryBuilder; +use Statamic\Stache\Query\QueriesEntryStatus; use Statamic\Stache\Query\QueriesTaxonomizedEntries; class EntryQueryBuilder extends EloquentQueryBuilder implements QueryBuilder { - use QueriesTaxonomizedEntries; + use QueriesEntryStatus, + QueriesTaxonomizedEntries; private $selectedQueryColumns; @@ -182,65 +184,35 @@ public function whereIn($column, $values, $boolean = 'and') return parent::whereIn($column, $values, $boolean); } - public function whereStatus(string $status) + private function ensureCollectionsAreQueriedForStatusQuery() { - if (! in_array($status, self::STATUSES)) { - throw new \Exception("Invalid status [$status]"); - } + $wheres = collect($this->builder->getQuery()->wheres); - if ($status === 'draft') { - return $this->where('published', false); + // If there are where clauses on the collection column, it means the user has explicitly + // queried for them. In that case, we'll use them and skip the auto-detection. + if ($wheres->where('column', 'collection')->isNotEmpty()) { + return; } - $this->where('published', true); + // Otherwise, we'll detect them by looking at where clauses targeting the "id" column. + $ids = $wheres->where('column', 'id')->flatMap(fn ($where) => $where['values'] ?? [$where['value']]); - return $this->where(fn ($query) => $this - ->getCollectionsForStatus() - ->each(fn ($collection) => $query->orWhere(fn ($q) => $this->addCollectionStatusLogicToQuery($q, $status, $collection)))); - } - - private function getCollectionsForStatus() - { - // Since we have to add nested queries for each collection, if collections have been provided, - // we'll use those to avoid the need for adding unnecessary query clauses. - - $wheres = collect($this->builder->getQuery()->wheres); - - if ($wheres->where('column', 'collection')->isEmpty()) { - return Collection::all(); + // If no IDs were queried, fall back to all collections. + if ($ids->isEmpty()) { + return $this->whereIn('collection', Collection::handles()); } - return $wheres->where('column', 'collection')->pluck('value')->map(fn ($handle) => Collection::find($handle)); + $this->whereIn('collection', app(static::class)->whereIn('id', $ids)->pluck('collection')->unique()->values()); } - private function addCollectionStatusLogicToQuery($query, $status, $collection) + private function getCollectionsForStatusQuery() { - $query->where('collection', $collection->handle()); - - if ($collection->futureDateBehavior() === 'public' && $collection->pastDateBehavior() === 'public') { - if ($status === 'scheduled' || $status === 'expired') { - $query->where('date', 'invalid'); // intentionally trigger no results. - } - } - - if ($collection->futureDateBehavior() === 'private') { - $status === 'scheduled' - ? $query->where('date', '>', now()) - : $query->where('date', '<', now()); + // Since we have to add nested queries for each collection, we only want to add clauses for the + // applicable collections. By this point, there should be where clauses on the collection column. - if ($status === 'expired') { - $query->where('date', 'invalid'); // intentionally trigger no results. - } - } - - if ($collection->pastDateBehavior() === 'private') { - $status === 'expired' - ? $query->where('date', '<', now()) - : $query->where('date', '>', now()); - - if ($status === 'scheduled') { - $query->where('date', 'invalid'); // intentionally trigger no results. - } - } + return collect($this->builder->getQuery()->wheres) + ->where('column', 'collection') + ->flatMap(fn ($where) => $where['values'] ?? [$where['value']]) + ->map(fn ($handle) => Collection::find($handle)); } } From 455eebefa7914a3b73cff82538b19d3209bd0c7c Mon Sep 17 00:00:00 2001 From: Jason Varga Date: Thu, 2 May 2024 23:04:32 -0400 Subject: [PATCH 06/11] typehints --- src/Entries/EntryQueryBuilder.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Entries/EntryQueryBuilder.php b/src/Entries/EntryQueryBuilder.php index 7319623e..0a37b5c3 100644 --- a/src/Entries/EntryQueryBuilder.php +++ b/src/Entries/EntryQueryBuilder.php @@ -184,7 +184,7 @@ public function whereIn($column, $values, $boolean = 'and') return parent::whereIn($column, $values, $boolean); } - private function ensureCollectionsAreQueriedForStatusQuery() + private function ensureCollectionsAreQueriedForStatusQuery(): void { $wheres = collect($this->builder->getQuery()->wheres); @@ -205,7 +205,7 @@ private function ensureCollectionsAreQueriedForStatusQuery() $this->whereIn('collection', app(static::class)->whereIn('id', $ids)->pluck('collection')->unique()->values()); } - private function getCollectionsForStatusQuery() + private function getCollectionsForStatusQuery(): \Illuminate\Support\Collection { // Since we have to add nested queries for each collection, we only want to add clauses for the // applicable collections. By this point, there should be where clauses on the collection column. From 02622dcffc2bdb2233fc1d48e9a5284ca7aaf58d Mon Sep 17 00:00:00 2001 From: Jason Varga Date: Thu, 2 May 2024 23:04:42 -0400 Subject: [PATCH 07/11] ternary --- src/Entries/EntryQueryBuilder.php | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/Entries/EntryQueryBuilder.php b/src/Entries/EntryQueryBuilder.php index 0a37b5c3..0eac75dc 100644 --- a/src/Entries/EntryQueryBuilder.php +++ b/src/Entries/EntryQueryBuilder.php @@ -198,11 +198,9 @@ private function ensureCollectionsAreQueriedForStatusQuery(): void $ids = $wheres->where('column', 'id')->flatMap(fn ($where) => $where['values'] ?? [$where['value']]); // If no IDs were queried, fall back to all collections. - if ($ids->isEmpty()) { - return $this->whereIn('collection', Collection::handles()); - } - - $this->whereIn('collection', app(static::class)->whereIn('id', $ids)->pluck('collection')->unique()->values()); + $ids->isEmpty() + ? $this->whereIn('collection', Collection::handles()) + : $this->whereIn('collection', app(static::class)->whereIn('id', $ids)->pluck('collection')->unique()->values()); } private function getCollectionsForStatusQuery(): \Illuminate\Support\Collection From ca74d84e4e48a44b3a75a05a6034163025f22764 Mon Sep 17 00:00:00 2001 From: Jason Varga Date: Thu, 2 May 2024 23:10:09 -0400 Subject: [PATCH 08/11] temporarily use branch to get tests running --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index ec810440..51125a5b 100644 --- a/composer.json +++ b/composer.json @@ -25,7 +25,7 @@ }, "require": { "php": "^8.1", - "statamic/cms": "v5.0.0-alpha.6" + "statamic/cms": "dev-where-status-trait" }, "require-dev": { "doctrine/dbal": "^3.3", From 782761e63ad69e38e4692829e1015aea69268910 Mon Sep 17 00:00:00 2001 From: Jason Varga Date: Thu, 2 May 2024 23:29:46 -0400 Subject: [PATCH 09/11] bump From 2240e49a763caa347fb92fd9d270c35836d345a5 Mon Sep 17 00:00:00 2001 From: Jason Varga Date: Thu, 2 May 2024 23:34:17 -0400 Subject: [PATCH 10/11] order doesnt matter --- tests/Data/Entries/EntryQueryBuilderTest.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/Data/Entries/EntryQueryBuilderTest.php b/tests/Data/Entries/EntryQueryBuilderTest.php index 3de87730..88240348 100644 --- a/tests/Data/Entries/EntryQueryBuilderTest.php +++ b/tests/Data/Entries/EntryQueryBuilderTest.php @@ -856,27 +856,27 @@ public function it_filters_by_status($status, $expected) EntryFactory::collection('calendar')->slug('calendar-past')->published(true)->date(now()->subDay())->create(); EntryFactory::collection('calendar')->slug('calendar-past-draft')->published(false)->date(now()->subDay())->create(); - $this->assertEquals($expected, Entry::query()->whereStatus($status)->get()->map->slug()->all()); + $this->assertEquals($expected, Entry::query()->whereStatus($status)->get()->map->slug()->sort()->all()); } public static function filterByStatusProvider() { return [ 'draft' => ['draft', [ - 'page-draft', 'blog-future-draft', 'blog-past-draft', - 'event-future-draft', - 'event-past-draft', 'calendar-future-draft', 'calendar-past-draft', + 'event-future-draft', + 'event-past-draft', + 'page-draft', ]], 'published' => ['published', [ - 'page', 'blog-past', - 'event-future', 'calendar-future', 'calendar-past', + 'event-future', + 'page', ]], 'scheduled' => ['scheduled', [ 'blog-future', From ef22b78776915ac5f429faa47d601df3e659feb1 Mon Sep 17 00:00:00 2001 From: Jason Varga Date: Thu, 2 May 2024 23:42:16 -0400 Subject: [PATCH 11/11] master --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 51125a5b..681cc389 100644 --- a/composer.json +++ b/composer.json @@ -25,7 +25,7 @@ }, "require": { "php": "^8.1", - "statamic/cms": "dev-where-status-trait" + "statamic/cms": "dev-master" }, "require-dev": { "doctrine/dbal": "^3.3",