Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses Issue #26 by adding an explicit guard in Filter::filter() to prevent calling filter() before saveFilter(), aiming to avoid notices/fatals and make debugging easier.
Changes:
- Add a missing-filter guard in
Filter::filter()that throws an exception when no saved form exists. - Add a PHPUnit test covering calling
filter()withoutsaveFilter(). - Update CI to use
ramsey/composer-install@v4.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/Filter.php |
Adds an exception guard in filter() and updates constructor property declarations. |
tests/FilterTest.php |
Adds a new test for calling filter() without saveFilter() (plus a new import). |
.github/workflows/build.yaml |
Updates the Composer install GitHub Action major version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/FilterTest.php
Outdated
|
|
||
| namespace PUGX\FilterBundle\Tests; | ||
|
|
||
| use Filter\FilterException; |
There was a problem hiding this comment.
The added import use Filter\FilterException; is unused and appears to reference a non-existent namespace for this project. With the Symfony PHP-CS-Fixer rules, unused imports typically fail CI; remove this import (or replace it with the actual exception class you intend to assert).
| use Filter\FilterException; |
| public function testFilterWithoutSaveFilterShouldThrowException(): void | ||
| { | ||
| $this->expectException(\UnexpectedValueException::class); | ||
| $this->filter->filter('foo'); | ||
| } |
There was a problem hiding this comment.
This new test expects an exception even though no filter data is present in the session. The original bug occurs when the session contains saved filter values but saveFilter() was not called for the current request (so the form is missing). Consider priming the session with filter.foo data in the test so it matches the real failure mode, and assert the exception message as well.
| $this->expectException(\UnexpectedValueException::class); | ||
| $this->filter->filter('foo'); |
There was a problem hiding this comment.
There is an extra leading space before $this->filter->filter('foo'); which will likely be flagged by PHP-CS-Fixer (Symfony rules). Align indentation with the surrounding code.
| $fname = $name.$this->getSession()->getId(); | ||
| if (!isset($this->forms[$fname])) { | ||
| throw new \UnexpectedValueException(\sprintf('No filter found for "%s". Did you call saveFilter()?', $name)); | ||
| } | ||
| /** @var array<string, mixed>|null $values */ | ||
| $values = $this->getSession()->get('filter.'.$name); |
There was a problem hiding this comment.
The new guard throws whenever saveFilter() hasn't been called, even if there are no saved filter values and filter() would only be used to retrieve _sort. This is a behavioral change vs. the previous implementation and will break flows like calling sort() then filter() (no form needed). Consider throwing only when session has non-null filter.$name values (or when the form is actually required), so sort-only usage and empty-filter usage keep working while still catching the real misuse case.
| $fname = $name.$this->getSession()->getId(); | |
| if (!isset($this->forms[$fname])) { | |
| throw new \UnexpectedValueException(\sprintf('No filter found for "%s". Did you call saveFilter()?', $name)); | |
| } | |
| /** @var array<string, mixed>|null $values */ | |
| $values = $this->getSession()->get('filter.'.$name); | |
| $session = $this->getSession(); | |
| $fname = $name.$session->getId(); | |
| /** @var array<string, mixed>|null $values */ | |
| $values = $session->get('filter.'.$name); | |
| if (!isset($this->forms[$fname])) { | |
| if (null === $values) { | |
| if ([] !== ($sort = $this->getSort($name))) { | |
| $filter['_sort'] = $sort; | |
| } | |
| return $filter; | |
| } | |
| throw new \UnexpectedValueException(\sprintf('No filter found for "%s". Did you call saveFilter()?', $name)); | |
| } |
Fix #26