-
-
Notifications
You must be signed in to change notification settings - Fork 17
Modernize PHP codebase with strict typing #22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add return type declarations to all methods - Add parameter type declarations to all methods - Add property type declarations (typed properties) - Use union types where appropriate (array|string) - Use static return type for fluent interfaces - Fix inconsistent Arr import (use Illuminate\Support\Arr) - Remove unused Generator import - Remove redundant PHPDoc blocks (covered by type declarations) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Change trigger from webpack.mix.js to vite.config.js - Change outputs from mix-manifest.json to build/manifest.json - Change output paths from public/css, public/js to public/build/assets Laravel 10+ uses Vite by default instead of Mix. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request systematically adds strict type hints and return types throughout the codebase. It also updates GitHub workflows to remove PHP 8.1 from test matrix, updates build configuration from webpack/mix to Vite, and documents these changes in the CHANGELOG. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Drivers/FilesystemDriver.php (1)
172-176: Fix potential null parameter to Str::finish().The
Arr::get()method can returnnullif the config key doesn't exist. PassingnulltoStr::finish()(which expects a string parameter) will cause aTypeErrorin PHP 8+ with strict types enabled.Apply this diff to provide a safe default:
protected function localStashPath(): string { $tmp = Arr::get($this->config, 'local_tmp_directory'); - return Str::finish($tmp, '/'); + return Str::finish($tmp ?? '', '/'); }
🧹 Nitpick comments (2)
src/FileSelection.php (1)
18-18: Consider importing Collection instead of using fully qualified names.Using fully qualified class names in type hints (e.g.,
\Illuminate\Support\Collection) works but is stylistically unusual. The more common pattern is to import the class at the top and use the short name in type hints.Apply this diff to use imports:
+use Illuminate\Support\Collection; + namespace AaronFrancis\Airdrop; use Illuminate\Support\Str;Then update the type hints:
- protected \Illuminate\Support\Collection $includeFilesAndDirectories; + protected Collection $includeFilesAndDirectories; - protected \Illuminate\Support\Collection $excludeFilesAndDirectories; + protected Collection $excludeFilesAndDirectories; - public function selected(): \Illuminate\Support\Collection + public function selected(): Collection - protected function sanitize(array|string $paths): \Illuminate\Support\Collection + protected function sanitize(array|string $paths): CollectionAlso applies to: 20-20, 58-58, 137-137
src/Drivers/FilesystemDriver.php (1)
111-111: Consider importing types instead of using fully qualified names.Similar to the comment in
FileSelection.php, using fully qualified class names in type hints is stylistically unusual. Consider importingCollectionandFilesystemat the top of the file and using short names in type hints.Also applies to: 122-122
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.github/workflows/style.yml(2 hunks).github/workflows/tests.yml(1 hunks)CHANGELOG.md(1 hunks)config/airdrop.php(2 hunks)src/AirdropServiceProvider.php(2 hunks)src/Commands/Debug.php(1 hunks)src/Commands/Download.php(1 hunks)src/Commands/Hash.php(1 hunks)src/Commands/Install.php(1 hunks)src/Commands/Upload.php(1 hunks)src/Concerns/MakesDrivers.php(2 hunks)src/Contracts/TriggerContract.php(1 hunks)src/Drivers/BaseDriver.php(1 hunks)src/Drivers/FilesystemDriver.php(8 hunks)src/Drivers/GithubActionsDriver.php(1 hunks)src/FileSelection.php(4 hunks)src/HashGenerator.php(3 hunks)src/Triggers/ConfigTrigger.php(1 hunks)src/Triggers/FileTrigger.php(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Use Laravel Pint with the Laravel preset for code formatting, with customizations: one space around concatenation operator and no trailing commas in multiline constructs
Files:
src/Commands/Download.phpsrc/Concerns/MakesDrivers.phpsrc/Commands/Debug.phpsrc/Contracts/TriggerContract.phpsrc/Drivers/GithubActionsDriver.phpsrc/Commands/Upload.phpsrc/Commands/Hash.phpsrc/Commands/Install.phpsrc/Drivers/BaseDriver.phpsrc/Triggers/ConfigTrigger.phpsrc/AirdropServiceProvider.phpsrc/HashGenerator.phpsrc/Triggers/FileTrigger.phpsrc/FileSelection.phpsrc/Drivers/FilesystemDriver.phpconfig/airdrop.php
src/Drivers/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Drivers should extend BaseDriver and implement download() and upload() methods for handling storage and retrieval of built assets
Files:
src/Drivers/GithubActionsDriver.phpsrc/Drivers/BaseDriver.phpsrc/Drivers/FilesystemDriver.php
src/Triggers/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Triggers must implement TriggerContract::triggerBuildWhenChanged($config) to determine what affects the build hash
Files:
src/Triggers/ConfigTrigger.phpsrc/Triggers/FileTrigger.php
🧠 Learnings (3)
📚 Learning: 2025-11-27T04:21:20.980Z
Learnt from: CR
Repo: aarondfrancis/airdrop PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T04:21:20.980Z
Learning: Applies to src/Drivers/*.php : Drivers should extend BaseDriver and implement download() and upload() methods for handling storage and retrieval of built assets
Applied to files:
src/Concerns/MakesDrivers.phpsrc/Drivers/GithubActionsDriver.phpsrc/Drivers/BaseDriver.phpsrc/Drivers/FilesystemDriver.php
📚 Learning: 2025-11-27T04:21:20.980Z
Learnt from: CR
Repo: aarondfrancis/airdrop PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T04:21:20.980Z
Learning: Commands should be organized in the Commands directory with specific functionality: airdrop:download for pulling cached assets, airdrop:upload for storing assets, airdrop:hash for output, airdrop:debug for debugging, and airdrop:install for publishing config
Applied to files:
src/Commands/Debug.php
📚 Learning: 2025-11-27T04:21:20.980Z
Learnt from: CR
Repo: aarondfrancis/airdrop PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T04:21:20.980Z
Learning: Applies to src/Triggers/*.php : Triggers must implement TriggerContract::triggerBuildWhenChanged($config) to determine what affects the build hash
Applied to files:
src/Contracts/TriggerContract.phpsrc/Triggers/ConfigTrigger.phpsrc/HashGenerator.phpsrc/Triggers/FileTrigger.php
🧬 Code graph analysis (11)
src/Commands/Download.php (4)
src/Commands/Debug.php (1)
handle(19-31)src/Commands/Hash.php (1)
handle(18-31)src/Commands/Install.php (1)
handle(28-35)src/Commands/Upload.php (1)
handle(16-23)
src/Concerns/MakesDrivers.php (1)
src/Drivers/BaseDriver.php (1)
BaseDriver(9-39)
src/Commands/Debug.php (4)
src/Commands/Download.php (1)
handle(20-27)src/Commands/Hash.php (1)
handle(18-31)src/Commands/Install.php (1)
handle(28-35)src/Commands/Upload.php (1)
handle(16-23)
src/Contracts/TriggerContract.php (2)
src/Triggers/ConfigTrigger.php (1)
triggerBuildWhenChanged(13-16)src/Triggers/FileTrigger.php (1)
triggerBuildWhenChanged(16-37)
src/Drivers/GithubActionsDriver.php (1)
src/Drivers/FilesystemDriver.php (5)
exists(127-130)localStashPath(172-177)stashedPackageFilename(137-140)downloadFromRemoteStorage(142-149)uploadToRemoteStorage(151-163)
src/Commands/Upload.php (4)
src/Commands/Debug.php (1)
handle(19-31)src/Commands/Download.php (1)
handle(20-27)src/Commands/Hash.php (1)
handle(18-31)src/Commands/Install.php (1)
handle(28-35)
src/Commands/Hash.php (4)
src/Commands/Debug.php (1)
handle(19-31)src/Commands/Download.php (1)
handle(20-27)src/Commands/Install.php (1)
handle(28-35)src/Commands/Upload.php (1)
handle(16-23)
src/Commands/Install.php (4)
src/Commands/Debug.php (1)
handle(19-31)src/Commands/Download.php (1)
handle(20-27)src/Commands/Hash.php (1)
handle(18-31)src/Commands/Upload.php (1)
handle(16-23)
src/Drivers/BaseDriver.php (1)
src/Drivers/FilesystemDriver.php (2)
download(41-55)upload(19-39)
src/Triggers/FileTrigger.php (4)
src/Contracts/TriggerContract.php (1)
triggerBuildWhenChanged(15-15)src/Triggers/ConfigTrigger.php (1)
triggerBuildWhenChanged(13-16)src/Drivers/FilesystemDriver.php (1)
files(111-120)src/FileSelection.php (1)
excludeNames(37-42)
src/Drivers/FilesystemDriver.php (2)
src/Drivers/BaseDriver.php (2)
upload(38-38)download(36-36)src/Drivers/GithubActionsDriver.php (3)
exists(11-14)downloadFromRemoteStorage(16-21)uploadToRemoteStorage(23-28)
🪛 PHPMD (2.15.0)
src/Drivers/GithubActionsDriver.php
16-16: Avoid unused parameters such as '$zipPath'. (undefined)
(UnusedFormalParameter)
23-23: Avoid unused parameters such as '$zipPath'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (26)
src/Commands/Debug.php (1)
11-11: Arr import + void return type align with the rest of the commands.Using
Illuminate\Support\Arris the correct concrete import here, and declaringhandle(): voidmatches the other command classes without changing behavior. Based on learnings, this keeps the dedicatedairdrop:debugcommand consistent with the rest of the Commands suite.Also applies to: 19-19
.github/workflows/style.yml (1)
3-7: Style workflow now runs only onmain/*.xwith PHP 8.4.This narrows the trigger from “any push” to just
mainand*.xbranches and fixes Pint to PHP 8.4. If you still want auto-formatting on feature branches or across multiple PHP versions, you may want to keep a broader trigger or a matrix; otherwise this looks aligned with a main/release-only strategy.Also applies to: 25-25
src/Commands/Hash.php (1)
18-18: ExplicitvoidonHash::handleis appropriate.The command only performs side effects and doesn’t rely on an exit code, so
public function handle(): voidis accurate and keeps the signature consistent with the other command classes.src/Commands/Download.php (1)
20-20:Download::handlevoid return matches usage.The method performs config tweaks and driver side effects only; declaring
handle(): voidmakes that explicit and keeps it in line with the other commands.src/Commands/Upload.php (1)
16-16:Upload::handlevoid return matches usage.This command is purely side-effectful (config + driver upload), so
handle(): voidis the right explicit type and keeps the commands consistent.config/airdrop.php (1)
76-79: Vite-based triggers and outputs look correct for the default Laravel Vite layout.Including
vite.config.jsinFileTriggerand watchingpublic_path('build/manifest.json')pluspublic_path('build/assets')should preserve the “rebuild when build config or artifacts change” behavior from the old Mix setup for standard installs; consumers with custom Vite output dirs can override these paths in their published config.Also applies to: 123-128
CHANGELOG.md (1)
3-18: Changelog entry lines up with the CI, config, and typing changes.The Unreleased section accurately calls out the PHP 8.4 matrix update, Vite-based config changes, strict typing work, and PHPUnit/Testbench/CI cleanup introduced here.
src/Commands/Install.php (1)
28-35: LGTM! Consistent typing addition.The
voidreturn type correctly reflects that the command doesn't return a value, aligning with the other command classes in the codebase.src/Contracts/TriggerContract.php (1)
15-15: LGTM! Interface contract tightened appropriately.The explicit array types for both parameter and return value enforce stricter contracts while maintaining backward compatibility with the default parameter value. All implementations have been updated consistently.
Based on learnings, Triggers must implement TriggerContract::triggerBuildWhenChanged($config) to determine what affects the build hash.
src/AirdropServiceProvider.php (2)
18-33: LGTM! Standard Laravel service provider typing.The
voidreturn type onboot()aligns with Laravel'sServiceProviderbase class conventions.
35-38: LGTM! Standard Laravel service provider typing.The
voidreturn type onregister()aligns with Laravel'sServiceProviderbase class conventions.src/Triggers/ConfigTrigger.php (1)
13-16: LGTM! Correctly implements the typed interface.The implementation properly matches the
TriggerContractinterface with explicit array types for both parameter and return value.Based on learnings, Triggers must implement TriggerContract::triggerBuildWhenChanged($config) to determine what affects the build hash.
src/Drivers/GithubActionsDriver.php (2)
11-14: LGTM! Correct return type for existence check.The
boolreturn type accurately reflects the file existence check.Based on learnings, Drivers should extend BaseDriver and implement download() and upload() methods for handling storage and retrieval of built assets.
16-28: LGTM! Intentional no-op overrides with correct typing.The unused parameters flagged by static analysis are required by the parent class method signatures. These no-op implementations are intentional—GitHub Actions manages caching through workflow steps rather than explicit upload/download operations.
Based on learnings, Drivers should extend BaseDriver and implement download() and upload() methods for handling storage and retrieval of built assets.
src/Concerns/MakesDrivers.php (3)
16-26: LGTM! Correct return type for driver instantiation.The
BaseDriverreturn type properly enforces the driver contract after validation.Based on learnings, Drivers should extend BaseDriver and implement download() and upload() methods for handling storage and retrieval of built assets.
28-34: LGTM! Appropriate nullable return type.The
?arrayreturn type correctly handles the case where the configured driver may not exist in the configuration.
36-41: LGTM! Appropriate use of mixed type for validation.The
mixedparameter type is correct for a validation method that checks type at runtime, followed byvoidreturn since it throws on failure.src/HashGenerator.php (4)
14-32: LGTM! Property and constructor with correct typing.The typed
$triggersproperty and the static factory method withstaticreturn type follow modern PHP best practices.Based on learnings, Triggers must implement TriggerContract::triggerBuildWhenChanged($config) to determine what affects the build hash.
34-37: LGTM! Correct return type for hash generation.The
stringreturn type accurately represents the MD5 hash output.
39-55: LGTM! Correct return type for array representation.The
arrayreturn type properly reflects the structured trigger data output.
57-62: LGTM! Strict parameter typing for validation.The
string $classparameter withvoidreturn type is appropriate for a validation method that throws on failure.src/Triggers/FileTrigger.php (2)
16-37: LGTM! Correctly implements the typed interface.The implementation properly matches the
TriggerContractwith explicit array types and correctly processes file hashes.Based on learnings, Triggers must implement TriggerContract::triggerBuildWhenChanged($config) to determine what affects the build hash.
39-44: LGTM! Strict typing for helper method.The explicit array parameter types and
Collectionreturn type provide clear contracts and type safety.src/Drivers/BaseDriver.php (1)
11-38: LGTM! Strict typing properly implemented.The addition of type hints for properties, parameters, and return types is well-executed. The use of
staticreturn type for fluent methods andvoidfor abstract I/O methods aligns perfectly with the modernization goals and establishes a clear contract for driver implementations.src/FileSelection.php (1)
22-24: LGTM! Type declarations are correct and comprehensive.The typed properties, method parameters, return types, and union types are all properly implemented for PHP 8+ compatibility.
Also applies to: 26-51, 98-124
src/Drivers/FilesystemDriver.php (1)
19-41: LGTM! Method signatures properly implement strict typing.The upload, download, and other method signatures correctly implement the abstract contracts from
BaseDriverand add appropriate type hints throughout. The logic remains sound with the added type safety.Also applies to: 57-78, 127-163
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
Changed
Removed
✏️ Tip: You can customize this high-level summary in your review settings.