Conversation
implode(): Argument #2 ($array) must be of type ?array, string given
📝 WalkthroughWalkthroughA bug fix addressing how OAuth2 scopes are read from configuration (changed from array expectation to string with empty string default) to prevent TypeError crashes. A regression test covering various scope input types is added, and the CI workflow is extended to run the new test suite. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-521/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/Services/Apis/AbstractOAuth2Api.php (1)
135-138: Consider normalizing scopes to handle all input types.The fix correctly addresses the
implode()TypeError when$scopesis a string. However, sinceConfig::get()can return various types (string, array, or null), consider normalizing$scopesto always be a string for robust handling:
- If
$scopesis an array, the currentsprintfwill log"Array"instead of the actual values- Some subclasses (e.g.,
PaymentsApi) may have array-valued scope configurations♻️ Optional: Normalize scopes to string
- $scopes = $appConfig['scopes'] ?? ''; - Log::debug(sprintf( "AbstractOAuth2Api::getAccessToken - got scopes %s", $scopes)); + $scopes = $appConfig['scopes'] ?? ''; + if (is_array($scopes)) { + $scopes = implode(' ', $scopes); + } + Log::debug(sprintf( "AbstractOAuth2Api::getAccessToken - got scopes %s", $scopes));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/Apis/AbstractOAuth2Api.php` around lines 135 - 138, Normalize the $scopes value before logging and passing to the client: in AbstractOAuth2Api::getAccessToken ensure $scopes is converted to a string (treat null as '', if it's an array implode with a space or the expected separator, otherwise cast to string) so the Log::debug prints actual scope values and the call to $client->getAccessToken('client_credentials', ['scope' => $scopes]) always receives a string; update the normalization where $scopes is obtained (e.g., from Config::get or $appConfig['scopes']) and use the normalized variable in both the debug message and the getAccessToken call.tests/Unit/Services/AbstractOAuth2ApiScopesTest.php (2)
76-84: Consider adding array scopes to the data provider.The data provider covers string and null cases but doesn't test array-valued scopes, which
Config::get()can return. Adding this case would verify the fix handles all possible configuration types:♻️ Add array scopes test case
public static function scopesProvider(): array { return [ 'string scopes (space-separated)' => ['scope1 scope2', 'space-separated string scopes'], 'single string scope' => ['payment-profile/read', 'single string scope'], 'null scopes' => [null, 'null scopes'], 'empty string scopes' => ['', 'empty string scopes'], + 'array scopes' => [['scope1', 'scope2'], 'array scopes'], ]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Services/AbstractOAuth2ApiScopesTest.php` around lines 76 - 84, Add an array-valued scopes case to the test data provider so the code handles Config::get() returning arrays: update scopesProvider() in AbstractOAuth2ApiScopesTest to include an entry like 'array scopes' => [['scope1','scope2'], 'array scopes'] (or similar) so the test suite verifies array inputs alongside existing string, null and empty-string cases.
61-73: Redundant assertion in Exception catch block.In PHP 7+,
TypeErrorextendsError, notException. Thecatch (\Exception $e)block will never catch aTypeError, makingassertNotInstanceOf(\TypeError::class, $e)always pass. Consider simplifying:♻️ Simplified exception handling
try { $reflection->invoke($api); } catch (\TypeError $e) { $this->fail("TypeError thrown with {$description}: " . $e->getMessage()); } catch (\Exception $e) { // Connection/HTTP errors are expected since IDP is not reachable. // The critical assertion is that no TypeError was thrown from implode(). - $this->assertNotInstanceOf( - \TypeError::class, - $e, - "No TypeError should occur with {$description}" - ); + $this->assertTrue(true, "Expected non-TypeError exception with {$description}"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Services/AbstractOAuth2ApiScopesTest.php` around lines 61 - 73, The catch(\Exception $e) block contains a redundant assertNotInstanceOf(\TypeError::class, $e) because TypeError does not extend Exception; remove that assertion and its message and keep the block only to swallow/handle expected non-TypeError exceptions after invoking the reflected method (reflection->invoke($api)) in AbstractOAuth2ApiScopesTest, or alternatively change the catch to catch(\Throwable $e) if you want to explicitly assert that the thrown Throwable is not a TypeError; pick one approach and update the catch accordingly.
🤖 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/Unit/Services/AbstractOAuth2ApiScopesTest.php`:
- Line 23: Update the placeholder `@see` annotation in the docblock of
AbstractOAuth2ApiScopesTest
(tests/Unit/Services/AbstractOAuth2ApiScopesTest.php) so it points to the real
issue or PR instead of "XXX" (for example replace
"https://github.com/OpenStackweb/summit-api/issues/XXX" with the actual issue/PR
URL or ClickUp task link); edit the `@see` line in the class docblock to include
the correct identifier or remove the tag if no reference is needed.
---
Nitpick comments:
In `@app/Services/Apis/AbstractOAuth2Api.php`:
- Around line 135-138: Normalize the $scopes value before logging and passing to
the client: in AbstractOAuth2Api::getAccessToken ensure $scopes is converted to
a string (treat null as '', if it's an array implode with a space or the
expected separator, otherwise cast to string) so the Log::debug prints actual
scope values and the call to $client->getAccessToken('client_credentials',
['scope' => $scopes]) always receives a string; update the normalization where
$scopes is obtained (e.g., from Config::get or $appConfig['scopes']) and use the
normalized variable in both the debug message and the getAccessToken call.
In `@tests/Unit/Services/AbstractOAuth2ApiScopesTest.php`:
- Around line 76-84: Add an array-valued scopes case to the test data provider
so the code handles Config::get() returning arrays: update scopesProvider() in
AbstractOAuth2ApiScopesTest to include an entry like 'array scopes' =>
[['scope1','scope2'], 'array scopes'] (or similar) so the test suite verifies
array inputs alongside existing string, null and empty-string cases.
- Around line 61-73: The catch(\Exception $e) block contains a redundant
assertNotInstanceOf(\TypeError::class, $e) because TypeError does not extend
Exception; remove that assertion and its message and keep the block only to
swallow/handle expected non-TypeError exceptions after invoking the reflected
method (reflection->invoke($api)) in AbstractOAuth2ApiScopesTest, or
alternatively change the catch to catch(\Throwable $e) if you want to explicitly
assert that the thrown Throwable is not a TypeError; pick one approach and
update the catch accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d55042c0-fb4d-422a-b9eb-f1337e614d31
📒 Files selected for processing (3)
.github/workflows/push.ymlapp/Services/Apis/AbstractOAuth2Api.phptests/Unit/Services/AbstractOAuth2ApiScopesTest.php
| /** | ||
| * Class AbstractOAuth2ApiScopesTest | ||
| * Regression test for implode() TypeError when scopes config is a string. | ||
| * @see https://github.com/OpenStackweb/summit-api/issues/XXX |
There was a problem hiding this comment.
Update placeholder issue reference.
The @see annotation contains a placeholder XXX that should be updated to reference the actual issue or PR number (e.g., PR #521 or the linked ClickUp task).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Unit/Services/AbstractOAuth2ApiScopesTest.php` at line 23, Update the
placeholder `@see` annotation in the docblock of AbstractOAuth2ApiScopesTest
(tests/Unit/Services/AbstractOAuth2ApiScopesTest.php) so it points to the real
issue or PR instead of "XXX" (for example replace
"https://github.com/OpenStackweb/summit-api/issues/XXX" with the actual issue/PR
URL or ClickUp task link); edit the `@see` line in the class docblock to include
the correct identifier or remove the tag if no reference is needed.
There was a problem hiding this comment.
Pull request overview
Fixes a regression in OAuth2 client-credentials token acquisition when the configured scopes value is a string (from env/config), which previously caused an implode() TypeError, and adds coverage to prevent reintroducing it.
Changes:
- Update
AbstractOAuth2Api::getAccessToken()to avoidimplode()on the scopes value and pass/log scopes as a string. - Add a regression unit test covering string/null/empty scopes values.
- Update CI workflow matrix to include the new Services unit test directory.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
app/Services/Apis/AbstractOAuth2Api.php |
Prevents implode() TypeError by treating scopes as a string and adjusting logging. |
tests/Unit/Services/AbstractOAuth2ApiScopesTest.php |
Adds regression coverage for various scopes config value types. |
.github/workflows/push.yml |
Attempts to run the new Services unit tests in CI via the test matrix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - { name: "Repositories", filter: "--filter tests/Repositories/" } | ||
| - { name: "Services", filter: "--filter tests/Unit/Services/" } |
There was a problem hiding this comment.
The workflow runs PHPUnit as vendor/bin/phpunit ${{ matrix.suite.filter }}; passing a directory should be done as a positional path (like tests/Unit/Entities/), not via --filter (which filters by test name regex). With --filter tests/Unit/Services/ this suite may execute zero tests. Suggest changing the entry to filter: "tests/Unit/Services/" (or use an actual name regex if you intended --filter).
| - { name: "Repositories", filter: "--filter tests/Repositories/" } | |
| - { name: "Services", filter: "--filter tests/Unit/Services/" } | |
| - { name: "Repositories", filter: "tests/Repositories/" } | |
| - { name: "Services", filter: "tests/Unit/Services/" } |
| @@ -0,0 +1,85 @@ | |||
| <?php namespace Tests\Unit\Services; | |||
There was a problem hiding this comment.
File header doesn't follow the existing test-file convention in this repo: other tests use <?php on its own line, a blank line, then namespace ...;. Keeping it consistent improves readability and tooling support (linters/formatters).
| <?php namespace Tests\Unit\Services; | |
| <?php | |
| namespace Tests\Unit\Services; |
| /** | ||
| * Class AbstractOAuth2ApiScopesTest | ||
| * Regression test for implode() TypeError when scopes config is a string. | ||
| * @see https://github.com/OpenStackweb/summit-api/issues/XXX |
There was a problem hiding this comment.
The @see link still contains a placeholder (issues/XXX). Please replace it with the real issue/ClickUp link or remove it to avoid leaving dead references in the codebase.
| * @see https://github.com/OpenStackweb/summit-api/issues/XXX |
| $api = new MailApi($cacheService); | ||
|
|
||
| $reflection = new \ReflectionMethod($api, 'getAccessToken'); | ||
| $reflection->setAccessible(true); | ||
|
|
||
| try { | ||
| $reflection->invoke($api); | ||
| } catch (\TypeError $e) { |
There was a problem hiding this comment.
This unit test currently triggers a real OAuth2 token request (it relies on a timeout/connection failure). That can make CI slow or flaky depending on DNS/network policies. Consider mocking the HTTP layer (e.g., a Guzzle MockHandler) or injecting a stubbed OAuth2 provider so the test deterministically exercises the scopes handling without any outbound network call.
ref: https://app.clickup.com/t/86b924d72
Summary by CodeRabbit
Bug Fixes
Tests