Update build-cli.yaml #141
Conversation
Signed-off-by: Rob Landers <landers.robert@gmail.com>
WalkthroughThe workflow file Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
![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)
cli/Makefile (1)
1-18: LGTM! Clean whitespace removal.The trailing whitespace cleanup improves code hygiene without affecting functionality.
Consider adding phony targets for better Makefile practices:
+.PHONY: all clean test + TARGET := dphp-linux-*cli/cli.go (1)
93-112: Remove commented code instead of leaving it in place.The CPU profiling functionality has been disabled by commenting out the code. For cleaner code maintenance, consider fully removing the commented lines instead of leaving them in place.
- //profile, err := os.CreateTemp("", "") - //if err != nil { - // panic(err) - //} - //err = pprof.StartCPUProfile(profile) - //if err != nil { - // panic(err) - //} - go func() { sigs := make(chan os.Signal, 1) signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM) <-sigs - //pprof.StopCPUProfile() - //profile.Close() - - //logger.Warn("Profile output", zap.String("Filename", profile.Name())) - os.RemoveAll(data) os.Exit(0) }()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
cli/go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
.idea/codeception.xml(0 hunks).idea/durable-php.iml(2 hunks).idea/php.xml(2 hunks).idea/phpspec.xml(0 hunks)Dockerfile(1 hunks)cli/Makefile(1 hunks)cli/cli.go(3 hunks)cli/glue/glue.go(1 hunks)cli/go.mod(3 hunks)cli/lib/api.go(1 hunks)composer.json(1 hunks)src/Events/EventQueue.php(3 hunks)src/Events/TaskFailed.php(1 hunks)src/Glue/glue.php(4 hunks)src/RemoteOrchestrationClient.php(4 hunks)tests/ClientTestCli.php(3 hunks)
💤 Files with no reviewable changes (2)
- .idea/codeception.xml
- .idea/phpspec.xml
✅ Files skipped from review due to trivial changes (7)
- tests/ClientTestCli.php
- Dockerfile
- cli/lib/api.go
- .idea/durable-php.iml
- src/Glue/glue.php
- src/Events/EventQueue.php
- .idea/php.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/go.mod
🧰 Additional context used
🪛 checkmake (0.2.2)
cli/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-osx (arm64)
- GitHub Check: build-linux (arm64)
- GitHub Check: build-linux (amd64)
- GitHub Check: build-docker
- GitHub Check: Unit Tests - PHP
🔇 Additional comments (8)
src/Events/TaskFailed.php (1)
47-48: LGTM! Improved type consistency and clarity.The change from
stringwith defaultnullto explicitstring|nullmakes the method signature consistent with the constructor's parameter types and provides better type safety. This correctly reflects that these parameters can accept either a string or null value.cli/glue/glue.go (1)
171-171: LGTM! Consistent logger option removal.The removal of
frankenphp.WithRequestLogger(logger)aligns with the coordinated simplification of FrankenPHP logging configuration across the CLI codebase.cli/cli.go (1)
203-203: LGTM! Consistent FrankenPHP options simplification.The removal of the logger option from FrankenPHP initialization aligns with the coordinated cleanup across the CLI codebase, maintaining only the essential thread count configuration.
composer.json (2)
40-40: LGTM! Version bump aligns with API changes.The withinboredom/time package major version bump from v5 to v6 is properly coordinated with the corresponding code changes that update TimeUnit to Unit throughout the codebase.
36-36: Assess the PHP 8.4 Requirement and CompatibilityThe version constraint in composer.json was bumped from
>=8.3to>=8.4(line 36). PHP 8.4 brings major features (e.g., property hooks, asymmetric visibility, new array functions, enhanced JIT, HTTP/3 in cURL) but is only installed on ~13.7% of PHP sites as of mid-2025 and still in “beta support” on WordPress.Please verify:
- Whether the codebase actually uses any PHP 8.4-only features (e.g.,
array_find, property hooks, asymmetric visibility).- That your CI pipelines, hosting environments, and any CMS/platform dependencies (WordPress, Drupal, package ecosystem) fully support PHP 8.4.
- If you’re not leveraging 8.4-specific functionality, consider reverting to
>=8.3for broader compatibility.Files to review:
- composer.json (line 36)
src/RemoteOrchestrationClient.php (3)
38-38: LGTM! Import updated for new package version.The import statement correctly reflects the API change from TimeUnit to Unit in the withinboredom/time v6 package.
52-52: LGTM! Improved multibyte string handling.The change from
rtrimtomb_rtrimensures proper handling of multibyte characters in the API host string, which is a good improvement for internationalization support.
171-173: LGTM! Timeout configurations updated for new API.The timeout method calls correctly use the new
Unit::Secondsconstant instead of the deprecatedTimeUnit::Seconds, maintaining the same timeout values while adapting to the updated package API.
Signed-off-by: Robert Landers <landers.robert@gmail.com>
This reverts commit 1584155.
Signed-off-by: Robert Landers <landers.robert@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Proxy/ClientProxy.php (1)
121-126: Same unused parameter issue as OrchestratorProxy.Consider applying the same fix suggested for OrchestratorProxy.
🧹 Nitpick comments (2)
src/Proxy/OrchestratorProxy.php (1)
112-117: Consider removing unused parameter.The
$classparameter is not used in the preamble method implementation.-protected function preamble(ReflectionClass $class): string +protected function preamble(ReflectionClass $class): string // phpcs:ignore Generic.CodeAnalysis.UnusedFunctionParameterOr update the abstract class to make this parameter optional if it's not needed in all implementations.
src/Proxy/SpyProxy.php (1)
38-84: Hook implementation looks good, but consider method name handling.The hook-specific logic correctly generates simplified get/set hooks without parameters or return types, which aligns with the pattern in other proxy classes. The
$getHookvariable usage is correct despite the static analysis warning.However, consider ensuring method names are handled consistently:
protected function impureCall(ReflectionMethod $method, bool $isHook = false): string { $getHook = 'return '; if ($isHook && str_ends_with($method->getName(), 'get')) { $name = 'get'; } elseif ($isHook && str_ends_with($method->getName(), 'set')) { $name = 'set'; $getHook = ''; } else { - $name = $method->getName(); + $name = $isHook ? $method->getName() : $method->getName(); }Actually, the current implementation is fine - the
$namevariable correctly holds the method name for non-hook cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.idea/php.xml(3 hunks)cli/lib/api.go(3 hunks)src/Proxy/ClientProxy.php(4 hunks)src/Proxy/Generator.php(8 hunks)src/Proxy/OrchestratorProxy.php(4 hunks)src/Proxy/SpyProxy.php(3 hunks)tests/Feature/ClientProxyTest.php(3 hunks)tests/Feature/OrchestratorProxyTest.php(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- cli/lib/api.go
🚧 Files skipped from review as they are similar to previous changes (1)
- .idea/php.xml
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/Proxy/Generator.php (3)
src/Proxy/ClientProxy.php (5)
getName(73-76)impureCall(83-119)pureMethod(33-71)preamble(121-126)impureSignal(78-81)src/Proxy/OrchestratorProxy.php (5)
getName(81-84)impureCall(38-79)pureMethod(33-36)preamble(112-117)impureSignal(86-110)src/Proxy/SpyProxy.php (5)
getName(86-89)impureCall(38-84)pureMethod(33-36)preamble(96-101)impureSignal(91-94)
src/Proxy/SpyProxy.php (3)
src/Proxy/Generator.php (6)
Generator(35-156)pureMethod(129-129)impureCall(133-133)getName(65-65)impureSignal(131-131)preamble(155-155)src/Proxy/ClientProxy.php (5)
pureMethod(33-71)impureCall(83-119)getName(73-76)impureSignal(78-81)preamble(121-126)src/Proxy/OrchestratorProxy.php (5)
pureMethod(33-36)impureCall(38-79)getName(81-84)impureSignal(86-110)preamble(112-117)
🪛 PHPMD (2.15.0)
src/Proxy/OrchestratorProxy.php
112-112: Avoid unused parameters such as '$class'. (Unused Code Rules)
(UnusedFormalParameter)
src/Proxy/SpyProxy.php
33-33: Avoid unused parameters such as '$isHook'. (Unused Code Rules)
(UnusedFormalParameter)
40-40: Avoid unused local variables such as '$getHook'. (Unused Code Rules)
(UnusedLocalVariable)
96-96: Avoid unused parameters such as '$class'. (Unused Code Rules)
(UnusedFormalParameter)
src/Proxy/ClientProxy.php
121-121: Avoid unused parameters such as '$class'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build-osx (arm64)
- GitHub Check: build-osx (x86_64)
- GitHub Check: build-linux (amd64)
- GitHub Check: build-linux (arm64)
- GitHub Check: build-docker
- GitHub Check: Unit Tests - PHP
🔇 Additional comments (10)
tests/Feature/OrchestratorProxyTest.php (2)
31-38: LGTM! Proper test coverage for property hooks.The addition of the public property with getter/setter hooks correctly tests the new proxy generation feature for PHP 8.4 property hooks.
48-52: Good improvement: Snapshot testing for generated proxies.Using snapshot assertions instead of hardcoded string comparisons improves test maintainability.
tests/Feature/ClientProxyTest.php (2)
26-36: Good consistency with test structure.The property addition matches the OrchestratorProxyTest, and the test rename improves clarity by specifying "client proxy".
Also applies to: 46-46
56-58: Correct implementation of interface property.The anonymous class properly implements the new
propproperty required by the interface.src/Proxy/OrchestratorProxy.php (1)
38-79: Well-implemented property hook support.The implementation correctly handles both getter and setter hooks, preserving the original method names for entity calls while generating appropriate proxy methods.
src/Proxy/Generator.php (1)
99-126: Excellent implementation of property hook generation.The property generation logic correctly:
- Handles both getter and setter hooks
- Preserves type information
- Filters out empty hooks for clean output
- Integrates seamlessly with the existing proxy generation flow
src/Proxy/ClientProxy.php (2)
35-64: Setter hook implementation looks good.The implementation correctly generates setter hooks that delegate to
__setmethods on the source object. Note that this assumes a specific naming convention for the generated methods.
85-112: Correct implementation of purity constraints.Getter hooks appropriately throw
ImpureExceptionto maintain purity constraints in client proxies.src/Proxy/SpyProxy.php (2)
27-29: LGTM: Import statements cleaned up properly.The import statements have been properly updated to use the reflection classes without leading backslashes, which is consistent with modern PHP practices and aligns with similar changes in other proxy classes.
86-101: LGTM: Method signatures updated for consistency.The type hints have been properly updated to use the imported reflection classes without leading backslashes. The unused parameter warning for
$classinpreambleis a false positive since it's required by the abstract parent class signature.
Signed-off-by: Robert Landers <landers.robert@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/Proxy/SpyProxy.php (1)
38-89: Fix unused variable and undefined variable issues.Two issues in the hook implementation:
- The
$getHookvariable is declared but never used in the hook template- The
$valuevariable is referenced but not defined in the current scopeApply this fix:
protected function impureCall(ReflectionMethod $method, bool $isHook = false): string { - $getHook = 'return '; if ($isHook && str_ends_with($method->getName(), 'get')) { $name = 'get'; } elseif ($isHook && str_ends_with($method->getName(), 'set')) { $name = 'set'; - $getHook = ''; } else { $name = $method->getName(); } // ... rest of method if ($isHook) { $hookName = $method->getName(); - if (str_ends_with($hookName, 'get')) { - $value = '[$value]'; - } else { - $value = '[]'; - } + $value = str_ends_with($hookName, 'get') ? '[]' : '[$value]'; return <<<EOT {$name} { \$this->operation = "{$hookName}"; \$this->arguments = {$value}; throw new \Exception('Not implemented'); } EOT; }
♻️ Duplicate comments (1)
src/Proxy/SpyProxy.php (1)
33-36: Pass the$isHookparameter to maintain consistency.The
$isHookparameter is not being passed toimpureCall, which breaks consistency with other proxy implementations and makes the parameter unused.Apply this fix:
protected function pureMethod(ReflectionMethod $method, bool $isHook = false): string { - return $this->impureCall($method); + return $this->impureCall($method, $isHook); }
🧹 Nitpick comments (2)
src/Proxy/OrchestratorProxy.php (1)
117-122: Consider removing unused parameter if not required by interface.The
$classparameter is not used in the method body. If this parameter is not required by the parent class interface, consider removing it to clean up the method signature.-protected function preamble(ReflectionClass $class): string +protected function preamble(): stringsrc/Proxy/SpyProxy.php (1)
101-106: Consider removing unused parameter if not required by interface.The
$classparameter is not used in the method body. If this parameter is not required by the parent class interface, consider removing it to clean up the method signature.-protected function preamble(ReflectionClass $class): string +protected function preamble(): string
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Proxy/OrchestratorProxy.php(4 hunks)src/Proxy/SpyProxy.php(3 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
src/Proxy/SpyProxy.php
33-33: Avoid unused parameters such as '$isHook'. (Unused Code Rules)
(UnusedFormalParameter)
40-40: Avoid unused local variables such as '$getHook'. (Unused Code Rules)
(UnusedLocalVariable)
101-101: Avoid unused parameters such as '$class'. (Unused Code Rules)
(UnusedFormalParameter)
src/Proxy/OrchestratorProxy.php
117-117: Avoid unused parameters such as '$class'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-linux (amd64)
- GitHub Check: build-osx (arm64)
- GitHub Check: build-linux (arm64)
- GitHub Check: build-docker
- GitHub Check: Unit Tests - PHP
🔇 Additional comments (3)
src/Proxy/OrchestratorProxy.php (2)
33-36: LGTM! Parameter delegation implemented correctly.The
$isHookparameter is properly passed through toimpureCall, maintaining consistency with the updated method signature.
38-84: No undefined$value—approving changes.The
$valuevariable is explicitly assigned in both branches of the hook case before being interpolated, so there’s no undefined reference.src/Proxy/SpyProxy.php (1)
27-30: LGTM! Import cleanup follows PHP standards.Removing leading backslashes from use statements is the correct approach and aligns with PSR-12 coding standards.
Signed-off-by: Robert Landers <landers.robert@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Proxy/Generator.php (1)
27-33: PropertyHookType import issue persists.This is the same issue previously identified - the
PropertyHookTypeimport still lacks its proper namespace qualification.
🧹 Nitpick comments (1)
src/Proxy/SpyProxy.php (1)
38-90: Hook implementation works correctly for spy functionality.The hook logic properly handles property getter/setter scenarios for the spy proxy, setting appropriate operation tracking and throwing the expected "Not implemented" exception.
However, there's a minor cleanup opportunity:
- $getHook = true; if ($isHook && str_ends_with($method->getName(), 'get')) { $name = 'get'; } elseif ($isHook && str_ends_with($method->getName(), 'set')) { $name = 'set'; - $getHook = false; } else { $name = $method->getName(); }The
$getHookvariable is set but never used since the condition directly checks the method name.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Proxy/Generator.php(8 hunks)src/Proxy/OrchestratorProxy.php(4 hunks)src/Proxy/SpyProxy.php(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Proxy/OrchestratorProxy.php (4)
src/Proxy/SpyProxy.php (4)
pureMethod(33-36)impureCall(38-90)getName(92-95)preamble(102-107)src/Proxy/Generator.php (4)
pureMethod(129-129)impureCall(133-133)getName(65-65)preamble(155-155)src/Proxy/ClientProxy.php (4)
pureMethod(33-71)impureCall(83-119)getName(73-76)preamble(121-126)src/EntityContext.php (1)
return(87-95)
🪛 PHPMD (2.15.0)
src/Proxy/OrchestratorProxy.php
118-118: Avoid unused parameters such as '$class'. (Unused Code Rules)
(UnusedFormalParameter)
src/Proxy/SpyProxy.php
40-40: Avoid unused local variables such as '$getHook'. (Unused Code Rules)
(UnusedLocalVariable)
102-102: Avoid unused parameters such as '$class'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build-osx (arm64)
- GitHub Check: build-osx (x86_64)
- GitHub Check: build-linux (arm64)
- GitHub Check: build-linux (amd64)
- GitHub Check: build-docker
- GitHub Check: Unit Tests - PHP
🔇 Additional comments (13)
src/Proxy/OrchestratorProxy.php (4)
33-36: LGTM: Method signature correctly updated for hook support.The
pureMethodsignature now properly matches the abstract method definition and correctly passes the$isHookparameter toimpureCall.
38-85: Well-implemented hook support for property getters and setters.The logic correctly differentiates between property getter and setter hooks:
- Getters use
returnto provide the retrieved value- Setters omit
returnas they don't need to return a value- Both properly call through the orchestration context using
waitOneandcallEntityThe implementation maintains backward compatibility for non-hook method calls.
111-116: Good formatting improvements for consistency.The indentation changes improve code readability and maintain consistent formatting across the class.
118-123: Formatting improvements enhance readability.The consistent indentation improves code formatting. The static analysis warning about the unused
$classparameter can be safely ignored as it's required by the abstract method signature.src/Proxy/Generator.php (6)
41-42: Excellent import usage improvements.Using imported class names instead of fully qualified names throughout the code significantly improves readability and maintainability.
Also applies to: 52-53, 65-65, 74-74, 79-79
99-117: Well-structured property hook generation.The implementation correctly:
- Retrieves public properties using reflection
- Generates type-hinted property declarations
- Adds getter/setter hooks when present using the hook-aware proxy methods
- Filters out empty hooks to keep the generated code clean
This provides comprehensive support for PHP property hooks in proxy generation.
119-126: Class template properly integrates property generation.The updated template correctly positions the generated properties before methods, following standard PHP class structure conventions.
129-129: Method signatures correctly updated for hook support.Adding the optional
$isHookparameter with a default value maintains backward compatibility while enabling the new property hook functionality.Also applies to: 133-133
135-153: Type handling method improved with cleaner imports.The updates to use imported reflection types improve code consistency and readability while maintaining the same functionality.
155-155: Preamble method signature updated consistently.The signature update to use imported
ReflectionClassmaintains consistency with the overall import cleanup improvements.src/Proxy/SpyProxy.php (3)
27-30: Clean import additions improve readability.Adding proper import statements for reflection classes improves code consistency and readability throughout the file.
33-36: Previous feedback successfully addressed.The method now correctly passes the
$isHookparameter toimpureCall, resolving the consistency issue identified in previous reviews.
92-95: Method signatures updated consistently with import cleanup.The updates to use imported reflection classes improve consistency across the file. The static analysis warning about the unused
$classparameter inpreamblecan be ignored as it's required by the abstract method interface.Also applies to: 97-100, 102-107
Signed-off-by: Robert Landers <landers.robert@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/State/EntityHistory.php(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build-linux (arm64)
- GitHub Check: build-linux (amd64)
- GitHub Check: build-osx (x86_64)
- GitHub Check: build-osx (arm64)
- GitHub Check: build-docker
- GitHub Check: Unit Tests - PHP
🔇 Additional comments (7)
src/State/EntityHistory.php (7)
71-78: LGTM! Improved constructor formatting.The multiline parameter formatting with trailing commas enhances readability and makes future parameter additions cleaner in version control diffs.
165-165: LGTM! Cleaner anonymous class instantiation.Removing the empty parentheses is the preferred PHP syntax when no constructor parameters are needed.
169-177: LGTM! Consistent multiline formatting.The Status constructor formatting aligns with the class constructor style, improving code consistency and readability.
266-267: LGTM! Appropriate use of goto for control flow.The goto label provides a clean way to avoid code duplication for the finalization logic when handling property operations.
288-291: LGTM! Improved array_filter readability.The multiline formatting makes the filter logic more readable while maintaining the same functionality.
125-130: LGTM! Consistent multiline method call formatting.The formatting aligns with the file's overall style and improves readability of nested method calls.
270-272: LGTM! Consistent formatting across similar constructs.The multiline formatting matches the pattern used elsewhere in the file, maintaining consistency.
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Proxy/SpyProxy.php (1)
33-36: ✅ Previous issue resolved - parameter now properly passed through.The
$isHookparameter is now correctly passed toimpureCall, addressing the consistency issue identified in previous reviews.
🧹 Nitpick comments (1)
src/Proxy/SpyProxy.php (1)
40-40: Remove unused variable.The
$getHookvariable is assigned but never used in the method.- $getHook = true; - if ($isHook && str_ends_with($method->getName(), 'get')) { + if ($isHook && str_ends_with($method->getName(), 'get')) { $name = 'get'; } elseif ($isHook && str_ends_with($method->getName(), 'set')) { $name = 'set'; - $getHook = false; } else {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tests/.pest/snapshots/Feature/ClientProxyTest/it_generates_a_client_proxy_correctly.snapis excluded by!**/*.snaptests/.pest/snapshots/Feature/OrchestratorProxyTest/it_generates_a_proxy_correctly.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
src/Proxy/SpyProxy.php(3 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
src/Proxy/SpyProxy.php
40-40: Avoid unused local variables such as '$getHook'. (Unused Code Rules)
(UnusedLocalVariable)
102-102: Avoid unused parameters such as '$class'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-linux (arm64)
- GitHub Check: build-docker
- GitHub Check: build-linux (amd64)
- GitHub Check: Unit Tests - PHP
🔇 Additional comments (4)
src/Proxy/SpyProxy.php (4)
27-30: LGTM! Import statement cleanup follows PHP best practices.Removing the leading backslashes from the use statements is the correct approach and improves code consistency.
66-81: LGTM! Property hook generation logic is well-implemented.The hook-specific code generation correctly handles getter/setter hooks with appropriate operation and argument handling. The differentiation between 'get' and 'set' hooks and the specialized method body generation aligns with property hook requirements.
92-92: LGTM! Method signatures updated consistently with import changes.The method signatures correctly use the imported reflection classes, maintaining consistency with the import statement updates.
Also applies to: 97-97
102-107: No changes needed:$classparameter is required by the parent abstract methodThe
Generatorbase class declares an abstract method
• src/Proxy/Generator.php:155:abstract protected function preamble(ReflectionClass $class): string;
All subclasses must match this signature, so even if unused internally, the$classparameter cannot be removed.
Signed-off-by: Robert Landers <landers.robert@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/State/OrchestrationInstance.php (1)
27-28: Consistent import pattern with architectural refactoring.The imports support the same refactoring pattern as EntityId.php. Note that
Stringableimport style changed from\StringabletoStringable(without leading backslash) - ensure this style is consistent across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.idea/durable-php.iml(2 hunks).idea/php.xml(3 hunks)composer.json(1 hunks)src/State/EntityId.php(1 hunks)src/State/OrchestrationInstance.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .idea/durable-php.iml
🚧 Files skipped from review as they are similar to previous changes (2)
- composer.json
- .idea/php.xml
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/State/EntityId.php (2)
src/State/OrchestrationInstance.php (1)
__construct(32-32)src/State/Ids/StateId.php (1)
__construct(41-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build-linux (amd64)
- GitHub Check: build-linux (arm64)
- GitHub Check: build-osx (x86_64)
- GitHub Check: build-docker
- GitHub Check: build-osx (arm64)
- GitHub Check: Unit Tests - PHP
🔇 Additional comments (5)
src/State/EntityId.php (3)
28-28: New dependency import added for Record base class.The import of
Withinboredom\Recordsupports the architectural refactoring from readonly class to extending a record-based infrastructure.
39-39: Property visibility changed to protected(set) for controlled mutability.The change from public properties to
protected(set)maintains read access while restricting write access to the class hierarchy. This provides controlled mutability that aligns with the Record base class approach.
33-33: Ensure EntityId property access remains valid after switching to RecordSwitching from a public
readonly classtoextends Recordwithprotected(set)properties may break any direct reads of$entityId->nameor$entityId->id. Please confirm that theWithinboredom\Recordbase class still exposes these values (e.g. via magic__getor public getters); if not, update all call sites to use the correct accessor method.Key locations to verify:
- src/State/EntityHistory.php around:
$this->name = $this->id->toEntityId()->name;- Any other occurrences of
->nameor->idonEntityIdinstancessrc/State/OrchestrationInstance.php (2)
30-30: Consistent architectural change: readonly to Record-based.This matches the same pattern applied to EntityId.php, creating consistency in the state management refactoring. The change from readonly class to extending Record provides controlled mutability.
32-32: Property visibility consistently updated to protected(set).The constructor properties follow the same pattern as EntityId.php, changing from public to
protected(set)for controlled access. This maintains the consistency of the refactoring approach across state-related classes.
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
cli/lib/api.go (1)
318-781: Consider refactoring to reduce code duplication.The entity and orchestration endpoints have nearly identical logic. Consider extracting common functionality into helper functions to improve maintainability.
For example, you could create helper functions like:
func handleShareOwnership(writer http.ResponseWriter, request *http.Request, ctx context.Context, rm *auth.ResourceManager, stateId *glue.StateId, logger *zap.Logger, config *config.Config, newUserId string) { // Common sharing logic } func handleGrantPermission(writer http.ResponseWriter, request *http.Request, ctx context.Context, rm *auth.ResourceManager, stateId *glue.StateId, logger *zap.Logger, config *config.Config, grantType, user, operation string) { // Common granting logic } func handleRevokePermission(writer http.ResponseWriter, request *http.Request, ctx context.Context, rm *auth.ResourceManager, stateId *glue.StateId, logger *zap.Logger, config *config.Config, revokeType, user string) { // Common revocation logic }This would eliminate the duplication between entity and orchestration endpoints while maintaining the same functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/lib/api.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-linux (amd64)
- GitHub Check: build-docker
- GitHub Check: build-osx (arm64)
- GitHub Check: build-linux (arm64)
- GitHub Check: Unit Tests - PHP
🔇 Additional comments (3)
cli/lib/api.go (3)
102-104: LGTM! Consistent logger removal.The removal of
frankenphp.WithRequestLogger(logger)aligns with similar changes across the codebase and maintains consistency.
887-887: Verify if escaped quotes are needed in error message.The added escaped quotes in the error message seem unnecessary for HTTP responses and may display incorrectly to clients.
- http.Error(writer, "\"Internal Server Error\"", http.StatusInternalServerError) + http.Error(writer, "Internal Server Error", http.StatusInternalServerError)The escaped quotes will likely show up as literal quote characters in the response body, which is not typical for HTTP error messages.
947-947: LGTM! Typo fix.Correctly fixes the spelling from "Unkown" to "Unknown" in the log message.
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
cli/lib/api.go (3)
691-691: Replace http.Error with WriteHeader for success response.Same issue as the entity sharing endpoint - using
http.Errorfor success responses is inconsistent.- http.Error(writer, "", http.StatusOK) + writer.WriteHeader(http.StatusOK)
694-767: Orchestration grant endpoint needs missing context and logging.The handler is missing correlation ID handling and request logging that other endpoints have.
Add the missing context and logging after the method validation:
if request.Method != "PUT" { http.Error(writer, "Method Not Allowed", http.StatusMethodNotAllowed) return } + ctx := getCorrelationId(ctx, &request.Header, nil) + logRequest(logger, request, ctx) + vars := mux.Vars(request)
769-819: Orchestration revoke endpoint needs missing context and logging.The handler is missing correlation ID handling and request logging that other endpoints have.
Add the missing context and logging after the method validation:
if request.Method != "DELETE" { http.Error(writer, "Method Not Allowed", http.StatusMethodNotAllowed) return } + ctx := getCorrelationId(ctx, &request.Header, nil) + logRequest(logger, request, ctx) + vars := mux.Vars(request)
🧹 Nitpick comments (1)
cli/lib/api.go (1)
369-369: Replace http.Error with WriteHeader for success response.Using
http.Errorwith an empty message for a 200 OK response is unusual. Consider usingwriter.WriteHeader(http.StatusOK)instead for consistency with successful responses.- http.Error(writer, "", http.StatusOK) + writer.WriteHeader(http.StatusOK)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cli/lib/api.go(5 hunks)src/DurableClient.php(3 hunks)src/EntityClientInterface.php(2 hunks)src/OrchestrationClientInterface.php(2 hunks)src/RemoteEntityClient.php(6 hunks)src/RemoteOrchestrationClient.php(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/EntityClientInterface.php (4)
cli/auth/user.go (1)
Operation(11-11)src/DurableClient.php (5)
shareEntityOwnership(147-150)grantEntityAccessToUser(152-155)grantEntityAccessToRole(157-160)revokeEntityAccessToUser(162-165)revokeEntityAccessToRole(167-170)src/RemoteEntityClient.php (5)
shareEntityOwnership(168-178)grantEntityAccessToUser(180-190)grantEntityAccessToRole(192-202)revokeEntityAccessToUser(204-214)revokeEntityAccessToRole(216-226)src/State/EntityId.php (1)
EntityId(33-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build-docker
- GitHub Check: build-osx (arm64)
- GitHub Check: build-osx (x86_64)
- GitHub Check: build-linux (amd64)
- GitHub Check: build-linux (arm64)
- GitHub Check: Unit Tests - PHP
🔇 Additional comments (26)
src/EntityClientInterface.php (2)
27-27: LGTM! Clean import addition.The import of the
Operationclass is properly placed and follows the existing import conventions in the file.
85-94: Well-designed access control interface methods.The five new methods provide a comprehensive access control API with consistent signatures:
- Clear separation between ownership sharing and permission grants
- Type-safe operation specification using the
Operationenum- Consistent parameter naming and ordering
- Proper distinction between user and role-based permissions
The interface design aligns well with the corresponding implementations in
RemoteEntityClient.phpand the API endpoints incli/lib/api.go.src/RemoteOrchestrationClient.php (6)
30-30: LGTM! Proper import for access control operations.The import of the
Operationclass is correctly added to support the new access control methods.
39-39: Good modernization with time unit import.The change from
TimeUnittoUnitappears to be part of a broader refactoring to modernize time handling across the codebase.
53-53: Excellent use of multibyte-safe string function.The switch from
rtrim()tomb_rtrim()ensures proper handling of multibyte characters in API host URLs, which is important for internationalization support.
151-151: Good use of factory method for StateId creation.Using
StateId::fromString()instead of direct construction is a good practice that provides better encapsulation and consistency across the codebase.
172-174: Consistent timeout handling with updated time units.The timeout configuration properly uses the new
Unit::Secondsenum and maintains the same timeout values (1 hour inactivity/transfer, 30 seconds connect).
197-255: Comprehensive orchestration access control implementation.The six new methods provide complete access control functionality:
- Consistent HTTP request patterns: All methods follow the same structure with proper URL encoding, authorization headers, and error handling
- Proper error handling: Each method checks for non-200 status codes and throws descriptive exceptions
- RESTful API design: Uses appropriate HTTP methods (PUT for grants, DELETE for revokes)
- Type safety: Uses the
Operationenum for specifying permission typesThe implementation mirrors the entity access control methods, providing a consistent API experience across the codebase.
src/OrchestrationClientInterface.php (5)
27-27: LGTM! Proper import for access control operations.The import of the
Operationclass is correctly added to support the new access control method signatures.
30-30: Good import addition for Generator type.Adding the explicit
Generatorimport improves code clarity and follows PHP best practices for type declarations.
38-38: Improved return type specification.The change from
\GeneratortoGeneratorwith the explicit import is cleaner and more consistent with modern PHP practices.
48-48: Clean nullable type syntax.The simplification from
string|nullto?stringis more concise and follows modern PHP nullable type conventions.
56-65: Well-designed orchestration access control interface.The six new methods provide comprehensive access control functionality with:
- Consistent naming convention: Methods follow the pattern
{action}Orchestration{AccessType}To{TargetType}- Type safety: Uses
OrchestrationInstanceandOperationtypes appropriately- Clear separation of concerns: Distinct methods for sharing ownership vs granting/revoking specific permissions
- User/role flexibility: Separate methods for user and role-based permissions
The interface design matches the entity access control methods, providing API consistency across the system.
src/DurableClient.php (3)
28-28: LGTM! Proper import for access control operations.The import of the
Operationclass is correctly added to support the new access control method signatures.
147-170: Excellent facade implementation for entity access control.The five entity access control methods properly delegate to the underlying
entityClient:
- Clean delegation pattern: Each method simply passes through parameters to the appropriate client method
- Consistent signatures: Method signatures match the interface definitions exactly
- Proper encapsulation: The facade hides the complexity of managing separate clients
- Type safety: Uses
EntityIdandOperationtypes appropriatelyThis implementation provides a unified API while maintaining separation of concerns.
172-195: Excellent facade implementation for orchestration access control.The five orchestration access control methods properly delegate to the underlying
orchestrationClient:
- Parallel design: Methods mirror the entity access control methods for consistency
- Clean delegation: Simple pass-through to the orchestration client
- Type consistency: Uses
OrchestrationInstanceandOperationtypes appropriately- Unified interface: Provides single entry point for both entity and orchestration operations
The implementation maintains the facade pattern while providing comprehensive access control functionality.
cli/lib/api.go (6)
104-104: LGTM! Simplified FrankenPHP request creation.The removal of
frankenphp.WithRequestLogger(logger)aligns with similar simplifications mentioned in the PR summary acrosscli/glue/glue.goandcli/cli.go.
985-985: Good typo fix.Corrected "Unkown" to "Unknown" in the log message.
318-370: Entity sharing endpoint implementation looks good.The handler properly implements entity ownership sharing with:
- CORS handling and method validation
- Proper authorization check with
auth.Ownerpermission- Resource discovery with error handling
- Ownership sharing operation with error handling
- Resource update with error handling
- Appropriate logging
The implementation addresses the authorization and error handling patterns correctly.
640-692: Orchestration sharing endpoint implementation looks good.The handler properly implements orchestration ownership sharing with all the required components:
- CORS and method validation
- Context and logging setup
- Authorization check with
auth.Ownerpermission- Resource discovery and error handling
- Ownership sharing with proper error handling
- Resource update with error handling
- Appropriate logging
The implementation follows the same pattern as the entity sharing endpoint.
465-465: Permission level verified ✅
Theauth.ShareMinuspermission is explicitly defined as “whether the share can see other shares and manage them,” and both the API’sauthorize(..., auth.ShareMinus)call and theRevokeUser/RevokeRolemethods enforce this same permission. No changes needed.
390-390: No change required: SharePlus correctly governs share invitations
TheSharePlusoperation is explicitly defined as “invite more shares” (cli/auth/user.go) and is consistently used in bothauthorize()andResource.Grant*()methods to guard granting any share-based permissions. RequiringOwnerhere would be overly broad—SharePlusis the intended permission level for this endpoint.src/RemoteEntityClient.php (4)
29-30: LGTM: Import additions support new functionality.The new imports for
OperationandSpyExceptionare correctly added and used throughout the file for the entity sharing functionality and improved exception handling.
52-52: Good improvement: Using multibyte-safe string trimming.Switching from
rtrim()tomb_rtrim()ensures proper handling of API host URLs that may contain multibyte characters.
61-65: LGTM: Improved code formatting.The multiline formatting of the Request instantiation with trailing commas improves readability and follows PHP best practices.
83-100: Excellent improvements to exception handling and code clarity.The changes implement several good practices:
- Specific exception catching (
SpyExceptionvsThrowable) for more precise error handling- Defensive programming with null initialization and early return guard
- Consistent use of the
EntityId()factory function- Improved code formatting with multiline method calls
Signed-off-by: Robert Landers <landers.robert@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
cli/glue/glue.go (1)
131-141: Consider more graceful error handling instead of panic.The authorization context integration looks good. However, using
panicfor errors fromDiscoverResourcecould crash the entire process. Consider returning an error or handling it more gracefully, especially since this appears to be in a request handling path.- rm := auth.GetResourceManager(ctx, stream) - res, err := rm.DiscoverResource(ctx, id, logger, true) - if err != nil { - logger.Error("DiscoverResource", zap.Error(err)) - panic(err) - } - if res != nil { - ac, _ := rm.ToAuthContext(ctx, res) - headers.Add("DPHP_AUTH_CONTEXT", string(ac)) - } + rm := auth.GetResourceManager(ctx, stream) + res, err := rm.DiscoverResource(ctx, id, logger, true) + if err != nil { + logger.Error("DiscoverResource", zap.Error(err)) + // Consider returning error to caller instead of panic + writer.status = http.StatusInternalServerError + return writer.events, writer.Header(), writer.status, writer.DeleteAfter + } + if res != nil { + ac, err := rm.ToAuthContext(ctx, res) + if err != nil { + logger.Warn("Failed to convert resource to auth context", zap.Error(err)) + } else { + headers.Add("DPHP_AUTH_CONTEXT", string(ac)) + } + }cli/auth/resourceManager.go (1)
92-131: Consider using structs for cleaner JSON marshaling.The manual construction of maps for JSON marshaling is verbose and error-prone. Consider defining structs with appropriate JSON tags for a cleaner, more maintainable approach.
+type authContextJSON struct { + ContextID contextID `json:"contextId"` + Owners []share `json:"owners"` + Shares []share `json:"shares"` +} + +type contextID struct { + ID string `json:"id"` +} + +type share struct { + ShareType string `json:"shareType"` + Subject string `json:"subject"` + Allowed []string `json:"allowed"` +} + func (r *ResourceManager) ToAuthContext(ctx context.Context, resource *Resource) ([]byte, error) { - var owners []map[string]interface{} + authCtx := authContextJSON{ + ContextID: contextID{ID: resource.id.String()}, + Owners: make([]share, 0, len(resource.Owners)), + Shares: make([]share, 0, len(resource.Shares)), + } for o, _ := range resource.Owners { - owners = append(owners, map[string]interface{}{ - "shareType": "owner", - "subject": string(o), - "allowed": []string{string(Owner)}, + authCtx.Owners = append(authCtx.Owners, share{ + ShareType: "owner", + Subject: string(o), + Allowed: []string{string(Owner)}, }) } - var shares []map[string]interface{} - for _, s := range resource.Shares { if u, ok := s.(*UserShare); ok { - shares = append(shares, map[string]interface{}{ - "shareType": "user", - "subject": string(u.UserId), - "allowed": maps.Keys(u.AllowedOperations), + authCtx.Shares = append(authCtx.Shares, share{ + ShareType: "user", + Subject: string(u.UserId), + Allowed: maps.Keys(u.AllowedOperations), }) } if r, ok := s.(*RoleShare); ok { - shares = append(shares, map[string]interface{}{ - "shareType": "role", - "subject": string(r.Role), - "allowed": maps.Keys(r.AllowedOperations), + authCtx.Shares = append(authCtx.Shares, share{ + ShareType: "role", + Subject: string(r.Role), + Allowed: maps.Keys(r.AllowedOperations), }) } } - c := map[string]interface{}{ - "contextId": map[string]string{ - "id": resource.id.String(), - }, - "owners": owners, - "shares": shares, - } - - return json.Marshal(c) + return json.Marshal(authCtx) }src/Contexts/AuthContext.php (1)
28-35: Consider error handling for deserialization failures.The implementation is correct, but consider adding error handling for cases where the header exists but contains invalid serialized data.
public static function fromCurrentContext(): ?AuthContext { if (isset($_SERVER['HTTP_DPHP_AUTH_CONTEXT'])) { - return Serializer::deserialize($_SERVER['HTTP_DPHP_AUTH_CONTEXT'], self::class); + try { + return Serializer::deserialize($_SERVER['HTTP_DPHP_AUTH_CONTEXT'], self::class); + } catch (Exception $e) { + // Log the error or handle as appropriate for your use case + return null; + } } return null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cli/auth/resourceManager.go(2 hunks)cli/glue/glue.go(3 hunks)composer.json(2 hunks)src/Contexts/AuthContext.php(1 hunks)src/Contexts/AuthContext/Share.php(1 hunks)src/Contexts/AuthContext/Share/Owner.php(1 hunks)src/Contexts/AuthContext/Share/Role.php(1 hunks)src/Contexts/AuthContext/Share/User.php(1 hunks)src/Contexts/AuthContext/functions.php(1 hunks)src/EntityContext.php(3 hunks)src/EntityContextInterface.php(2 hunks)src/Events/RevokeRole.php(2 hunks)src/Events/RevokeUser.php(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/Contexts/AuthContext/Share/Role.php
- src/Contexts/AuthContext/Share/User.php
- src/Contexts/AuthContext/Share/Owner.php
🚧 Files skipped from review as they are similar to previous changes (1)
- composer.json
🧰 Additional context used
🧬 Code Graph Analysis (6)
src/Events/RevokeUser.php (1)
src/Events/RevokeRole.php (2)
__construct(34-37)completely(39-42)
src/EntityContext.php (8)
src/Events/GiveOwnership.php (1)
GiveOwnership(28-44)src/Events/ShareOwnership.php (1)
ShareOwnership(28-44)cli/auth/user.go (1)
Operation(11-11)src/State/Ids/StateId.php (1)
fromEntityId(68-71)src/EntityContextInterface.php (6)
shareOwnership(117-117)giveOwnership(127-127)grantUser(119-119)grantRole(121-121)revokeUser(123-123)revokeRole(125-125)src/Events/WithEntity.php (2)
WithEntity(29-59)forInstance(36-43)src/Events/RevokeRole.php (1)
completely(39-42)src/Events/RevokeUser.php (1)
completely(39-42)
src/Contexts/AuthContext/functions.php (5)
src/Contexts/AuthContext.php (1)
AuthContext(12-36)cli/auth/user.go (2)
Share(67-70)Operation(11-11)src/Contexts/AuthContext/Share/Owner.php (1)
Owner(29-29)src/Contexts/AuthContext/Share/Role.php (1)
Role(7-7)src/Contexts/AuthContext/Share/User.php (1)
User(7-7)
src/EntityContextInterface.php (1)
src/EntityContext.php (6)
shareOwnership(199-207)grantUser(219-227)grantRole(229-237)revokeUser(239-247)revokeRole(249-257)giveOwnership(209-217)
src/Contexts/AuthContext.php (2)
src/Contexts/AuthContext/Share/Owner.php (1)
Owner(29-29)src/Contexts/AuthContext/functions.php (1)
Owner(11-16)
src/Contexts/AuthContext/Share.php (6)
src/Contexts/AuthContext.php (1)
AuthContext(12-36)cli/auth/user.go (2)
Share(67-70)Operation(11-11)src/Contexts/AuthContext/Share/Owner.php (1)
Owner(29-29)src/Contexts/AuthContext/functions.php (3)
Owner(11-16)Role(18-23)User(25-30)src/Contexts/AuthContext/Share/Role.php (1)
Role(7-7)src/Contexts/AuthContext/Share/User.php (1)
User(7-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-linux (arm64)
- GitHub Check: build-linux (amd64)
- GitHub Check: build-docker
- GitHub Check: Unit Tests - PHP
🔇 Additional comments (14)
cli/glue/glue.go (1)
184-184: LGTM!The simplification of the FrankenPHP request creation by removing the logger option is appropriate.
src/Events/RevokeRole.php (3)
34-34: LGTM!Using the nullable shorthand
?Operationis more concise and follows modern PHP conventions.
39-42: LGTM!The rename to
completelyfollows PHP naming conventions for static factory methods and is consistent with similar changes in related files.
46-46: Good catch!Fixed the syntax error by removing the extraneous trailing comma in the sprintf call.
src/Events/RevokeUser.php (1)
34-42: LGTM!The changes maintain consistency with
RevokeRole.php:
- Using nullable shorthand
?Operation- Renaming to
completelyfollowing PHP conventions- Removing the unused
StateId $targetparameter simplifies the APIsrc/Contexts/AuthContext.php (2)
12-12: LGTM - Well-designed class declaration.The abstract readonly class design is appropriate for an authorization context, preventing direct instantiation while allowing subclassing and ensuring immutability.
14-26: LGTM - Well-structured properties with proper serialization.The property definitions are clean with appropriate type annotations and serialization attributes. The separation of
$ownersand$sharesprovides good architectural clarity.src/Contexts/AuthContext/Share.php (2)
13-17: LGTM - Proper polymorphic serialization setup.The
StaticTypeMapconfiguration correctly maps discriminator values to their corresponding subclasses, enabling proper polymorphic deserialization.
18-26: LGTM - Clean property definitions for authorization model.The properties are well-structured with proper type annotations and serialization attributes, supporting the authorization system's subject-operation model effectively.
src/EntityContext.php (3)
27-34: LGTM - Complete imports for authorization functionality.The new imports properly support the added ownership and permission management methods with all necessary event classes and operation types.
141-141: LGTM - Consistent factory function usage.The change from direct instantiation to the
OrchestrationInstance()factory function aligns with the codebase-wide refactoring for consistency.
199-257: LGTM - Consistent and well-structured authorization methods.All six authorization methods follow a clean, consistent pattern:
- Event-driven architecture using the event dispatcher
- Proper entity targeting with
WithEntity::forInstance- Appropriate event classes with static factory methods
- Variadic
Operationparameters for flexible permission grantingThe implementation integrates well with the existing event system and maintains architectural consistency.
src/EntityContextInterface.php (2)
27-27: LGTM - Necessary import for authorization methods.The
Operationimport is required for the new authorization method signatures and is properly placed.
117-127: LGTM - Complete and consistent interface contract.The method signatures properly define the authorization functionality contract:
- Clear method names indicating their purpose
- Appropriate parameter types with variadic
Operationparameters for flexible permission granting- Consistent void return types for fire-and-forget operations
The interface matches the implementation in
EntityContext.phpexactly.
Signed-off-by: Robert Landers <landers.robert@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
cli/lib/api.go (9)
394-394: Add missing correlation ID and request logging.if request.Method != "PUT" { http.Error(writer, "Method Not Allowed", http.StatusMethodNotAllowed) return } + ctx := getCorrelationId(ctx, &request.Header, nil) + logRequest(logger, request, ctx) + vars := mux.Vars(request)
469-469: Add missing correlation ID and request logging.if request.Method != "DELETE" { http.Error(writer, "Method Not Allowed", http.StatusMethodNotAllowed) return } + ctx := getCorrelationId(ctx, &request.Header, nil) + logRequest(logger, request, ctx) + vars := mux.Vars(request)
716-716: Add missing correlation ID and request logging.if request.Method != "PUT" { http.Error(writer, "Method Not Allowed", http.StatusMethodNotAllowed) return } + ctx := getCorrelationId(ctx, &request.Header, nil) + logRequest(logger, request, ctx) + vars := mux.Vars(request)
791-791: Add missing correlation ID and request logging.if request.Method != "DELETE" { http.Error(writer, "Method Not Allowed", http.StatusMethodNotAllowed) return } + ctx := getCorrelationId(ctx, &request.Header, nil) + logRequest(logger, request, ctx) + vars := mux.Vars(request)
937-937: Fix JSON encoding issue in error response.The error response has extra quotes which will result in malformed JSON.
- http.Error(writer, "\"Internal Server Error\"", http.StatusInternalServerError) + http.Error(writer, "Internal Server Error", http.StatusInternalServerError)
433-434: Add missing return statements and fix error responses.if err != nil { logger.Error("Failed to discover resource", zap.Error(err)) - http.Error(writer, "", http.StatusNotFound) + http.Error(writer, "Not Found", http.StatusNotFound) + return } switch vars["type"] { case "user": err = r.GrantUser(auth.UserId(vars["user"]), operation, ctx) case "role": err = r.GrantRole(auth.Role(vars["user"]), operation, ctx) } if err != nil { logger.Error("Failed to grant resource", zap.Error(err)) - http.Error(writer, "", http.StatusForbidden) + http.Error(writer, "Forbidden", http.StatusForbidden) + return } err = r.Update(ctx, logger) if err != nil { logger.Error("Failed to update resource", zap.Error(err)) - http.Error(writer, "", http.StatusInternalServerError) + http.Error(writer, "Internal Server Error", http.StatusInternalServerError) + return } - http.Error(writer, "", http.StatusOK) + writer.WriteHeader(http.StatusOK)Also applies to: 445-446, 452-453, 456-456
485-486: Add missing return statements and fix error responses.if err != nil { logger.Error("Failed to discover resource", zap.Error(err)) - http.Error(writer, "", http.StatusNotFound) + http.Error(writer, "Not Found", http.StatusNotFound) + return } switch vars["type"] { case "user": err = r.RevokeUser(auth.UserId(vars["user"]), ctx) case "role": err = r.RevokeRole(auth.Role(vars["user"]), ctx) } if err != nil { logger.Error("Failed to revoke resource", zap.Error(err)) - http.Error(writer, "", http.StatusForbidden) + http.Error(writer, "Forbidden", http.StatusForbidden) + return } err = r.Update(ctx, logger) if err != nil { logger.Error("Failed to update resource", zap.Error(err)) - http.Error(writer, "", http.StatusInternalServerError) + http.Error(writer, "Internal Server Error", http.StatusInternalServerError) + return } - http.Error(writer, "", http.StatusOK) + writer.WriteHeader(http.StatusOK)Also applies to: 497-498, 504-505, 508-508
755-756: Add missing return statements and fix error responses.if err != nil { logger.Error("Failed to discover resource", zap.Error(err)) - http.Error(writer, "", http.StatusNotFound) + http.Error(writer, "Not Found", http.StatusNotFound) + return } switch vars["type"] { case "user": err = r.GrantUser(auth.UserId(vars["user"]), operation, ctx) case "role": err = r.GrantRole(auth.Role(vars["user"]), operation, ctx) } if err != nil { logger.Error("Failed to grant resource", zap.Error(err)) - http.Error(writer, "", http.StatusForbidden) + http.Error(writer, "Forbidden", http.StatusForbidden) + return } err = r.Update(ctx, logger) if err != nil { logger.Error("Failed to update resource", zap.Error(err)) - http.Error(writer, "", http.StatusInternalServerError) + http.Error(writer, "Internal Server Error", http.StatusInternalServerError) + return } - http.Error(writer, "", http.StatusOK) + writer.WriteHeader(http.StatusOK)Also applies to: 767-768, 774-775, 778-778
807-808: Add missing return statements and fix error responses.if err != nil { logger.Error("Failed to discover resource", zap.Error(err)) - http.Error(writer, "", http.StatusNotFound) + http.Error(writer, "Not Found", http.StatusNotFound) + return } switch vars["type"] { case "user": err = r.RevokeUser(auth.UserId(vars["user"]), ctx) case "role": err = r.RevokeRole(auth.Role(vars["user"]), ctx) } if err != nil { logger.Error("Failed to revoke resource", zap.Error(err)) - http.Error(writer, "", http.StatusForbidden) + http.Error(writer, "Forbidden", http.StatusForbidden) + return } err = r.Update(ctx, logger) if err != nil { logger.Error("Failed to update resource", zap.Error(err)) - http.Error(writer, "", http.StatusInternalServerError) + http.Error(writer, "Internal Server Error", http.StatusInternalServerError) + return } - http.Error(writer, "", http.StatusOK) + writer.WriteHeader(http.StatusOK)Also applies to: 819-820, 826-827, 830-830
🧹 Nitpick comments (2)
cli/lib/api.go (2)
381-381: Use WriteHeader for successful responses.Replace
http.ErrorwithWriteHeaderfor successful responses as there's no error message.- http.Error(writer, "", http.StatusOK) + writer.WriteHeader(http.StatusOK)
703-703: Use WriteHeader for successful responses.- http.Error(writer, "", http.StatusOK) + writer.WriteHeader(http.StatusOK)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cli/auth/resource.go(5 hunks)cli/auth/resourceManager.go(3 hunks)cli/cli.go(11 hunks)cli/glue/glue.go(7 hunks)cli/glue/response_writer.go(3 hunks)cli/glue/state.go(1 hunks)cli/ids/id.go(2 hunks)cli/lib/api.go(16 hunks)cli/lib/billing.go(3 hunks)cli/lib/consumer.go(6 hunks)cli/lib/indexer.go(6 hunks)cli/lib/locks.go(4 hunks)src/Contexts/AuthContext.php(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- cli/ids/id.go
- cli/glue/response_writer.go
- cli/glue/state.go
- cli/lib/locks.go
- cli/auth/resource.go
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Contexts/AuthContext.php
- cli/auth/resourceManager.go
- cli/cli.go
- cli/glue/glue.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
cli/lib/billing.go (2)
cli/ids/id.go (2)
StateId(176-179)ParseStateId(118-130)cli/glue/response_writer.go (3)
HeaderStateId(20-20)HeaderEmittedAt(25-25)HeaderEmittedBy(24-24)
cli/lib/indexer.go (4)
cli/config/config.go (1)
Config(66-72)cli/ids/id.go (4)
IdKind(9-9)Entity(13-13)ParseStateId(118-130)Orchestration(14-14)cli/glue/state.go (1)
GetObjectStore(9-16)cli/lib/index.go (1)
CreateOrchestrationIndex(39-74)
cli/lib/consumer.go (2)
cli/ids/id.go (3)
IdKind(9-9)ParseStateId(118-130)Entity(13-13)cli/glue/response_writer.go (2)
HeaderStateId(20-20)HeaderEmittedBy(24-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-osx (arm64)
- GitHub Check: build-linux (arm64)
- GitHub Check: build-docker
- GitHub Check: build-linux (amd64)
- GitHub Check: Unit Tests - PHP
🔇 Additional comments (7)
cli/lib/billing.go (1)
7-7: LGTM! Consistent migration to theidspackage.The changes correctly migrate state ID handling from the
gluepackage to theidspackage, which aligns with the broader refactoring effort across the codebase.Also applies to: 46-46, 91-91, 93-93
cli/lib/indexer.go (1)
7-7: LGTM! Consistent migration to theidspackage.All references to
IdKindtype, constants (Entity,Orchestration), and theParseStateIdfunction have been correctly updated to use theidspackage instead ofglue.Also applies to: 19-19, 25-25, 103-103, 123-123, 144-144, 152-152, 230-230
cli/lib/consumer.go (2)
9-9: LGTM! Consistent migration to theidspackage.All state ID parsing and kind comparisons have been correctly updated to use the
idspackage.Also applies to: 20-20, 59-59, 102-103, 132-132
295-303: Authorization context injection looks good.The resource discovery and authorization context injection follows the established error handling pattern (panic on error) and correctly adds the context to headers for downstream processing.
cli/lib/api.go (3)
8-8: LGTM! Consistent migration to theidspackage.All ID type references and related functions have been correctly updated from the
gluepackage to theidspackage throughout the file.Also applies to: 105-105, 168-170, 259-259, 523-526, 639-642, 848-851, 914-914, 979-982, 1011-1011, 1081-1081, 1093-1093
997-997: Good catch on the typo fix.Fixed "Unkown" to "Unknown" in the log message.
264-273: Authorization context injection implemented correctly.The resource discovery and authorization context header injection follows the same pattern as in
consumer.goand maintains consistency with the error handling approach in this function.
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cli/cli.go (1)
546-567: The input validation and error handling issues remain unaddressed.The parsing logic for roles and claims still lacks proper validation as identified in the previous review. The code will fail with empty options, extra whitespace, and provides unclear error messages.
The previous review comment's suggestions for improving input validation, whitespace handling, and error messages are still applicable and should be implemented.
🧹 Nitpick comments (1)
cli/cli.go (1)
94-102: Remove commented CPU profiling code entirely.The commented CPU profiling code should be removed completely rather than left as comments, as it creates unnecessary code clutter.
- //profile, err := os.CreateTemp("", "") - //if err != nil { - // panic(err) - //} - //err = pprof.StartCPUProfile(profile) - //if err != nil { - // panic(err) - //}- //pprof.StopCPUProfile() - //profile.Close() - - //logger.Warn("Profile output", zap.String("Filename", profile.Name()))Also applies to: 110-113
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cli/auth/keys.go(2 hunks)cli/cli.go(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/auth/keys.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
cli/cli.go (2)
cli/ids/id.go (6)
Activity(12-12)Entity(13-13)Orchestration(14-14)IdKind(9-9)StateId(176-179)ParseStateId(118-130)cli/auth/keys.go (1)
CreateUser(117-143)
🔇 Additional comments (4)
cli/cli.go (4)
184-186: LGTM! Systematic package migration completed correctly.The migration from
gluepackage constants toidspackage constants is consistent and aligns with the broader refactor described in the PR. All references have been properly updated.Also applies to: 215-215, 220-220, 225-225, 232-232, 238-238
204-204: LGTM! Appropriate simplification of FrankenPHP options.The removal of the logger option and retention of CPU-based thread configuration is a reasonable simplification.
455-514: LGTM! Inspect command properly migrated to ids package.The migration of the inspect command from
gluetoidspackage maintains the same functionality while aligning with the broader package refactor.
533-534: LGTM! New CLI options properly defined.The addition of
rolesandclaimsoptions follows the established CLI patterns and provides appropriate configuration for enhanced user creation.
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
cli/lib/api.go (6)
357-382: Add missing return statements and fix success response.Several error responses are missing return statements, and the success response incorrectly uses
http.Error.r, err := rm.DiscoverResource(ctx, stateId, logger, true) if err != nil { logger.Error("Failed to discover resource", zap.Error(err)) http.Error(writer, "Not Found", http.StatusNotFound) + return } newUser := strings.TrimSpace(vars["userid"]) err = r.ShareOwnership(auth.UserId(newUser), auth.GetUserFromContext(ctx), true) if err != nil { logger.Error("Failed to share ownership", zap.Error(err)) http.Error(writer, "Internal Server Error", http.StatusInternalServerError) + return } err = r.Update(ctx, logger) if err != nil { logger.Error("Failed to update resource", zap.Error(err)) http.Error(writer, "Internal Server Error", http.StatusInternalServerError) + return } logger.Info("Shared ownership", zap.String("id", id.String()), zap.String("newUser", newUser)) -http.Error(writer, "", http.StatusOK) +writer.WriteHeader(http.StatusOK)
433-460: Add missing return statements and error messages.Missing return statements after error responses and empty error messages need to be fixed.
r, err := rm.DiscoverResource(ctx, stateId, logger, true) if err != nil { logger.Error("Failed to discover resource", zap.Error(err)) - http.Error(writer, "", http.StatusNotFound) + http.Error(writer, "Not Found", http.StatusNotFound) + return } switch vars["type"] { case "user": err = r.GrantUser(auth.UserId(vars["user"]), operation, ctx) case "role": err = r.GrantRole(auth.Role(vars["user"]), operation, ctx) } if err != nil { logger.Error("Failed to grant resource", zap.Error(err)) - http.Error(writer, "", http.StatusForbidden) + http.Error(writer, "Forbidden", http.StatusForbidden) + return } err = r.Update(ctx, logger) if err != nil { logger.Error("Failed to update resource", zap.Error(err)) - http.Error(writer, "", http.StatusInternalServerError) + http.Error(writer, "Internal Server Error", http.StatusInternalServerError) + return } -http.Error(writer, "", http.StatusOK) +writer.WriteHeader(http.StatusOK)
488-515: Fix missing return statements and error messages.This handler has the same pattern of missing returns and empty error messages.
r, err := rm.DiscoverResource(ctx, stateId, logger, true) if err != nil { logger.Error("Failed to discover resource", zap.Error(err)) - http.Error(writer, "", http.StatusNotFound) + http.Error(writer, "Not Found", http.StatusNotFound) + return } switch vars["type"] { case "user": err = r.RevokeUser(auth.UserId(vars["user"]), ctx) case "role": err = r.RevokeRole(auth.Role(vars["user"]), ctx) } if err != nil { logger.Error("Failed to revoke resource", zap.Error(err)) - http.Error(writer, "", http.StatusForbidden) + http.Error(writer, "Forbidden", http.StatusForbidden) + return } err = r.Update(ctx, logger) if err != nil { logger.Error("Failed to update resource", zap.Error(err)) - http.Error(writer, "", http.StatusInternalServerError) + http.Error(writer, "Internal Server Error", http.StatusInternalServerError) + return } -http.Error(writer, "", http.StatusOK) +writer.WriteHeader(http.StatusOK)
685-710: Add missing return statements and fix success response.Same pattern of missing returns as the entity share handler.
r, err := rm.DiscoverResource(ctx, stateId, logger, true) if err != nil { logger.Error("Failed to discover resource", zap.Error(err)) http.Error(writer, "Not Found", http.StatusNotFound) + return } newUser := strings.TrimSpace(vars["userid"]) err = r.ShareOwnership(auth.UserId(newUser), auth.GetUserFromContext(ctx), true) if err != nil { logger.Error("Failed to share ownership", zap.Error(err)) http.Error(writer, "Internal Server Error", http.StatusInternalServerError) + return } err = r.Update(ctx, logger) if err != nil { logger.Error("Failed to update resource", zap.Error(err)) http.Error(writer, "Internal Server Error", http.StatusInternalServerError) + return } logger.Info("Shared ownership", zap.String("id", id.String()), zap.String("newUser", newUser)) -http.Error(writer, "", http.StatusOK) +writer.WriteHeader(http.StatusOK)
761-788: Fix missing return statements and error messages.Same issues as the entity grant handler.
r, err := rm.DiscoverResource(ctx, stateId, logger, true) if err != nil { logger.Error("Failed to discover resource", zap.Error(err)) - http.Error(writer, "", http.StatusNotFound) + http.Error(writer, "Not Found", http.StatusNotFound) + return } switch vars["type"] { case "user": err = r.GrantUser(auth.UserId(vars["user"]), operation, ctx) case "role": err = r.GrantRole(auth.Role(vars["user"]), operation, ctx) } if err != nil { logger.Error("Failed to grant resource", zap.Error(err)) - http.Error(writer, "", http.StatusForbidden) + http.Error(writer, "Forbidden", http.StatusForbidden) + return } err = r.Update(ctx, logger) if err != nil { logger.Error("Failed to update resource", zap.Error(err)) - http.Error(writer, "", http.StatusInternalServerError) + http.Error(writer, "Internal Server Error", http.StatusInternalServerError) + return } -http.Error(writer, "", http.StatusOK) +writer.WriteHeader(http.StatusOK)
816-843: Fix missing return statements and error messages.Same issues as the entity revoke handler.
r, err := rm.DiscoverResource(ctx, stateId, logger, true) if err != nil { logger.Error("Failed to discover resource", zap.Error(err)) - http.Error(writer, "", http.StatusNotFound) + http.Error(writer, "Not Found", http.StatusNotFound) + return } switch vars["type"] { case "user": err = r.RevokeUser(auth.UserId(vars["user"]), ctx) case "role": err = r.RevokeRole(auth.Role(vars["user"]), ctx) } if err != nil { logger.Error("Failed to revoke resource", zap.Error(err)) - http.Error(writer, "", http.StatusForbidden) + http.Error(writer, "Forbidden", http.StatusForbidden) + return } err = r.Update(ctx, logger) if err != nil { logger.Error("Failed to update resource", zap.Error(err)) - http.Error(writer, "", http.StatusInternalServerError) + http.Error(writer, "Internal Server Error", http.StatusInternalServerError) + return } -http.Error(writer, "", http.StatusOK) +writer.WriteHeader(http.StatusOK)
🧹 Nitpick comments (1)
src/Proxy/SpyProxy.php (1)
101-125: Consider documenting why the$classparameter is unused.The
$classparameter appears to be unused but is likely required by the parent class contract. Consider adding a comment to clarify this for future maintainers.protected function preamble(ReflectionClass $class): string { + // $class parameter is required by parent contract but not used in this implementation return <<<'EOT'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Dockerfile(2 hunks)cli/lib/api.go(17 hunks)src/Proxy/SpyProxy.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile
🧰 Additional context used
🪛 PHPMD (2.15.0)
src/Proxy/SpyProxy.php
101-101: Avoid unused parameters such as '$class'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build-docker
- GitHub Check: build-osx (x86_64)
- GitHub Check: build-osx (arm64)
- GitHub Check: build-linux (amd64)
- GitHub Check: build-linux (arm64)
- GitHub Check: Unit Tests - PHP
🔇 Additional comments (1)
src/Proxy/SpyProxy.php (1)
33-36: Good fix for passing the$isHookparameter.The implementation now correctly passes the
$isHookparameter toimpureCall, maintaining consistency with other proxy implementations.
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
cli/build.sh (1)
114-116:-raceis passed via-ldflags; it has no effect there.
-raceis a compile-time flag forgo build, not a linker flag.
Because it is appended toextraLdflagsand injected into-ldflags, race detection is never enabled.- extraLdflags="-w -s -race" + extraLdflags="-w -s" + raceFlag="-race" # compile-timeThen include
"$raceFlag"next to-buildmodein thego buildinvocation (mind that static PIE + race is unsupported on some platforms, so keep it conditional).This guarantees that the data-race detector is really compiled in when expected.
🧹 Nitpick comments (1)
cli/build.sh (1)
118-120:go get durable_phpis deprecated in Go 1.24; prefergo mod downloadorgo install.
go getoutside a module context is now a no-op and emits a warning.
If the intent is to pre-fetch module dependencies use:go mod downloadIf the intent is to install a binary, use:
go install durable_php@latestAdjusting this keeps the build script future-proof with recent Go tool-chain behaviour.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/build.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-linux (arm64)
- GitHub Check: build-docker
- GitHub Check: build-linux (amd64)
- GitHub Check: build-osx (arm64)
- GitHub Check: Unit Tests - PHP
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v2 #141 +/- ##
============================================
+ Coverage 45.00% 46.26% +1.26%
- Complexity 1166 1248 +82
============================================
Files 94 99 +5
Lines 3273 3562 +289
============================================
+ Hits 1473 1648 +175
- Misses 1800 1914 +114 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Performance Metrics
|
Summary by CodeRabbit