feat(#3541756): set default CORS allowedHeaders. #21
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a default CORS allowed header of '*' when enabling CORS in DruxtServiceProvider::alter if none are set, and updates the functional test to assert this default. Changes
Sequence Diagram(s)sequenceDiagram
participant SP as DruxtServiceProvider::alter
participant Cfg as cors.config
SP->>Cfg: Read enabled, allowedHeaders
alt CORS disabled
SP->>Cfg: If allowedHeaders empty, set ['*']
SP->>Cfg: Set enabled = true
SP->>Cfg: Save updated config
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/DruxtServiceProvider.php (1)
22-26: Use empty() and initialize the array to avoid notices; assign instead of append.
- count($cors_config['allowedHeaders']) can emit a notice if the key is unset/non-array.
- Prefer initializing the array when empty/undefined and assigning ['*'] rather than appending. This is both clearer and safer.
Apply this diff:
- // Set allowed headers to '*' by default. - if (count($cors_config['allowedHeaders']) === 0) { - $cors_config['allowedHeaders'][] = '*'; - } + // Set allowed headers to '*' by default when empty/undefined. + if (empty($cors_config['allowedHeaders'])) { + $cors_config['allowedHeaders'] = ['*']; + }Please confirm that in all supported environments, cors.config['allowedHeaders'] may be absent or empty (e.g., across Drupal core versions you target), so this guard is appropriate and won’t mask unexpected shapes.
tests/src/Functional/CorsIntegrationTest.php (1)
30-30: Make the assertion robust: assert presence rather than array index.Index-based assertion is brittle. Prefer asserting that '*' is present in the list. Also keep expected argument first when using equals.
Apply this diff:
- $this->assertEquals($cors_config['allowedHeaders'][0], '*'); + $this->assertContains('*', $cors_config['allowedHeaders']);If you intentionally require '*' to be the first element, clarify that contract and ensure the service provider enforces ordering accordingly. Otherwise, the presence check is safer across core defaults.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/DruxtServiceProvider.php(1 hunks)tests/src/Functional/CorsIntegrationTest.php(1 hunks)
🔇 Additional comments (1)
src/DruxtServiceProvider.php (1)
19-21: Comment clarifies intent.The inline comment complements the class-level docblock and makes the enablement step self-explanatory.
Summary by CodeRabbit