Skip to content

fix: validate types of excluded words and word list in PassphraseServ…#41

Merged
nicobleiler merged 5 commits into
alphafrom
dev
Apr 30, 2026
Merged

fix: validate types of excluded words and word list in PassphraseServ…#41
nicobleiler merged 5 commits into
alphafrom
dev

Conversation

@nicobleiler

@nicobleiler nicobleiler commented Apr 30, 2026

Copy link
Copy Markdown
Owner

…iceProvider

Summary by CodeRabbit

  • New Features

    • Added runtime validation for configuration arrays to ensure proper data types.
    • Improved IDE support and type-hinting for facade classes.
  • Chores

    • Updated static analysis configuration to include comprehensive type validation across the codebase.

…iceProvider

Co-authored-by: Copilot <copilot@github.com>
@coderabbitai

coderabbitai Bot commented Apr 30, 2026

Copy link
Copy Markdown

Warning

Rate limit exceeded

@nicobleiler has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 47 minutes and 49 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 46106ffd-4b2f-4dfa-9953-5de3d91fd19a

📥 Commits

Reviewing files that changed from the base of the PR and between d69e532 and a3275d1.

📒 Files selected for processing (1)
  • tests/LaravelIntegrationTest.php

Walkthrough

The PR introduces configuration-level type validation for word lists in the service provider, adds PHPDoc mixin annotation to the facade, updates PHPStan configuration to analyze both source and test code, and hardens tests with null-safe operators and explicit error handling.

Changes

Cohort / File(s) Summary
Configuration & Analysis
phpstan.dist.neon
Updated PHPStan configuration to analyze both src and tests directories; enabled benevolent union type checking.
Type Documentation
src/Facades/Passphrase.php
Added PHPDoc @mixin PassphraseGenerator annotation to improve IDE type recognition and autocomplete.
Service Provider Validation
src/PassphraseServiceProvider.php
Added element-level type validation for excluded_words and word_list configuration arrays; throws WordListException when non-string elements are encountered.
Test Robustness
tests/LaravelIntegrationTest.php
Refactored to use null-safe operators (?->) for nullable app container; updated service provider registration to use container's register() method; added explicit failure check for tempnam() return value.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • Release: Promote dev to alpha #30: Modifies the same test file and addresses excluded_words/word_list configuration validation behavior.
  • Dev #13: Introduces WordListException variants and element-level validation in word lists that this PR now uses in the service provider.
  • Dev #14: Previously modified PassphraseServiceProvider to validate and configure array-based word lists with exception handling.

Suggested labels

released on @alpha``

Poem

🐰 A rabbit audits word-by-word,
With null-safe hops, validation stirred,
Types aligned and exceptions caught,
PHPStan's blessing now is sought! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.88% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: validating types of excluded words and word list in PassphraseServiceProvider, which is the core focus across multiple files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch dev

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.

❤️ Share
Review rate limit: 0/1 reviews remaining, refill in 47 minutes and 49 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

nicobleiler and others added 3 commits May 1, 2026 01:32
…tan configuration

Co-authored-by: Copilot <copilot@github.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/LaravelIntegrationTest.php (1)

126-149: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add tests for non-string elements in word_list and excluded_words.

The new runtime behavior in PassphraseServiceProvider validates array element types, but this file currently only tests non-array config values. Please add assertions for mixed arrays (e.g., ['ok', 123]) expecting WordListException::invalidType() and WordListException::invalidExcludedWordsType().

Proposed test additions
+    public function test_non_string_word_list_item_throws_clear_message(): void
+    {
+        config(['passphrase.word_list' => ['correct', 123, 'staple']]);
+        $this->refreshServiceProvider();
+        $this->expectExceptionObject(\NicoBleiler\Passphrase\Exceptions\WordListException::invalidType());
+        $this->app?->make(WordList::class);
+    }
+
+    public function test_non_string_excluded_words_item_throws_clear_message(): void
+    {
+        config([
+            'passphrase.word_list' => ['correct', 'horse', 'battery', 'staple'],
+            'passphrase.excluded_words' => ['horse', 999],
+        ]);
+        $this->refreshServiceProvider();
+        $this->expectExceptionObject(\NicoBleiler\Passphrase\Exceptions\WordListException::invalidExcludedWordsType());
+        $this->app?->make(WordList::class);
+    }

As per coding guidelines: tests/**/*.php: Add/adjust tests in tests/ for any behavioral change.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/LaravelIntegrationTest.php` around lines 126 - 149, Add two tests in
tests/LaravelIntegrationTest.php after the existing non-array tests that verify
mixed-type arrays throw the new WordList exceptions: create
test_word_list_with_non_string_element_throws which sets
config(['passphrase.word_list' => ['ok', 123]]) then calls
$this->refreshServiceProvider(), asserts
$this->expectExceptionObject(WordListException::invalidType()), and invokes
$this->app?->make(WordList::class); and create
test_excluded_words_with_non_string_element_throws which sets
config(['passphrase.word_list' => ['ok','horse'], 'passphrase.excluded_words' =>
['ok', 123]]), calls $this->refreshServiceProvider(), asserts
$this->expectExceptionObject(WordListException::invalidExcludedWordsType()), and
invokes $this->app?->make(WordList::class); ensure names reference
WordListException and the WordList binding so failures map to
PassphraseServiceProvider validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@tests/LaravelIntegrationTest.php`:
- Around line 126-149: Add two tests in tests/LaravelIntegrationTest.php after
the existing non-array tests that verify mixed-type arrays throw the new
WordList exceptions: create test_word_list_with_non_string_element_throws which
sets config(['passphrase.word_list' => ['ok', 123]]) then calls
$this->refreshServiceProvider(), asserts
$this->expectExceptionObject(WordListException::invalidType()), and invokes
$this->app?->make(WordList::class); and create
test_excluded_words_with_non_string_element_throws which sets
config(['passphrase.word_list' => ['ok','horse'], 'passphrase.excluded_words' =>
['ok', 123]]), calls $this->refreshServiceProvider(), asserts
$this->expectExceptionObject(WordListException::invalidExcludedWordsType()), and
invokes $this->app?->make(WordList::class); ensure names reference
WordListException and the WordList binding so failures map to
PassphraseServiceProvider validation.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3b26f1c3-7af8-4b8e-8461-f4ba05187bf0

📥 Commits

Reviewing files that changed from the base of the PR and between 9811c8a and d69e532.

📒 Files selected for processing (4)
  • phpstan.dist.neon
  • src/Facades/Passphrase.php
  • src/PassphraseServiceProvider.php
  • tests/LaravelIntegrationTest.php

…uded words

Co-authored-by: Copilot <copilot@github.com>
@nicobleiler nicobleiler merged commit 856c3ad into alpha Apr 30, 2026
24 checks passed
@github-actions

Copy link
Copy Markdown

🎉 This PR is included in version 2.2.0-alpha.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@github-actions

github-actions Bot commented May 1, 2026

Copy link
Copy Markdown

🎉 This PR is included in version 2.2.0-beta.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant