Modernize codebase with PHP 8.1+ features #166
Conversation
- Add declare(strict_types=1) to all PHP files - Use constructor property promotion in Event classes, Result classes, and BaseAction - Add native typed properties throughout (int, string, bool, array, class types) - Add explicit return types to methods - Fix Finder::shouldExclude() to pass pathname string instead of SplFileInfo - Fix SettledResult::parseLogs() to handle null LogResult - Remove constructor from ResultContract interface (anti-pattern) All 95 tests pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Convert Runtime abstract class to backed enum with string values - Convert Architecture abstract class to backed enum with string values - Add RuntimeConstants and ArchitectureConstants as deprecated aliases - Add runtimeValue() and architectureValue() methods for string resolution - Allow runtime() and architecture() to return enum or string (Runtime|string) - Add new runtimes: nodejs24.x, nodejs22.x, python3.14, python3.13, java25, dotnet9, ruby3.4 - Mark newly deprecated AWS runtimes: nodejs18.x, dotnet6 Usage examples: - Override runtime(): return Runtime::PYTHON_312; // enum - Override runtime(): return 'python3.12'; // string (legacy) - Get string value: $function->runtimeValue(); 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Tests cover enum values, backwards compatibility with constants classes, and the runtimeValue()/architectureValue() resolution methods. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The tests expected Architecture::X86_64 enum but the actual code passes the string value 'x86_64'. Updated test expectations to match. 🤖 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 WalkthroughComprehensive refactor converting Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
|
@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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/WarmingConfig.php (2)
24-31: Add type hint to prevent runtime TypeError.With
strict_types=1enabled, line 28 will throw a TypeError if$countis not an int, since$instancesis now strictly typed. The parameter must be type-hinted to match.Apply this diff:
- public static function instances($count) + public static function instances(int $count): static { $self = new static; $self->instances = $count; return $self; }
33-38: Add type hint to prevent runtime TypeError.With
strict_types=1enabled, line 35 will throw a TypeError if$payloadis not an array, since$this->payloadis now strictly typed. The parameter must be type-hinted to match.Apply this diff:
- public function withPayload($payload = []) + public function withPayload(array $payload = []): static { $this->payload = $payload; return $this; }
🧹 Nitpick comments (11)
src/Exceptions/NoFunctionsRegisteredException.php (1)
15-22: Consider removing the unused$messageparameter.The
$messageparameter is immediately overwritten on line 17, making it non-functional. While matching the standardExceptionsignature may be intentional, accepting a parameter that's never used creates a misleading API.Apply this diff to remove the unused parameter:
- public function __construct(string $message = '', int $code = 0, ?Throwable $previous = null) + public function __construct(int $code = 0, ?Throwable $previous = null) { $message = "No Sidecar functions have been configured. \n" . "Please check your config/sidecar.php file to ensure you have registered your functions. \n" . 'Read more at https://hammerstone.dev/sidecar/docs/main/configuration#registering-functions'; parent::__construct($message, $code, $previous); }Alternatively, if maintaining the signature for consistency is important, consider adding a comment explaining why the parameter exists but isn't used.
src/WarmingConfig.php (1)
19-22: Consider constructor property promotion for consistency.For consistency with other classes modernized in this PR (e.g.,
AfterFunctionsActivated,BeforeFunctionsDeployed), consider using constructor property promotion:- public int $instances = 0; - - public array $payload = [ + public array $payload = [ 'warming' => true ]; - public function __construct(int $instances = 0) - { - $this->instances = $instances; - } + public function __construct( + public int $instances = 0 + ) {}This reduces boilerplate and aligns with the modernization patterns seen in other event classes within this PR.
src/Commands/Warm.php (1)
3-4: Based on my thorough verification of the codebase, I can now provide the rewritten review comment:
Strict types declaration is safe and correctly positioned.
The addition of
declare(strict_types=1);is properly placed and introduces no type errors. Thehandle()method callsSidecar::addCommandLogger($this), which expects aCommandparameter—and sinceWarmextendsEnvironmentAwareCommand(which extendsIlluminate\Console\Command), the type requirement is satisfied.Optional: Consider adding a return type hint to the
handle()method for complete PHP 8.1+ modernization:- public function handle() + public function handle(): intNote: While the return type is not explicitly declared, the implicit return is valid for Laravel console commands.
src/Commands/Deploy.php (1)
35-36: Consider importing or fully qualifyingExceptionin the docblock.The
@throws Exceptionannotation references an unqualified class name that isn't imported. While this doesn't affect runtime, it could confuse IDEs and static analyzers.use Hammerstone\Sidecar\Deployment; use Hammerstone\Sidecar\Exceptions\NoFunctionsRegisteredException; use Hammerstone\Sidecar\Sidecar; +use Exception;src/Concerns/HandlesLogging.php (1)
3-18: Typed properties look good; consider tightening method signatures laterThe new
array/boolproperty types are consistent with usage and should be safe under existing callers. As a follow‑up (not required for this PR), you could gradually add signatures likepublic function addLogger(callable $closure): static,public function log(string $message): void,protected function write(string $message, string $level): voidto further clarify the contract, assuming no callers rely on non‑string messages.src/Commands/Configure.php (1)
3-32: Typed properties here align with actual usageThe new
?stringtypes for$key,$secret,$regionandintfor$widthmatch how these fields are used (AWS config and formatting) and should be safe under existing flows. If you want to go further later,text()/client()/action()could also grow explicit return types (e.g.void/ mixed), but that’s not necessary for this refactor.tests/Unit/Support/EnumRuntimeTestFunction.php (1)
16-24: Add return type declarations for consistency with PR goals.Given that this PR aims to modernize the codebase with PHP 8.1+ features and strict types, the
handler()andpackage()methods should include return type declarations to match the parent class signatures.- public function handler() + public function handler(): string { return 'index.handler'; } - public function package() + public function package(): array { return []; }src/Commands/Actions/DestroyAdminKeys.php (1)
29-33: Consider logging the exception before returning null.The exception is silently swallowed here. While returning
nullmay be intentional to indicate the user couldn't be retrieved, logging the exception would aid debugging without changing the control flow.try { $user = $client->getUser(); } catch (Throwable $e) { + // Optionally log the exception for debugging: + // report($e); return null; }tests/Unit/ArchitectureTest.php (1)
21-24: Consider usingassertSame()for stricter type comparisons.For string value assertions,
assertSame()provides stricter type checking thanassertEquals(), which is preferable when testing exact values.public function test_x86_64_has_correct_value() { - $this->assertEquals('x86_64', Architecture::X86_64->value); + $this->assertSame('x86_64', Architecture::X86_64->value); } public function test_arm64_has_correct_value() { - $this->assertEquals('arm64', Architecture::ARM_64->value); + $this->assertSame('arm64', Architecture::ARM_64->value); }Also applies to: 26-29
src/Results/PendingResult.php (1)
41-52: PHPDoc and parameter type are slightly inconsistent.The PHPDoc specifies
@param Request $requestbut the actual parameter type ismixed. Consider updating the PHPDoc to reflect the actual type, or keeping it as documentation of the expected type./** * Defer to the SettledResult. * - * @param Request $request + * @param mixed $request * @return Response * * @throws Exception */ public function toResponse(mixed $request)src/Results/SettledResult.php (1)
64-73: Docblock is inconsistent with the actual type hint.The docblock at line 65 specifies
@param Request $requestbut the method signature usesmixed $request. Consider updating the docblock to reflect the broadermixedtype that matches theResultContractinterface./** - * @param Request $request + * @param mixed $request * @return Response * * @throws Exception */ public function toResponse(mixed $request)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (53)
config/sidecar.php(1 hunks)src/Architecture.php(1 hunks)src/ArchitectureConstants.php(1 hunks)src/Clients/CloudWatchLogsClient.php(1 hunks)src/Clients/Configurations/AwsClientConfiguration.php(1 hunks)src/Clients/LambdaClient.php(1 hunks)src/Clients/S3Client.php(1 hunks)src/Commands/Actions/BaseAction.php(2 hunks)src/Commands/Actions/CreateBucket.php(1 hunks)src/Commands/Actions/CreateDeploymentUser.php(2 hunks)src/Commands/Actions/CreateExecutionRole.php(2 hunks)src/Commands/Actions/DestroyAdminKeys.php(4 hunks)src/Commands/Actions/DetermineRegion.php(2 hunks)src/Commands/Activate.php(1 hunks)src/Commands/Configure.php(2 hunks)src/Commands/Deploy.php(1 hunks)src/Commands/EnvironmentAwareCommand.php(2 hunks)src/Commands/Install.php(1 hunks)src/Commands/Warm.php(1 hunks)src/Concerns/HandlesLogging.php(2 hunks)src/Concerns/ManagesEnvironments.php(2 hunks)src/Contracts/AwsClientConfiguration.php(1 hunks)src/Deployment.php(3 hunks)src/Events/AfterFunctionExecuted.php(2 hunks)src/Events/AfterFunctionsActivated.php(2 hunks)src/Events/AfterFunctionsDeployed.php(2 hunks)src/Events/BeforeFunctionExecuted.php(2 hunks)src/Events/BeforeFunctionsActivated.php(2 hunks)src/Events/BeforeFunctionsDeployed.php(2 hunks)src/Exceptions/ConfigurationException.php(1 hunks)src/Exceptions/FunctionNotFoundException.php(2 hunks)src/Exceptions/LambdaExecutionException.php(1 hunks)src/Exceptions/NoFunctionsRegisteredException.php(2 hunks)src/Exceptions/SidecarException.php(1 hunks)src/Finder.php(3 hunks)src/LambdaFunction.php(4 hunks)src/Manager.php(2 hunks)src/Package.php(2 hunks)src/Providers/SidecarServiceProvider.php(3 hunks)src/Region.php(2 hunks)src/Results/PendingResult.php(4 hunks)src/Results/ResultContract.php(1 hunks)src/Results/SettledResult.php(5 hunks)src/Runtime.php(1 hunks)src/RuntimeConstants.php(1 hunks)src/Sidecar.php(1 hunks)src/WarmingConfig.php(2 hunks)tests/Unit/ArchitectureTest.php(1 hunks)tests/Unit/FunctionTest.php(2 hunks)tests/Unit/LambdaClientTest.php(4 hunks)tests/Unit/RuntimeTest.php(1 hunks)tests/Unit/Support/EnumRuntimeTestFunction.php(1 hunks)tests/Unit/Support/StringRuntimeTestFunction.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (25)
src/Clients/Configurations/AwsClientConfiguration.php (1)
src/Contracts/AwsClientConfiguration.php (1)
getConfiguration(13-13)
src/Contracts/AwsClientConfiguration.php (1)
src/Clients/Configurations/AwsClientConfiguration.php (2)
AwsClientConfiguration(9-29)getConfiguration(11-28)
src/Providers/SidecarServiceProvider.php (2)
src/Clients/Configurations/AwsClientConfiguration.php (1)
getConfiguration(11-28)src/Contracts/AwsClientConfiguration.php (1)
getConfiguration(13-13)
src/Commands/EnvironmentAwareCommand.php (1)
src/Concerns/ManagesEnvironments.php (1)
overrideEnvironment(15-18)
tests/Unit/Support/EnumRuntimeTestFunction.php (2)
src/LambdaFunction.php (1)
LambdaFunction(22-477)tests/Unit/Support/StringRuntimeTestFunction.php (1)
runtime(26-29)
src/Commands/Actions/CreateBucket.php (5)
src/Commands/Actions/BaseAction.php (2)
BaseAction(13-26)invoke(20-20)src/Package.php (1)
bucket(428-431)src/Commands/Actions/CreateDeploymentUser.php (1)
invoke(19-28)src/Commands/Actions/CreateExecutionRole.php (1)
invoke(19-30)src/Commands/Actions/DetermineRegion.php (1)
invoke(21-38)
src/WarmingConfig.php (8)
src/Commands/Actions/BaseAction.php (1)
__construct(15-18)src/Events/AfterFunctionsActivated.php (1)
__construct(13-15)src/Events/AfterFunctionsDeployed.php (1)
__construct(13-15)src/Events/BeforeFunctionExecuted.php (1)
__construct(15-18)src/Events/BeforeFunctionsActivated.php (1)
__construct(13-15)src/Events/BeforeFunctionsDeployed.php (1)
__construct(13-15)src/Results/PendingResult.php (1)
__construct(22-25)src/Results/SettledResult.php (1)
__construct(31-36)
tests/Unit/RuntimeTest.php (2)
src/RuntimeConstants.php (1)
RuntimeConstants(10-107)tests/Unit/Base.php (1)
Base(13-32)
src/Events/AfterFunctionsActivated.php (3)
src/Events/AfterFunctionsDeployed.php (1)
__construct(13-15)src/Events/BeforeFunctionsActivated.php (1)
__construct(13-15)src/Events/BeforeFunctionsDeployed.php (1)
__construct(13-15)
src/Events/BeforeFunctionsActivated.php (3)
src/Events/AfterFunctionsActivated.php (1)
__construct(13-15)src/Events/AfterFunctionsDeployed.php (1)
__construct(13-15)src/Events/BeforeFunctionsDeployed.php (1)
__construct(13-15)
tests/Unit/ArchitectureTest.php (1)
tests/Unit/Base.php (1)
Base(13-32)
src/Events/AfterFunctionsDeployed.php (3)
src/Events/AfterFunctionsActivated.php (1)
__construct(13-15)src/Events/BeforeFunctionsActivated.php (1)
__construct(13-15)src/Events/BeforeFunctionsDeployed.php (1)
__construct(13-15)
src/Commands/Actions/BaseAction.php (2)
src/Commands/Configure.php (1)
Configure(20-130)src/Commands/Actions/CreateDeploymentUser.php (1)
invoke(19-28)
src/Exceptions/FunctionNotFoundException.php (2)
src/Deployment.php (1)
make(31-34)src/Package.php (1)
make(49-52)
src/Concerns/ManagesEnvironments.php (1)
src/Commands/EnvironmentAwareCommand.php (1)
overrideEnvironment(27-32)
src/Events/BeforeFunctionExecuted.php (5)
src/Events/AfterFunctionsActivated.php (1)
__construct(13-15)src/Events/AfterFunctionsDeployed.php (1)
__construct(13-15)src/Events/BeforeFunctionsActivated.php (1)
__construct(13-15)src/Events/BeforeFunctionsDeployed.php (1)
__construct(13-15)src/LambdaFunction.php (1)
LambdaFunction(22-477)
src/Results/PendingResult.php (3)
src/Results/SettledResult.php (4)
SettledResult(23-296)settled(59-62)__construct(31-36)toResponse(70-73)src/Results/ResultContract.php (2)
settled(22-22)toResponse(20-20)src/LambdaFunction.php (2)
LambdaFunction(22-477)toResponse(189-194)
src/Commands/Actions/DetermineRegion.php (2)
src/Commands/Actions/BaseAction.php (2)
BaseAction(13-26)invoke(20-20)src/Commands/Actions/CreateBucket.php (1)
invoke(22-51)
src/Commands/Actions/CreateExecutionRole.php (3)
src/Commands/Configure.php (1)
client(68-80)src/Commands/Actions/BaseAction.php (1)
invoke(20-20)src/Commands/Actions/CreateDeploymentUser.php (1)
invoke(19-28)
src/Exceptions/NoFunctionsRegisteredException.php (1)
src/Deployment.php (1)
__construct(39-48)
src/Deployment.php (1)
src/LambdaFunction.php (3)
architectureValue(264-269)packageType(279-282)runtimeValue(244-249)
src/Events/BeforeFunctionsDeployed.php (3)
src/Events/AfterFunctionsActivated.php (1)
__construct(13-15)src/Events/AfterFunctionsDeployed.php (1)
__construct(13-15)src/Events/BeforeFunctionsActivated.php (1)
__construct(13-15)
src/Results/ResultContract.php (3)
src/LambdaFunction.php (1)
toResponse(189-194)src/Results/PendingResult.php (2)
toResponse(49-52)settled(27-34)src/Results/SettledResult.php (3)
toResponse(70-73)settled(59-62)SettledResult(23-296)
src/Results/SettledResult.php (2)
src/Results/PendingResult.php (3)
__construct(22-25)settled(27-34)toResponse(49-52)src/Results/ResultContract.php (2)
settled(22-22)toResponse(20-20)
src/Architecture.php (1)
src/Sidecar.php (1)
Sidecar(13-24)
🔇 Additional comments (68)
src/Exceptions/ConfigurationException.php (1)
3-3: Good modernization aligned with PHP 8.1+ best practices.The addition of
declare(strict_types=1);at the file level is a solid practice for the codebase modernization. Since this exception class has no methods, there are no compatibility concerns.src/Commands/Install.php (1)
3-4: Enabling strict types here is appropriate and low-riskAdding
declare(strict_types=1);aligns this command with the rest of the modernized codebase and should not change behavior, since this file doesn’t pass loosely-typed scalars into typed APIs or declare its own scalar-typed parameters/returns.Please rerun your test suite and a quick manual
sidecar:installin a sample app to confirm no unexpected strict-types regressions in this flow.src/Exceptions/NoFunctionsRegisteredException.php (1)
3-3: LGTM! Strict types declaration added.The addition of strict type checking aligns well with the modernization objectives.
src/WarmingConfig.php (2)
3-3: LGTM: Strict types enabled.The addition of
declare(strict_types=1);is appropriate for PHP 8.1+ modernization and improves type safety.
13-13: LGTM: Property and constructor types are correct.The explicit type declarations for
$instances(int) and$payload(array) align with the constructor parameter type, which is good. However, the setter methodsinstances()andwithPayload()need type hints to avoid runtime errors (see separate comments below).Also applies to: 15-17, 19-21
src/Concerns/ManagesEnvironments.php (4)
3-3: LGTM!The strict types declaration is a best practice for PHP 8.1+ and ensures strict type checking throughout this trait.
13-13: LGTM!The nullable string type declaration with explicit null default is appropriate and maintains backward compatibility while adding type safety.
15-18: LGTM!The string parameter type and void return type are correct and align with the property type and usage patterns.
20-23: LGTM!The void return type is appropriate for a method that only sets the property to null.
src/Sidecar.php (1)
3-4: LGTM!The
declare(strict_types=1);is correctly placed and this Facade class is compatible with strict typing sincegetFacadeAccessor()returns a string literal.src/Commands/Deploy.php (1)
3-4: LGTM!The strict types declaration is correctly applied. The command's
handle()method logic remains compatible with strict typing.src/Exceptions/LambdaExecutionException.php (1)
3-4: LGTM!Straightforward strict types addition to an exception class.
src/Commands/Activate.php (1)
3-4: LGTM!The strict types declaration is correctly applied to this command class.
src/Region.php (1)
3-4: LGTM!Good modernization: strict types declaration and explicit
: arrayreturn type onall(). The return type accurately reflectsReflectionClass::getConstants()output.Also applies to: 61-64
src/Exceptions/SidecarException.php (1)
3-3: Strict types addition here is safe
declare(strict_types=1);on this simple exception class has no behavioral impact and keeps it aligned with the rest of the codebase.src/Events/AfterFunctionsDeployed.php (1)
3-15: Constructor promotion & typing are consistent with usageUsing constructor property promotion with
public array $functions = []modernizes this event nicely and matches the pattern in the other event classes. Just ensure all call sites pass an array (or rely on the default), as non‑array payloads that may have slipped through before will now fail under strict typing.src/Clients/S3Client.php (1)
3-9: Thin S3Client wrapper is fineDefining
Hammerstone\Sidecar\Clients\S3Clientas a straight subclass ofAws\S3\S3Clientunder strict types is a clean way to type‑hint and bind this dependency in the container without altering behavior.src/Clients/CloudWatchLogsClient.php (1)
3-3: Strict types addition is non-invasive hereAdding
declare(strict_types=1);to this simple client subclass doesn’t change behavior and keeps it consistent with the rest of the clients.config/sidecar.php (1)
49-49: UsingArchitecture::X86_64->valueis the right default for a string configSwitching the default to the enum’s string value keeps the config purely scalar while still anchored to the
Architectureenum. This also meansSIDECAR_ARCHshould now always be a plain string (x86_64/arm64), which lines up with the deployment helpers using string architecture values.src/Clients/LambdaClient.php (1)
3-4: LGTM! Strict types enabled.The addition of
declare(strict_types=1);aligns with the PR's modernization objectives and enhances type safety throughout this client class.src/Manager.php (2)
3-4: LGTM! Strict types enabled.
30-30: LGTM! Property now explicitly typed.The addition of the
stringtype declaration for$executionVersionleverages PHP 8.0+ typed properties and improves type safety.tests/Unit/LambdaClientTest.php (1)
182-183: LGTM! Test expectations updated for string-backed architecture values.The test assertions now correctly expect string values (
['x86_64']) instead of enum constants, aligning with the Architecture enum refactoring in this PR.Also applies to: 193-193, 220-220, 230-230
src/Commands/EnvironmentAwareCommand.php (2)
3-4: LGTM! Strict types enabled.
27-27: LGTM! Return type added.The explicit
voidreturn type improves the method signature clarity and type safety.src/Clients/Configurations/AwsClientConfiguration.php (2)
3-4: LGTM! Strict types enabled.
11-11: LGTM! Return type now matches interface contract.The explicit
arrayreturn type correctly satisfies theAwsClientConfigurationContractinterface requirement (see src/Contracts/AwsClientConfiguration.php line 12).tests/Unit/Support/StringRuntimeTestFunction.php (1)
1-30: LGTM! Test support class for string-based runtime values.This fixture correctly implements a test function that returns a string runtime value (
'custom-runtime'), enabling validation of the runtime system's support for both enum and string values.src/Commands/Actions/CreateDeploymentUser.php (3)
3-4: LGTM! Strict types enabled.
17-17: LGTM! Property now uses typed declaration.The typed property
protected IamClient $clientreplaces the docblock approach with PHP 8.0+ native typing.
19-19: LGTM! Return type declaration added.The explicit
arrayreturn type correctly reflects all return paths in the method (lines 27, 112-115, 134-137, and viacreateAccessKey()).src/Exceptions/FunctionNotFoundException.php (2)
3-4: LGTM! Strict types enabled.
16-16: LGTM! Static factory method now properly typed.The explicit
: staticreturn type correctly reflects that the method returns an instance of the called class (line 26).src/Commands/Actions/BaseAction.php (1)
3-3: LGTM! Clean modernization with PHP 8.1+ features.The changes correctly apply:
- Strict types declaration
- Constructor property promotion with appropriate type hints
- Explicit return types on methods
The abstract
invoke(): mixedsignature appropriately allows concrete implementations to return different types.Also applies to: 15-18, 20-20, 22-22
src/Contracts/AwsClientConfiguration.php (1)
3-3: LGTM! Interface typing aligns with implementation.The strict types and explicit
arrayreturn type are correctly applied and match the concrete implementation insrc/Clients/Configurations/AwsClientConfiguration.php.Also applies to: 13-13
src/Providers/SidecarServiceProvider.php (1)
3-3: LGTM! Service provider correctly typed.All method signatures are properly typed:
register(): voidandboot(): voidfollow Laravel conventionsgetAwsClientConfiguration(): arrayaligns with the updated interface contractAlso applies to: 26-26, 47-47, 52-52
src/ArchitectureConstants.php (1)
7-16: Excellent backward-compatibility strategy.The deprecated constant shim provides a smooth migration path from constants to the new
Architectureenum while clearly marking the old API as deprecated.src/Commands/Actions/CreateExecutionRole.php (1)
3-3: LGTM! Type declarations are accurate.The changes correctly add:
- Strict types declaration
- Typed
IamClientpropertystringreturn type forinvoke()(returns the role ARN)All align with the modernization pattern and the abstract
BaseAction::invoke(): mixedsignature.Also applies to: 17-17, 19-19
src/Events/AfterFunctionsActivated.php (1)
3-3: LGTM! Clean event class modernization.Constructor property promotion with typed array parameter and default value is correctly applied, consistent with other event classes in the PR.
Also applies to: 13-15
src/Deployment.php (2)
3-3: LGTM! Property types correctly declared.The strict types and explicit property type declarations (
array $functions,LambdaClient $lambda) are appropriate.Also applies to: 22-24
104-104: LGTM! Correct usage of value accessor methods.The switch from
architecture()/runtime()toarchitectureValue()/runtimeValue()ensures string values are used in logs and deployment, correctly handling both enum and string-based configurations. Based on the code snippets, these helpers properly extract the underlying string value from enums or pass through string values directly.Also applies to: 107-107
tests/Unit/RuntimeTest.php (1)
16-101: All runtime identifiers verified against official AWS specifications.The test suite accurately validates the AWS Lambda runtime strings. Web search of AWS documentation confirms:
nodejs24.x,python3.14,java25,dotnet9,ruby3.4, andprovided.al2023are all official AWS Lambda runtime identifiers.The comprehensive test coverage is appropriate and correct.
tests/Unit/Support/EnumRuntimeTestFunction.php (1)
26-29: LGTM!The
runtime()method correctly returns theRuntime::PYTHON_312enum case with the appropriate union return type, aligning with the base class signature and the PR's enum-backed type modernization.src/Commands/Actions/DestroyAdminKeys.php (1)
3-3: LGTM - Strict typing and modern PHP features applied correctly.The additions of
declare(strict_types=1), typed property?string $key,staticreturn type on setter, andmixedreturn type oninvoke()are appropriate PHP 8.1+ modernizations.Also applies to: 16-18, 25-25
tests/Unit/ArchitectureTest.php (1)
16-46: LGTM - Comprehensive test coverage for Architecture enum.The tests thoroughly validate:
- Architecture is a backed enum
- Correct string values for X86_64 and ARM_64
- Construction from string values via
from()- Backward compatibility with ArchitectureConstants
src/Events/AfterFunctionExecuted.php (1)
3-3: LGTM - Clean constructor property promotion.The strict types declaration and constructor property promotion are correctly applied. The union type
PendingResult|SettledResultappropriately handles both execution result types.Also applies to: 17-21
src/Events/BeforeFunctionsDeployed.php (1)
3-3: LGTM - Consistent with other event classes.The strict types declaration and constructor property promotion follow the same pattern as the other event classes (
AfterFunctionsActivated,AfterFunctionsDeployed,BeforeFunctionsActivated), maintaining consistency across the codebase.Also applies to: 13-15
src/Results/PendingResult.php (1)
3-3: LGTM - Constructor property promotion applied correctly.The strict types declaration and constructor property promotion for
$rawand$functionare well-implemented. The typed nullable$settledproperty with default null is appropriate for lazy initialization.Also applies to: 20-25
src/Commands/Actions/CreateBucket.php (1)
3-4: LGTM - Clean modernization with strict types and typed properties.The additions of
declare(strict_types=1), typed properties, and the explicit return type align well with the PHP 8.1+ modernization goal. The covariant return typestring(narrowing fromBaseAction::invoke(): mixed) is valid.The typed properties
$clientand$bucketare initialized withininvoke()before any usage, so there's no risk of accessing uninitialized properties given the current class design.Also applies to: 12-12, 18-22
src/Commands/Actions/DetermineRegion.php (1)
3-4: Strict types and return type look good.The
declare(strict_types=1),Throwableimport (used in the catch block at line 52), and thestringreturn type are appropriate modernization changes.Also applies to: 13-13, 21-21
src/Finder.php (2)
3-4: Clean typed property declarations.The typed properties (
Collection,bool) align with the PHP 8.1+ modernization effort and improve code clarity.Also applies to: 22-26
101-107: Good fix: passing string toshouldExclude()instead of SplFileInfo.The change from
$this->shouldExclude($file)to$this->shouldExclude($file->getPathname())is a correctness fix. TheshouldExclude()method at line 138 expectsstring $path, and withdeclare(strict_types=1)enabled, passing theSplFileInfoobject directly would throw aTypeError.src/Events/BeforeFunctionsActivated.php (1)
3-4: LGTM - Consistent with sibling event classes.The constructor property promotion with
public array $functions = []is idiomatic PHP 8+ and matches the pattern inAfterFunctionsActivated,BeforeFunctionsDeployed, andAfterFunctionsDeployed.Note: The parameter now has a default value (
= []), making it optional. This is a minor API change but appropriate for an event class.Also applies to: 13-15
src/Architecture.php (1)
7-10: Backward compatibility layer is properly implemented and code is already using the enum correctly.The verification confirms:
ArchitectureConstants.phpexists with@deprecatednotices guiding migration to the Architecture enum- All production usages already employ
->valuecorrectly (e.g.,config/sidecar.php:49,LambdaFunction.php:256)- Type handling in
LambdaFunction.phpusesinstanceofchecks to handle both enum and string values gracefully- No breaking changes detected; the migration is complete
src/Events/BeforeFunctionExecuted.php (1)
3-18: LGTM!Clean modernization using PHP 8+ constructor property promotion. The
mixedtype for payload is appropriate given the flexible nature of Lambda payloads, and the pattern is consistent with other event classes in the codebase.src/Package.php (1)
33-43: LGTM!Proper use of typed properties with appropriate defaults. The nullable
?Collectionand?stringtypes correctly represent optional state that gets lazily initialized.src/LambdaFunction.php (3)
241-249: LGTM!The
runtimeValue()helper cleanly resolves both enum instances and raw strings to their string representation, enabling backward compatibility while supporting the new enum-based API.
261-269: LGTM!Consistent pattern with
runtimeValue()for resolving architecture to a string value.
449-449: LGTM!Correct use of
runtimeValue()andarchitectureValue()in the deployment array to ensure string values are passed to AWS SDK.Also applies to: 464-464
src/Runtime.php (2)
7-68: Clean migration from abstract class to string-backed enum.The enum structure is well-organized with proper deprecation annotations. The string values correctly match AWS Lambda runtime identifiers.
9-9: The flagged runtimes (nodejs24.x, nodejs22.x, python3.14) are now officially supported by AWS Lambda as of November 2025. The code uses the correct runtime identifier strings, and deployments with these values will succeed. The original concern about "anticipatory" runtimes that don't exist is no longer valid.tests/Unit/FunctionTest.php (1)
125-192: LGTM!Comprehensive test coverage for the new runtime and architecture handling:
- Tests enum return types and string resolution
- Covers both enum-based and string-based configuration paths
- Verifies deployment array contains correct values
Good coverage for the backward compatibility scenarios.
src/Results/SettledResult.php (4)
3-4: LGTM!Adding
declare(strict_types=1);aligns well with the PHP 8.1+ modernization objective.
25-36: LGTM!Good use of PHP 8.1+ constructor property promotion and typed properties with appropriate defaults. This is consistent with the
PendingResultclass pattern shown in the relevant snippets.
59-62: LGTM!The return type correctly matches the
ResultContractinterface signature.
194-201: LGTM!Good null-safety improvement. Returning early when
LogResultis null prevents potential issues withbase64_decodeon null values and aligns with the new strict types declaration.src/RuntimeConstants.php (2)
7-11: LGTM!Good approach for backward compatibility. Using an abstract class prevents instantiation while clearly marking the entire class as deprecated with a migration path to the
Runtimeenum.
12-107: Review comment is accurate—verification confirms all enum cases exist and PHP 8.1+ supports this pattern.The
Runtimeenum is properly defined atsrc/Runtime.php:7as a backed string enum with all required cases. The project requires PHP 8.1+ (^8.1in composer.json), which fully supports using->valueon backed enum cases within class constants. All constant mappings inRuntimeConstants.phplines 12–107 correctly reference their corresponding enum cases, and the deprecation annotations align with those in the enum definition.
| protected S3Client $client; | ||
|
|
||
| protected bool $isVapor = false; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused properties $client and $isVapor.
Neither property is used anywhere in this class:
$clientis declared but never assigned or accessed$isVaporis initialized tofalsebut never read
These appear to be remnants from a prior implementation or copy-paste from another action class.
class DetermineRegion extends BaseAction
{
- protected S3Client $client;
-
- protected bool $isVapor = false;
-
public function invoke(): stringAlso remove the unused import:
use Aws\S3\S3Client;
-use Illuminate\Support\Facades\File;
+use Illuminate\Support\Facades\File;Actually, File is used on line 42. Only remove the S3Client import:
-use Aws\S3\S3Client;
use Illuminate\Support\Facades\File;📝 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.
| protected S3Client $client; | |
| protected bool $isVapor = false; |
🤖 Prompt for AI Agents
In src/Commands/Actions/DetermineRegion.php around lines 17 to 19, remove the
two unused class properties protected S3Client $client; and protected bool
$isVapor = false; (they are never assigned or read) and also remove the unused
S3Client import at the top of the file; do not remove the File import (it’s used
on line 42). Ensure no other references rely on those properties or the S3Client
import before committing.
| public function getEnvironment(): string | ||
| { | ||
| return $this->environment ?? config('sidecar.env') ?? config('app.env'); | ||
| } |
There was a problem hiding this comment.
Potential TypeError: null return violates non-nullable string return type.
With declare(strict_types=1) enabled, this method will throw a TypeError if all three values ($this->environment, config('sidecar.env'), and config('app.env')) are null. Laravel's config() function returns null when a key doesn't exist, so this is a realistic scenario.
Apply this diff to provide a safe default:
public function getEnvironment(): string
{
- return $this->environment ?? config('sidecar.env') ?? config('app.env');
+ return $this->environment ?? config('sidecar.env') ?? config('app.env') ?? 'production';
}Alternatively, if null is a valid return value, change the return type to ?string:
-public function getEnvironment(): string
+public function getEnvironment(): ?string
{
return $this->environment ?? config('sidecar.env') ?? config('app.env');
}🤖 Prompt for AI Agents
In src/Concerns/ManagesEnvironments.php around lines 25 to 28, the
getEnvironment() method can return null which violates its string return type
under strict types; update the method so it never returns null by providing a
safe default (e.g. fallback to a known environment like 'production' or use
config('app.env', 'production') as the last fallback) or, if null is intended,
change the return type to ?string and adjust callers accordingly.
| public function architecture(): Architecture|string | ||
| { | ||
| return config('sidecar.architecture', Architecture::X86_64); | ||
| $arch = config('sidecar.architecture', Architecture::X86_64->value); | ||
|
|
||
| return $arch instanceof Architecture ? $arch : $arch; | ||
| } |
There was a problem hiding this comment.
Redundant ternary expression returns the same value in both branches.
The expression return $arch instanceof Architecture ? $arch : $arch; is a no-op—both branches return $arch unchanged. This appears to be a copy-paste artifact or incomplete refactor.
Apply this diff to simplify:
public function architecture(): Architecture|string
{
- $arch = config('sidecar.architecture', Architecture::X86_64->value);
-
- return $arch instanceof Architecture ? $arch : $arch;
+ return config('sidecar.architecture', Architecture::X86_64->value);
}📝 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.
| public function architecture(): Architecture|string | |
| { | |
| return config('sidecar.architecture', Architecture::X86_64); | |
| $arch = config('sidecar.architecture', Architecture::X86_64->value); | |
| return $arch instanceof Architecture ? $arch : $arch; | |
| } | |
| public function architecture(): Architecture|string | |
| { | |
| return config('sidecar.architecture', Architecture::X86_64->value); | |
| } |
🤖 Prompt for AI Agents
In src/LambdaFunction.php around lines 254 to 259 the ternary `return $arch
instanceof Architecture ? $arch : $arch;` is redundant because both branches
return $arch; replace that statement with a single return of $arch (i.e. remove
the ternary) so the method simply returns the resolved config value while
preserving the declared Architecture|string return type.
| /** | ||
| * @param Request $request | ||
| * @return Response | ||
| */ | ||
| public function toResponse($response); | ||
| public function toResponse(mixed $response); | ||
|
|
||
| /** | ||
| * @return SettledResult | ||
| */ | ||
| public function settled(); | ||
| public function settled(): SettledResult; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Script executed:
# Find and examine ResultContract.php to see the actual interface definition
fd -t f "ResultContract.php" -x cat -n {}Repository: aarondfrancis/sidecar
<shell_result>
Repository: aarondfrancis/sidecar
Exit code: 0
stdout:
1 <?php
2
3 declare(strict_types=1);
4
5 /**
6 * @author Aaron Francis <aaron@hammerstone.dev|https://twitter.com/aarondfrancis>
7 */
8
9 namespace Hammerstone\Sidecar\Results;
10
11 use Illuminate\Http\Request;
12 use Illuminate\Http\Response;
13
14 interface ResultContract
15 {
16 /**
17 * @param Request $request
18 * @return Response
19 */
20 public function toResponse(mixed $response);
21
22 public function settled(): SettledResult;
23 }
</shell_result>
Script executed:
# Search for implementations of ResultContract
rg -n "implements ResultContract" -A 10 srcRepository: aarondfrancis/sidecar
<shell_result>
Repository: aarondfrancis/sidecar
Exit code: 1
</shell_result>
Script executed:
# Find SettledResult and PendingResult implementations
fd -t f -e php "SettledResult\|PendingResult" -x cat -n {}Repository: aarondfrancis/sidecar
<shell_result>
Repository: aarondfrancis/sidecar
Exit code: 0
</shell_result>
Script executed:
# Check the current Laravel version in composer.json
cat -n composer.json | head -50Repository: aarondfrancis/sidecar
<shell_result>
Repository: aarondfrancis/sidecar
Exit code: 0
stdout:
1 {
2 "name": "hammerstone/sidecar",
3 "description": "A Laravel package to deploy Lambda functions alongside your main application.",
4 "type": "library",
5 "license": "MIT",
6 "authors": [
7 {
8 "name": "Aaron Francis",
9 "email": "aaron@hammerstone.dev"
10 }
11 ],
12 "require": {
13 "php": "^8.1",
14 "illuminate/filesystem": "^8|^9|^10|^11|^12.0",
15 "illuminate/console": "^8|^9|^10|^11|^12.0",
16 "illuminate/support": "^8|^9|^10|^11|^12.0",
17 "maennchen/zipstream-php": "^3.1",
18 "guzzlehttp/guzzle": "^6.5.8|^7.2",
19 "aws/aws-sdk-php": "^3.216.1"
20 },
21 "require-dev": {
22 "orchestra/testbench": "^5|^6|^7|^8|^9|^10.0",
23 "mockery/mockery": "^1.3.3",
24 "phpunit/phpunit": ">=8.5.23|^9|^10"
25 },
26 "autoload": {
27 "psr-4": {
28 "Hammerstone\\Sidecar\\": "src/"
29 }
30 },
31 "autoload-dev": {
32 "psr-4": {
33 "Hammerstone\\Sidecar\\Tests\\": "tests/"
34 }
35 },
36 "extra": {
37 "laravel": {
38 "providers": [
39 "Hammerstone\\Sidecar\\Providers\\SidecarServiceProvider"
40 ]
41 }
42 }
43 }
</shell_result>
Script executed:
# List all PHP files in the Results directory
fd -t f -e php src/Results/Repository: aarondfrancis/sidecar
<shell_result>
Repository: aarondfrancis/sidecar
Exit code: 1
stderr:
[fd error]: The search pattern 'src/Results/' contains a path-separation character ('/') and will not lead to any search results.
If you want to search for all files inside the 'src/Results/' directory, use a match-all pattern:
fd . 'src/Results/'
Instead, if you want your pattern to match the full file path, use:
fd --full-path 'src/Results/'
</shell_result>
Script executed:
# Search for SettledResult and PendingResult anywhere in src
rg -n "class SettledResult|class PendingResult" -A 5 srcRepository: aarondfrancis/sidecar
<shell_result>
Repository: aarondfrancis/sidecar
Exit code: 0
stdout:
src/Results/SettledResult.php:23:class SettledResult implements Responsable, ResultContract
src/Results/SettledResult.php-24-{
src/Results/SettledResult.php-25- protected array $report = [];
src/Results/SettledResult.php-26-
src/Results/SettledResult.php-27- protected ?string $requestId = null;
src/Results/SettledResult.php-28-
--
src/Results/PendingResult.php:18:class PendingResult implements Responsable, ResultContract
src/Results/PendingResult.php-19-{
src/Results/PendingResult.php-20- protected ?SettledResult $settled = null;
src/Results/PendingResult.php-21-
src/Results/PendingResult.php-22- public function __construct(
src/Results/PendingResult.php-23- protected PromiseInterface $raw,
</shell_result>
Laravel Responsable interface signature toResponse parameter type
<web_search_result>
The Responsable::toResponse method accepts an Illuminate\Http\Request instance. Signature:
public function toResponse(\Illuminate\Http\Request $request): \Symfony\Component\HttpFoundation\Response
Sources: [1], [2].
</web_search_result>
Script executed:
# View SettledResult.php completely
cat -n src/Results/SettledResult.phpRepository: aarondfrancis/sidecar
<shell_result>
Repository: aarondfrancis/sidecar
Exit code: 0
stdout:
1 <?php
2
3 declare(strict_types=1);
4
5 /**
6 * @author Aaron Francis <aaron@hammerstone.dev|https://twitter.com/aarondfrancis>
7 */
8
9 namespace Hammerstone\Sidecar\Results;
10
11 use Aws\Result;
12 use Carbon\Carbon;
13 use Exception;
14 use Hammerstone\Sidecar\Exceptions\LambdaExecutionException;
15 use Hammerstone\Sidecar\LambdaFunction;
16 use Illuminate\Contracts\Support\Responsable;
17 use Illuminate\Http\Request;
18 use Illuminate\Http\Response;
19 use Illuminate\Support\Arr;
20 use Illuminate\Support\Str;
21 use Throwable;
22
23 class SettledResult implements Responsable, ResultContract
24 {
25 protected array $report = [];
26
27 protected ?string $requestId = null;
28
29 protected array $logs = [];
30
31 public function __construct(
32 protected Result $raw,
33 protected LambdaFunction $function
34 ) {
35 $this->logs = $this->parseLogs();
36 }
37
38 /**
39 * @return bool
40 */
41 public function isError()
42 {
43 return $this->raw->get('FunctionError') !== '';
44 }
45
46 /**
47 * @return Result
48 */
49 public function rawAwsResult()
50 {
51 return $this->raw;
52 }
53
54 /**
55 * This is here as a little nicety for the developer, so that they
56 * can call `settled` on either kind of result (PendingResult
57 * or SettledResult) and get a SettledResult back.
58 */
59 public function settled(): SettledResult
60 {
61 return $this;
62 }
63
64 /**
65 * @param Request $request
66 * @return Response
67 *
68 * @throws Exception
69 */
70 public function toResponse(mixed $request)
71 {
72 return $this->function->toResponse($request, $this);
73 }
74
75 /**
76 * @param ?int $numberOfBacktraces
77 *
78 * Throw an exception if there was an error, otherwise do nothing.
79 *
80 * @throws Exception
81 */
82 public function throw(?int $numberOfBacktraces)
83 {
84 if (!$this->isError()) {
85 return $this;
86 }
87
88 throw new LambdaExecutionException(sprintf('Lambda Execution Exception for %s: "%s".', ...[
89 get_class($this->function),
90 $this->errorAsString($numberOfBacktraces)
91 ]));
92 }
93
94 /**
95 * The Payload of the result is always JSON encoded, even if it's not JSON.
96 *
97 * @param int $options
98 * @return mixed
99 */
100 public function body($options = JSON_OBJECT_AS_ARRAY)
101 {
102 return json_decode((string)$this->raw->get('Payload'), $options);
103 }
104
105 /**
106 * @return array
107 */
108 public function logs()
109 {
110 return $this->logs;
111 }
112
113 /**
114 * An aggregated report of the stats.
115 *
116 * @return array
117 */
118 public function report()
119 {
120 return [
121 'request' => $this->requestId(),
122 'billed_duration' => $this->billedDurationMs(),
123 'execution_duration' => $this->executionDurationMs(),
124 'cold_boot_delay' => $this->coldBootDelayMs(),
125 'total_duration' => $this->totalDurationMs(),
126 'max_memory' => $this->maxMemoryUsedMb(),
127 'memory' => $this->memorySizeMb(),
128 ];
129 }
130
131 public function requestId()
132 {
133 return $this->requestId;
134 }
135
136 public function executionDurationMs()
137 {
138 return $this->unitlessNumberFromReport('Duration');
139 }
140
141 public function coldBootDelayMs()
142 {
143 return $this->unitlessNumberFromReport('Init Duration') ?? 0;
144 }
145
146 public function billedDurationMs()
147 {
148 return $this->unitlessNumberFromReport('Billed Duration');
149 }
150
151 public function totalDurationMs()
152 {
153 return $this->coldBootDelayMs() + $this->executionDurationMs();
154 }
155
156 public function maxMemoryUsedMb()
157 {
158 return $this->unitlessNumberFromReport('Max Memory Used');
159 }
160
161 public function memorySizeMb()
162 {
163 return $this->unitlessNumberFromReport('Memory Size');
164 }
165
166 public function trace()
167 {
168 if (!$this->isError()) {
169 return [];
170 }
171
172 return Arr::get($this->body(), 'trace', []);
173 }
174
175 public function errorAsString(int $numberOfBacktraces = 2)
176 {
177 if (!$this->isError()) {
178 return '';
179 }
180
181 $message = Arr::get($this->body(), 'errorMessage', 'Unknown error.');
182
183 // Only the number of backtraces (plus the error) for the string.
184 $trace = array_slice($this->trace(), 0, $numberOfBacktraces + 1);
185 $trace = implode(' ', array_map('trim', $trace));
186
187 if ($trace) {
188 return "$message. [TRACE] $trace";
189 }
190
191 return $message;
192 }
193
194 protected function parseLogs(): array
195 {
196 $logResult = $this->raw->get('LogResult');
197 if ($logResult === null) {
198 return [];
199 }
200
201 $lines = base64_decode($logResult);
202 $lines = explode("\n", $lines);
203
204 $lines = array_map(function ($line) use (&$reportLineReached) {
205 try {
206 if ($reportLineReached) {
207 return null;
208 }
209
210 if (Str::startsWith($line, 'START RequestId:')) {
211 return $this->parseStartLine($line);
212 }
213
214 if (Str::startsWith($line, 'END RequestId:')) {
215 return null;
216 }
217
218 if (Str::startsWith($line, 'REPORT RequestId')) {
219 $reportLineReached = true;
220
221 return $this->parseReportLine($line);
222 }
223
224 if ($line === '') {
225 return null;
226 }
227
228 return $this->parseInfoLine($line);
229 } catch (Throwable $exception) {
230 return $this->unknownLine($line);
231 }
232 }, $lines);
233
234 return array_values(array_filter($lines));
235 }
236
237 protected function parseStartLine($line)
238 {
239 $this->requestId = Str::after(
240 Str::before($line, ' Version'),
241 'START RequestId: '
242 );
243 }
244
245 protected function parseInfoLine($line)
246 {
247 $parts = explode("\t", $line);
248
249 if (count($parts) < 4) {
250 return $this->unknownLine($line);
251 }
252
253 $body = $parts[3];
254
255 if ($body === 'Invoke Error ') {
256 $body .= $parts[4];
257 }
258
259 return [
260 'timestamp' => Carbon::make($parts[0])->timestamp,
261 'level' => $parts[2],
262 'body' => $body
263 ];
264 }
265
266 protected function unknownLine($line)
267 {
268 return [
269 'timestamp' => now()->timestamp,
270 'level' => 'UNKNOWN',
271 'body' => $line,
272 ];
273 }
274
275 protected function parseReportLine($line)
276 {
277 $parts = array_filter(explode("\t", $line));
278 array_shift($parts);
279
280 $this->report = collect($parts)->mapWithKeys(function ($part) {
281 [$key, $value] = explode(': ', $part);
282
283 return [$key => $value];
284 })->toArray();
285 }
286
287 protected function unitlessNumberFromReport($key)
288 {
289 if (!Arr::has($this->report, $key)) {
290 return;
291 }
292
293 // Coerce from a string into either a float or an int.
294 return 0 + head(explode(' ', $this->report[$key]));
295 }
296 }
</shell_result>
Script executed:
# View PendingResult.php completely
cat -n src/Results/PendingResult.phpRepository: aarondfrancis/sidecar
<shell_result>
Repository: aarondfrancis/sidecar
Exit code: 0
stdout:
1 <?php
2
3 declare(strict_types=1);
4
5 /**
6 * @author Aaron Francis <aaron@hammerstone.dev|https://twitter.com/aarondfrancis>
7 */
8
9 namespace Hammerstone\Sidecar\Results;
10
11 use Exception;
12 use GuzzleHttp\Promise\PromiseInterface;
13 use Hammerstone\Sidecar\LambdaFunction;
14 use Illuminate\Contracts\Support\Responsable;
15 use Illuminate\Http\Request;
16 use Illuminate\Http\Response;
17
18 class PendingResult implements Responsable, ResultContract
19 {
20 protected ?SettledResult $settled = null;
21
22 public function __construct(
23 protected PromiseInterface $raw,
24 protected LambdaFunction $function
25 ) {}
26
27 public function settled(): SettledResult
28 {
29 if ($this->settled) {
30 return $this->settled;
31 }
32
33 return $this->settled = $this->function->toSettledResult($this->raw->wait());
34 }
35
36 public function rawPromise(): PromiseInterface
37 {
38 return $this->raw;
39 }
40
41 /**
42 * Defer to the SettledResult.
43 *
44 * @param Request $request
45 * @return Response
46 *
47 * @throws Exception
48 */
49 public function toResponse(mixed $request)
50 {
51 return $this->settled()->toResponse($request);
52 }
53 }
</shell_result>
Parameter name mismatch between interface and implementations creates signature confusion
The review comment identified real issues, but the severity and nature differ from what was stated:
-
Parameter name mismatch (critical clarity issue)
ResultContract::toResponsedeclares the parameter asmixed $response(line 20)- Both implementations (
SettledResult::toResponseandPendingResult::toResponse) usemixed $request - The docblock documents
@param Request $requestwhich conflicts with the declared parameter name$response - This creates a signature contract mismatch: the interface says "I accept
$response" but implementations use$request, confusing developers and static analysis tools
-
Type signature is actually compatible with Laravel's Responsable
- Laravel's
Responsable::toResponse($request)uses an untyped parameter - The implementations using
toResponse(mixed $request)are actually stricter (contravariant), which is allowed and compatible in PHP - This is not a breaking incompatibility; it's a legitimate type refinement
- Laravel's
-
BC break confirmed for return type on
settled()- Adding
: SettledResulttosettled()in the interface breaks any external implementations without this return type - Both internal implementations already have it, suggesting this is acceptable as a major version bump
- Adding
Recommended fix:
Update the interface to align parameter name with implementations and docblock:
/**
* @param Request $request
* @return Response
*/
- public function toResponse(mixed $response);
+ public function toResponse(mixed $request);This resolves the parameter naming inconsistency and aligns the interface contract with its implementations.
📝 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.
| /** | |
| * @param Request $request | |
| * @return Response | |
| */ | |
| public function toResponse($response); | |
| public function toResponse(mixed $response); | |
| /** | |
| * @return SettledResult | |
| */ | |
| public function settled(); | |
| public function settled(): SettledResult; | |
| } | |
| /** | |
| * @param Request $request | |
| * @return Response | |
| */ | |
| public function toResponse(mixed $request); | |
| public function settled(): SettledResult; | |
| } |
🤖 Prompt for AI Agents
In src/Results/ResultContract.php around lines 16 to 23, rename the toResponse
parameter from mixed $response to mixed $request and update the docblock to
@param Request $request so the interface parameter name and phpdoc match the
implementations (SettledResult::toResponse and PendingResult::toResponse); keep
the settled(): SettledResult return type if you accept the breaking change
(major version) or remove it to preserve BC, then run static analysis/tests to
verify no signature mismatches remain.
Removed unused S3Client import and two unused class properties ($client and $isVapor) that were never assigned or read. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Added 'production' as a safe default fallback when neither sidecar.env nor app.env are configured, ensuring the string return type is satisfied. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Main added new runtimes as class constants; our refactor converted Runtime to a backed enum. Kept the enum format with all runtimes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Update composer.json to require Laravel 10+ - Remove Guzzle 6.x support (Laravel 10+ requires Guzzle 7) - Update CI matrix to only test Laravel 10, 11, 12 - Update documentation to reflect supported versions This resolves CI failures caused by Composer blocking Laravel 8/9 due to security advisories. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Using EnumCase->value in class constant initializers is not supported in PHP 8.1 (only available from 8.2+). Hardcode the string values instead to maintain backward compatibility. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Testbench v9.0.0/9.0.1 has a bug with undeclared $latestResponse static property that causes all tests to fail. 🤖 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>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary
declare(strict_types=1)to all 44 PHP source files@deprecatedannotationsRuntimeConstantsandArchitectureConstantsfor backwards compatibilitygetEnvironment()potential null return under strict typesarchitecture()method no-op ternaryLambdaClientTestexpectations for architecture valuesDetermineRegionRuntimeenum in customization docsTest plan
🤖 Generated with Claude Code