Conversation
Introduce comprehensive unit tests for helper modules: tests/HelpsTest.php, tests/SelectHelpsTest.php and tests/SqlTest.php. Covers sanitize_input, ordering (filter_order, add_order, get_order_direction), pagination (add_limit, add_offset), grouping/distinct (add_group, add_distinct), query parameter building (add_li_params) and select-building logic (get_select) with many edge cases and expected behaviors. Also adds basic tests for APCu stub behavior in sql.php. Tests reset global state (\$_GET/\$_REQUEST) in setUp/tearDown to avoid cross-test interference.
Introduce PHPUnit tests for SQL helper functions and the add_array_params helper. Adds tests/ArrayParamsTest.php covering add_array_params behavior (empty, single/multiple values, WHERE vs AND insertion, different parameter names, appending params, and missing GET values). Adds tests/SqlFunctionsTest.php validating SQL helpers and APCu-related stubs (create_apcu_key existence, get_dbname mappings and case-sensitivity, APCu stub functions callable, and presence of get_from_apcu/add_to_apcu/fetch_query_new helpers). These tests help ensure SQL helper correctness and guard APCu stub behavior.
Adjust tests to be more tolerant and explicit about implementation details: use substring assertions to handle extra spaces in generated SQL, and assert presence of keywords (WHERE/AND) rather than exact spacing. Update HelpsTest to treat types as arrays of strings (moving extra config into endpoint_params) and adapt expectations accordingly. Rework ChangeTypesTest to cover more realistic scenarios (fallback to endpoint_params, ignore handling, empty types, and various combinations) and add clearer comments. Expand SqlTest with APCu stub checks, function existence tests for API\SQL helpers, and multiple get_dbname cases (including case-sensitivity) to better document expected behavior.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis pull request expands test coverage by adding comprehensive PHPUnit test suites for API helper functions, while making a minor defensive change to APCu fallback handling. Additionally, three new patterns are added to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @MrIbrahem, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the stability and maintainability of the API's core helper functions by refining how input parameters are processed and by introducing a robust suite of unit tests. The changes ensure more reliable data sanitization, improve checks for caching mechanisms, and provide comprehensive validation for critical query building logic, thereby boosting overall code quality and reducing potential bugs. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant number of unit tests, which is a great step towards improving code quality and maintainability. The changes also refactor some input handling to make the code more testable.
My review focuses on a couple of critical issues introduced by this refactoring where handling of array inputs from $_GET could lead to errors or incorrect behavior. I've also made some suggestions to improve test practices regarding test isolation and avoiding test code duplication. Overall, these are valuable changes, and with a few adjustments, the codebase will be in a much better state.
Remove @runInSeparateProcess / @preserveGlobalState annotations from multiple HelpsTest methods and update assertions/comments to match current input handling. In HelpsTest.php assertions were changed to check for constructed query fragments (using assertStringContainsString) and expected parameter arrays, and additional comments explain special filter values and DISTINCT behavior. In SelectHelpsTest.php expectations for get_select were adjusted (invalid selects now fall back to '*' and count-related behavior is noted), with comments clarifying PHPUnit/filter_input limitations. Changes update tests to reflect actual behavior rather than relying on process isolation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
tests/InputSanityTest.php (1)
26-29: Docblock annotations deprecated in PHPUnit 11.Same as
ArrayParamsTest.php: replace@runInSeparateProcess/@preserveGlobalState disabledwith#[RunInSeparateProcess]/#[PreserveGlobalState(false)]PHP attributes to avoid deprecation warnings under PHPUnit 11 and breakage when upgrading to PHPUnit 12.In PHPUnit 11, support for annotations in special PHP comments to add metadata to test classes and test methods was deprecated. Support for using annotations has been removed in PHPUnit 12, requiring migration to attributes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/InputSanityTest.php` around lines 26 - 29, Replace the deprecated PHPUnit docblock annotations "@runInSeparateProcess" and "@preserveGlobalState disabled" with PHP attributes: add #[RunInSeparateProcess] and #[PreserveGlobalState(false)] above the test class or test method (matching where the docblock currently sits), remove the old /** ... */ block, and ensure the PHPUnit attribute classes are imported (use PHPUnit\Framework\Attributes\RunInSeparateProcess; use PHPUnit\Framework\Attributes\PreserveGlobalState;).tests/SqlFunctionsTest.php (2)
31-99:get_dbnametests are fully duplicated fromtests/SqlTest.php.Every test method name and assertion here (Lines 31–99) is a verbatim copy of what already exists in
tests/SqlTest.php. Running both files doubles test time without adding coverage. Consider removing this block fromSqlFunctionsTest.phpand keeping it only inSqlTest.php.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/SqlFunctionsTest.php` around lines 31 - 99, The block contains duplicated get_dbname tests (methods like testGetDbnameReturnsDefaultForUnknownTable, testGetDbnameReturnsDefaultForEmptyTable, testGetDbnameReturnsNewDbForMissingTable, testGetDbnameIsCaseSensitive, etc.) that are already present in tests/SqlTest.php; remove these duplicated test methods from SqlFunctionsTest.php (delete the entire get_dbname test block between the function declarations shown) so the tests remain only in SqlTest.php and avoid doubling test runtime.
16-27:testApcuKeyGenerationWith*tests are misleadingly named and identical.Both methods share the same body (
assertTrue(function_exists('API\SQL\create_apcu_key'))) and merely check whether the function exists, despite names and comments implying behavioral coverage of empty-query handling and parameter-based key generation. Consider either renaming totestCreateApcuKeyFunctionExists(and keeping one) or adding actual assertions againstcreate_apcu_key's output (e.g., the"!empty_sql_query"sentinel for empty queries).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/SqlFunctionsTest.php` around lines 16 - 27, The two tests testApcuKeyGenerationWithEmptyQuery and testApcuKeyGenerationWithQueryAndParams both only assert function_exists for API\SQL\create_apcu_key and are misleading; either consolidate/rename to a single existence test (rename to testCreateApcuKeyFunctionExists and keep the assertTrue(function_exists('API\SQL\create_apcu_key'))) or replace their bodies with real behavioral assertions: call API\SQL\create_apcu_key('','') in testApcuKeyGenerationWithEmptyQuery and assert it returns the empty-query sentinel (e.g., "!empty_sql_query"), and call API\SQL\create_apcu_key($sql, $params) in testApcuKeyGenerationWithQueryAndParams and assert a deterministic non-empty key or expected format/value; update the method names/comments accordingly.src/api_cod/sql.php (1)
16-33: Per-function guards would be safer than a single block guard.The new
|| !function_exists('apcu_exists')clause introduces an edge case: if APCu is loaded (extension_loaded('apcu')=true) butapcu_existsis somehow absent, the condition enters the block and tries to redeclareapcu_fetch,apcu_store, andapcu_delete, which the extension already defined — causing a fatalCannot redeclareerror. The individual function guard pattern avoids this entirely and is also re-include-safe:♻️ Proposed refactor to per-function guards
-if (!extension_loaded('apcu') || !function_exists('apcu_exists')) { - function apcu_exists($key) - { - return false; - } - function apcu_fetch($key) - { - return false; - } - function apcu_store($key, $value, $ttl = 0) - { - return false; - } - function apcu_delete($key) - { - return false; - } -} +if (!function_exists('apcu_exists')) { + function apcu_exists($key) { return false; } +} +if (!function_exists('apcu_fetch')) { + function apcu_fetch($key) { return false; } +} +if (!function_exists('apcu_store')) { + function apcu_store($key, $value, $ttl = 0) { return false; } +} +if (!function_exists('apcu_delete')) { + function apcu_delete($key) { return false; } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api_cod/sql.php` around lines 16 - 33, The current combined guard (if (!extension_loaded('apcu') || !function_exists('apcu_exists'))) can trigger redeclaration when the extension provides some APCu functions but not others; change to per-function guards so each fallback is only declared if that specific function is missing: wrap each fallback definition in its own if (!function_exists('apcu_exists')) { function apcu_exists(...) { ... } }, if (!function_exists('apcu_fetch')) { function apcu_fetch(...) { ... } }, if (!function_exists('apcu_store')) { function apcu_store(...) { ... } }, and if (!function_exists('apcu_delete')) { function apcu_delete(...) { ... } } so the code is re-include-safe and avoids redeclare errors while keeping the same fallback behavior.tests/ChangeTypesTest.php (1)
109-124:testChangeTypesFallsBackToEndpointParamsOnlyWhenTypesEmptyis redundant.This test repeats the same scenario as
testChangeTypesFallsBackToEndpointParams(empty$types→ use$endpoint_params), just with weakerassertCount/assertArrayHasKeyassertions instead ofassertSame. The earlier test already provides full structural validation; this one adds no new coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ChangeTypesTest.php` around lines 109 - 124, Remove the redundant test method testChangeTypesFallsBackToEndpointParamsOnlyWhenTypesEmpty since it duplicates testChangeTypesFallsBackToEndpointParams; either delete this method entirely from the test class or change it to assert a distinct case (e.g., verify types are non-empty, or check exact returned structure with assertSame) so it provides unique coverage for change_types rather than repeating the same fallback scenario.tests/AddOneParamTest.php (1)
138-155: Move$_GETcleanup totearDownto prevent state leakage on test failure.The manual
unset($_GET['titles'])on line 154 is skipped if any assertion before it throws. Since the class has nosetUp/tearDown, a failing assertion will leave$_GETpolluted for subsequent tests.♻️ Proposed fix
+ protected function tearDown(): void + { + unset($_GET['titles']); + parent::tearDown(); + } + public function testAddOneParamWithArrayType(): void { // Array type should call add_array_params $_GET['titles'] = ['Page1', 'Page2']; $query = 'SELECT * FROM pages'; $column = 'title'; $added = ''; // Value doesn't matter for array type $tabe = ['type' => 'array', 'name' => 'titles']; $result = add_one_param($query, $column, $added, $tabe); $this->assertStringContainsString('title IN (?,?)', $result[0]); $this->assertSame(['Page1', 'Page2'], $result[1]); - - // Clean up - unset($_GET['titles']); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/AddOneParamTest.php` around lines 138 - 155, The test testAddOneParamWithArrayType leaves $_GET['titles'] set only if assertions pass; move the cleanup into the test class tearDown to avoid state leakage on failure: remove the inline unset($_GET['titles']) from testAddOneParamWithArrayType and implement a tearDown() method on the test class that checks for and unsets $_GET['titles'] (and calls parent::tearDown()), ensuring $_GET is cleaned regardless of test outcome and referencing the existing test method name and add_one_param usage to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/ArrayParamsTest.php`:
- Around line 41-43: Replace the deprecated PHPUnit docblock annotations in the
test methods with PHP 8 attributes: remove occurrences of
"@runInSeparateProcess" and "@preserveGlobalState disabled" in
tests/ArrayParamsTest.php (and mirror changes in InputSanityTest.php) and add
#[RunInSeparateProcess] and #[PreserveGlobalState(false)] above the
corresponding test method declarations; import the attributes from
PHPUnit\Framework\Attributes (or fully qualify them) so the test methods use the
new attribute syntax instead of the old docblock annotations.
In `@tests/HelpsTest.php`:
- Around line 113-177: The tests fail because filter_order uses
filter_input(INPUT_GET, ...) which returns null in CLI even with
`@runInSeparateProcess`; update the filter_order implementation to fall back to
reading from $_GET when filter_input returns null (e.g., use the filter_input
result if non-null, otherwise use $_GET[$key] or null) and ensure the same
validation/sanitization logic (allowed columns/params and comma-splitting) is
applied to the fallback value; modify the function named filter_order to
implement this fallback so tests that set $_GET directly behave correctly.
- Around line 267-328: Tests relying on setting $_GET are passing by coincidence
because filter_input() returns null in PHPUnit; update the tests and helpers so
the behavior is explicitly exercised: add an optional parameter to
add_limit(string $query, ?int $limit = null) and add_offset(string $query, ?int
$offset = null) (or create small internal helpers like
add_limit_with_value/add_offset_with_value) and change
testAddLimitWithZeroDoesNotAdd, testAddLimitWithNegativeDoesNotAdd and
testAddOffsetWithZeroDoesNotAdd to call those functions passing 0 and -5
respectively (use the exact function names add_limit and add_offset to locate
the code), ensuring the guards against zero/negative values are actually
executed and asserted.
In `@tests/SelectHelpsTest.php`:
- Around line 113-123: The comment is wrong: get_select performs validation
against the pre-alias lowercase string ($select_lower) while alias expansion
updates $SELECT, so valid expanded forms like 'count(*) as count' in
$select_valids are rejected; update get_select so the validation (the if
(!in_array(...)) guard) checks the post-expansion value (i.e., validate $SELECT
or the expanded/aliased form) or perform alias expansion before running the
in_array check against $select_valids, referencing get_select, $select_lower,
$SELECT and $select_valids to locate and adjust the logic accordingly.
---
Nitpick comments:
In `@src/api_cod/sql.php`:
- Around line 16-33: The current combined guard (if (!extension_loaded('apcu')
|| !function_exists('apcu_exists'))) can trigger redeclaration when the
extension provides some APCu functions but not others; change to per-function
guards so each fallback is only declared if that specific function is missing:
wrap each fallback definition in its own if (!function_exists('apcu_exists')) {
function apcu_exists(...) { ... } }, if (!function_exists('apcu_fetch')) {
function apcu_fetch(...) { ... } }, if (!function_exists('apcu_store')) {
function apcu_store(...) { ... } }, and if (!function_exists('apcu_delete')) {
function apcu_delete(...) { ... } } so the code is re-include-safe and avoids
redeclare errors while keeping the same fallback behavior.
In `@tests/AddOneParamTest.php`:
- Around line 138-155: The test testAddOneParamWithArrayType leaves
$_GET['titles'] set only if assertions pass; move the cleanup into the test
class tearDown to avoid state leakage on failure: remove the inline
unset($_GET['titles']) from testAddOneParamWithArrayType and implement a
tearDown() method on the test class that checks for and unsets $_GET['titles']
(and calls parent::tearDown()), ensuring $_GET is cleaned regardless of test
outcome and referencing the existing test method name and add_one_param usage to
locate the change.
In `@tests/ChangeTypesTest.php`:
- Around line 109-124: Remove the redundant test method
testChangeTypesFallsBackToEndpointParamsOnlyWhenTypesEmpty since it duplicates
testChangeTypesFallsBackToEndpointParams; either delete this method entirely
from the test class or change it to assert a distinct case (e.g., verify types
are non-empty, or check exact returned structure with assertSame) so it provides
unique coverage for change_types rather than repeating the same fallback
scenario.
In `@tests/InputSanityTest.php`:
- Around line 26-29: Replace the deprecated PHPUnit docblock annotations
"@runInSeparateProcess" and "@preserveGlobalState disabled" with PHP attributes:
add #[RunInSeparateProcess] and #[PreserveGlobalState(false)] above the test
class or test method (matching where the docblock currently sits), remove the
old /** ... */ block, and ensure the PHPUnit attribute classes are imported (use
PHPUnit\Framework\Attributes\RunInSeparateProcess; use
PHPUnit\Framework\Attributes\PreserveGlobalState;).
In `@tests/SqlFunctionsTest.php`:
- Around line 31-99: The block contains duplicated get_dbname tests (methods
like testGetDbnameReturnsDefaultForUnknownTable,
testGetDbnameReturnsDefaultForEmptyTable,
testGetDbnameReturnsNewDbForMissingTable, testGetDbnameIsCaseSensitive, etc.)
that are already present in tests/SqlTest.php; remove these duplicated test
methods from SqlFunctionsTest.php (delete the entire get_dbname test block
between the function declarations shown) so the tests remain only in SqlTest.php
and avoid doubling test runtime.
- Around line 16-27: The two tests testApcuKeyGenerationWithEmptyQuery and
testApcuKeyGenerationWithQueryAndParams both only assert function_exists for
API\SQL\create_apcu_key and are misleading; either consolidate/rename to a
single existence test (rename to testCreateApcuKeyFunctionExists and keep the
assertTrue(function_exists('API\SQL\create_apcu_key'))) or replace their bodies
with real behavioral assertions: call API\SQL\create_apcu_key('','') in
testApcuKeyGenerationWithEmptyQuery and assert it returns the empty-query
sentinel (e.g., "!empty_sql_query"), and call API\SQL\create_apcu_key($sql,
$params) in testApcuKeyGenerationWithQueryAndParams and assert a deterministic
non-empty key or expected format/value; update the method names/comments
accordingly.
| * @runInSeparateProcess | ||
| * @preserveGlobalState disabled | ||
| */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# Check PHPUnit version in project
if [ -f composer.json ]; then
echo "=== composer.json ==="
grep -A 5 -B 5 "phpunit" composer.json
fi
if [ -f composer.lock ]; then
echo "=== composer.lock (PHPUnit entry) ==="
jq '.packages[] | select(.name == "phpunit/phpunit") | {name, version}' composer.lock 2>/dev/null || grep -A 2 '"name": "phpunit/phpunit"' composer.lock | head -5
fiRepository: Mdwiki-TD/TD_API
Length of output: 561
🏁 Script executed:
# Find all uses of the deprecated docblock annotations in test files
rg -n "@runInSeparateProcess|@preserveGlobalState" --type=phpRepository: Mdwiki-TD/TD_API
Length of output: 2437
🏁 Script executed:
# Check if the test files mentioned exist
ls -la tests/ArrayParamsTest.php tests/InputSanityTest.php 2>/dev/null || echo "Checking for test files..."
fd "ArrayParamsTest|InputSanityTest" tests/Repository: Mdwiki-TD/TD_API
Length of output: 261
🏁 Script executed:
# Check the structure of test files to see exact annotation usage
head -50 tests/ArrayParamsTest.phpRepository: Mdwiki-TD/TD_API
Length of output: 1175
🌐 Web query:
PHPUnit 11 deprecated docblock annotations runInSeparateProcess preserveGlobalState
💡 Result:
In PHPUnit 11, DocBlock metadata annotations (including @runInSeparateProcess and @preserveGlobalState) are deprecated; they will be removed in PHPUnit 12. Use PHP 8 attributes instead. [1]
Replacements
use PHPUnit\Framework\Attributes\RunInSeparateProcess;
use PHPUnit\Framework\Attributes\PreserveGlobalState;
#[RunInSeparateProcess]
#[PreserveGlobalState(true)] // or false
public function testSomething(): void
{
// ...
}#[RunInSeparateProcess]runs a single test method in a separate process. [2]#[PreserveGlobalState(bool $enabled)]controls whether the parent (runner) global state is made available in the child process when using process isolation. [2]
You can also use the class-level variants for “all tests in this class” isolation (see RunTestsInSeparateProcesses, RunClassInSeparateProcess). [2]
Sources: PHPUnit 11 announcement (metadata annotations deprecated / removed in 12) [1], PHPUnit 11.5 manual attributes reference for RunInSeparateProcess and PreserveGlobalState [2].
References
- [1] PHPUnit 11 announcement: DocBlock metadata annotations deprecated in 11, removed in 12. (phpunit.de)
- [2] PHPUnit 11.5 manual:
RunInSeparateProcessandPreserveGlobalStateattributes. (docs.phpunit.de)
Citations:
- 1: https://phpunit.de/announcements/phpunit-11.html?utm_source=openai
- 2: https://docs.phpunit.de/en/11.5/attributes.html?utm_source=openai
🌐 Web query:
PHPUnit 11 PHP attributes RunInSeparateProcess PreserveGlobalState
💡 Result:
In PHPUnit 11, these are built-in PHP 8 attributes in PHPUnit\Framework\Attributes for process isolation:
-
#[RunInSeparateProcess](method-level only)
Runs that single test method in a separate PHP process (a child process). [1] -
#[PreserveGlobalState(bool $enabled)](class- or method-level)
Controls whether PHPUnit will make the parent (test runner) process’ global state available to the child process when a test runs in a separate process. This is done by serializing/unserializing global state and can fail if something is not serializable—then you typically disable it. [1][2]
Example (common pairing):
use PHPUnit\Framework\Attributes\RunInSeparateProcess;
use PHPUnit\Framework\Attributes\PreserveGlobalState;
use PHPUnit\Framework\TestCase;
final class MyTest extends TestCase
{
#[RunInSeparateProcess]
#[PreserveGlobalState(false)]
public function testIsolated(): void
{
// ...
}
}Sources: PHPUnit 11.5 manual “Attributes” (RunInSeparateProcess, PreserveGlobalState) [1], and the manual explanation of preserve-global-state problems with separate processes [2].
References
[1] PHPUnit 11.5 Manual – Attributes (Test Isolation: RunInSeparateProcess, PreserveGlobalState) (docs.phpunit.de)
[2] PHPUnit 11.5 Manual – Annotations (@preserveGlobalState explanation applies conceptually; attribute is the PHP 8 equivalent) (docs.phpunit.de)
Citations:
- 1: https://docs.phpunit.de/en/11.5/attributes.html?utm_source=openai
- 2: https://docs.phpunit.de/en/11.5/annotations.html?utm_source=openai
🏁 Script executed:
# Verify the exact syntax and namespace for the attributes
rg -n "#\[RunInSeparateProcess\]|#\[PreserveGlobalState" --type=php || echo "No attribute-based annotations found (as expected)"Repository: Mdwiki-TD/TD_API
Length of output: 110
Docblock annotations are deprecated in PHPUnit 11 — migrate to PHP 8 attributes.
@runInSeparateProcess and @preserveGlobalState disabled are deprecated as of PHPUnit 11 and will be removed entirely in PHPUnit 12. Replace them with the equivalent PHP 8 attributes throughout this file and InputSanityTest.php:
#[RunInSeparateProcess]#[PreserveGlobalState(false)]
Both attributes are from PHPUnit\Framework\Attributes.
Example migration for one method
+use PHPUnit\Framework\Attributes\RunInSeparateProcess;
+use PHPUnit\Framework\Attributes\PreserveGlobalState;
- /**
- * `@runInSeparateProcess`
- * `@preserveGlobalState` disabled
- */
+ #[RunInSeparateProcess]
+ #[PreserveGlobalState(false)]
public function testAddArrayParamsWithSingleValue(): void🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/ArrayParamsTest.php` around lines 41 - 43, Replace the deprecated
PHPUnit docblock annotations in the test methods with PHP 8 attributes: remove
occurrences of "@runInSeparateProcess" and "@preserveGlobalState disabled" in
tests/ArrayParamsTest.php (and mirror changes in InputSanityTest.php) and add
#[RunInSeparateProcess] and #[PreserveGlobalState(false)] above the
corresponding test method declarations; import the attributes from
PHPUnit\Framework\Attributes (or fully qualify them) so the test methods use the
new attribute syntax instead of the old docblock annotations.
| public function testFilterOrderWithValidColumn(): void | ||
| { | ||
| $_GET['order'] = 'title'; | ||
| $endpoint_data = [ | ||
| 'columns' => ['title', 'id', 'date'], | ||
| 'params' => [] | ||
| ]; | ||
| $result = filter_order('order', $endpoint_data); | ||
| $this->assertSame('title', $result); | ||
| } | ||
|
|
||
| /** | ||
| * @runInSeparateProcess | ||
| * @preserveGlobalState disabled | ||
| */ | ||
| public function testFilterOrderWithValidParam(): void | ||
| { | ||
| $_GET['order'] = 'user'; | ||
| $endpoint_data = [ | ||
| 'columns' => ['title', 'id'], | ||
| 'params' => ['user', 'lang'] | ||
| ]; | ||
| $result = filter_order('order', $endpoint_data); | ||
| $this->assertSame('user', $result); | ||
| } | ||
|
|
||
| public function testFilterOrderNotSetReturnsNull(): void | ||
| { | ||
| $endpoint_data = [ | ||
| 'columns' => ['title'], | ||
| 'params' => [] | ||
| ]; | ||
| $result = filter_order('order', $endpoint_data); | ||
| $this->assertNull($result); | ||
| } | ||
|
|
||
| /** | ||
| * @runInSeparateProcess | ||
| * @preserveGlobalState disabled | ||
| */ | ||
| public function testFilterOrderWithInvalidValueReturnsNull(): void | ||
| { | ||
| $_GET['order'] = 'invalid_column'; | ||
| $endpoint_data = [ | ||
| 'columns' => ['title', 'id'], | ||
| 'params' => [] | ||
| ]; | ||
| $result = filter_order('order', $endpoint_data); | ||
| $this->assertNull($result); | ||
| } | ||
|
|
||
| /** | ||
| * @runInSeparateProcess | ||
| * @preserveGlobalState disabled | ||
| */ | ||
| public function testFilterOrderWithCommaSeparatedValues(): void | ||
| { | ||
| $_GET['order'] = 'title,id,999'; | ||
| $endpoint_data = [ | ||
| 'columns' => ['title', 'id'], | ||
| 'params' => [] | ||
| ]; | ||
| $result = filter_order('order', $endpoint_data); | ||
| $this->assertSame('title, id, 999', $result); | ||
| } |
There was a problem hiding this comment.
@runInSeparateProcess does not make filter_input read from $_GET in CLI.
The class-level comment (lines 21–23) states that @runInSeparateProcess is required because filter_input() doesn't work with direct $_GET assignments. However, spawning a separate process still runs PHP in CLI mode with no HTTP GET input. filter_input(INPUT_GET, …) returns null in that process too, which is why testFilterOrderWithValidColumn, testFilterOrderWithValidParam, and testFilterOrderWithCommaSeparatedValues all fail despite the annotation. The fix is in the production code (see comment above), not in process isolation.
🧰 Tools
🪛 GitHub Actions: phpunit
[error] 121-121: Filter order with valid column failed: Actual result did not match expected value.
[error] 136-136: Filter order with valid param failed: Actual result did not match expected value.
[error] 176-176: Filter order with comma separated values failed: Expected 'title, id, 999' but got different result.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/HelpsTest.php` around lines 113 - 177, The tests fail because
filter_order uses filter_input(INPUT_GET, ...) which returns null in CLI even
with `@runInSeparateProcess`; update the filter_order implementation to fall back
to reading from $_GET when filter_input returns null (e.g., use the filter_input
result if non-null, otherwise use $_GET[$key] or null) and ensure the same
validation/sanitization logic (allowed columns/params and comma-splitting) is
applied to the fallback value; modify the function named filter_order to
implement this fallback so tests that set $_GET directly behave correctly.
|
|
||
| // 'count(*)' is not in select_valids and alias expansion doesn't help | ||
| // because the expanded form also fails validation | ||
| $_GET['select'] = 'count(*)'; | ||
| $result = get_select($endpoint_params, $endpoint_columns); | ||
| $this->assertSame('*', $result); | ||
|
|
||
| // 'year' is not in select_valids, falls back to '*' | ||
| $_GET['select'] = 'year'; | ||
| $result = get_select($endpoint_params, $endpoint_columns); | ||
| $this->assertSame('*', $result); |
There was a problem hiding this comment.
Inaccurate comment — the expanded form count(*) as count IS valid; the real issue is validation uses the pre-alias value.
Line 115: "because the expanded form also fails validation" is incorrect. 'count(*) as count' is present in $select_valids. The actual reason the result is '*' is that validation (the if (!in_array(...)) block in get_select) operates on $select_lower — the pre-alias-expansion lowercase string ('count(*)') — while only $SELECT was updated by the alias lookup. 'count(*)' is not in $select_valids, so the guard fires and resets $SELECT to '*'. The same applies to the 'year' case below. The test assertions are correct, but the comment should reflect the true cause:
- // 'count(*)' is not in select_valids and alias expansion doesn't help
- // because the expanded form also fails validation
+ // 'count(*)' is not in select_valids; the alias expands $SELECT to
+ // 'count(*) as count', but validation still checks the pre-expansion
+ // $select_lower ('count(*)'), which is not in $select_valids → resets to '*'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 'count(*)' is not in select_valids and alias expansion doesn't help | |
| // because the expanded form also fails validation | |
| $_GET['select'] = 'count(*)'; | |
| $result = get_select($endpoint_params, $endpoint_columns); | |
| $this->assertSame('*', $result); | |
| // 'year' is not in select_valids, falls back to '*' | |
| $_GET['select'] = 'year'; | |
| $result = get_select($endpoint_params, $endpoint_columns); | |
| $this->assertSame('*', $result); | |
| // 'count(*)' is not in select_valids; the alias expands $SELECT to | |
| // 'count(*) as count', but validation still checks the pre-expansion | |
| // $select_lower ('count(*)'), which is not in $select_valids → resets to '*' | |
| $_GET['select'] = 'count(*)'; | |
| $result = get_select($endpoint_params, $endpoint_columns); | |
| $this->assertSame('*', $result); | |
| // 'year' is not in select_valids, falls back to '*' | |
| $_GET['select'] = 'year'; | |
| $result = get_select($endpoint_params, $endpoint_columns); | |
| $this->assertSame('*', $result); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/SelectHelpsTest.php` around lines 113 - 123, The comment is wrong:
get_select performs validation against the pre-alias lowercase string
($select_lower) while alias expansion updates $SELECT, so valid expanded forms
like 'count(*) as count' in $select_valids are rejected; update get_select so
the validation (the if (!in_array(...)) guard) checks the post-expansion value
(i.e., validate $SELECT or the expanded/aliased form) or perform alias expansion
before running the in_array check against $select_valids, referencing
get_select, $select_lower, $SELECT and $select_valids to locate and adjust the
logic accordingly.
1 participant
Summary by CodeRabbit
Bug Fixes
Tests
Chores