Annotations and migration to PHP 8.1#101
Merged
Merged
Conversation
WalkthroughThis pull request upgrades the Penneo SDK to require PHP 8.1 minimum, adds comprehensive PHP type hints across public and internal APIs, integrates Psalm static analysis with baseline tracking, strengthens OAuth middleware token handling with defensive null checks, expands Document/SignatureLine/CaseFile APIs with explicit signatures, updates test fixtures for API key exchange scenarios, and documents the migration path in changelog and README alongside new developer tooling guidance for Psalm and PHPCS. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/OAuth/Tokens/PenneoTokens.php (1)
50-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
serialize()string-typed and fail fast on JSON encoding errors.
deserialize()has a strict type hint requiringstring(seedeserialize(string $tokens)), butserialize()currently returnsfalse|string. Ifjson_encodefails, this can passfalseto callers expecting a string, breaking the type contract. The codebase already handles JSON errors explicitly elsewhere (e.g.,Entity.php,ApiConnector.php), so add consistent error handling here.Use
JSON_THROW_ON_ERROR(supported in PHP 8.1+) to throw an exception on failure and enforce thestringreturn type.Proposed fix
/** - * `@return` false|string + * `@throws` \JsonException */ - public function serialize() + public function serialize(): string { - return json_encode([ + return json_encode([ 'accessToken' => $this->getAccessToken(), 'refreshToken' => $this->getRefreshToken(), 'accessTokenExpiresAt' => $this->getAccessTokenExpiresAt(), 'refreshTokenExpiresAt' => $this->getRefreshTokenExpiresAt(), - ]); + ], JSON_THROW_ON_ERROR); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OAuth/Tokens/PenneoTokens.php` around lines 50 - 60, serialize() currently can return false|string from json_encode which violates deserialize(string $tokens) expectations; update PenneoTokens::serialize to declare/ensure a string return and use json_encode with JSON_THROW_ON_ERROR so encoding failures throw an exception instead of returning false. Specifically, modify the serialize() method in class PenneoTokens to call json_encode([...], JSON_THROW_ON_ERROR), enforce a string return type, and let the thrown JsonException bubble (or rethrow a clearer exception) so callers never receive false.src/Validation.php (1)
41-45:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
getPdf()before returningstring.
base64_decode()can returnfalseon invalid input. With the: stringreturn type declared on line 41, an invalid PDF payload will raise aTypeErrorat runtime instead of allowing controlled error handling.Suggested fix
public function getPdf(): string { $data = parent::getAssets($this, 'pdf'); - return base64_decode($data[0]); + $decoded = base64_decode($data[0], true); + if ($decoded === false) { + throw new Exception('Penneo: Invalid PDF payload returned by API'); + } + + return $decoded; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Validation.php` around lines 41 - 45, getPdf() currently calls base64_decode on data from parent::getAssets and returns it as a string but base64_decode() can return false; update getPdf() to guard the result of base64_decode (and validate parent::getAssets returned a non-empty array) and handle failure explicitly instead of letting a TypeError occur—e.g., check that $data is an array with index 0, call base64_decode, if the result === false throw a meaningful exception (RuntimeException/InvalidArgumentException) describing invalid PDF payload so callers can catch it; reference the getPdf() method and parent::getAssets(...) call when making the change.src/CaseFile.php (1)
198-226:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn type mismatch: nullable properties with non-nullable return types.
getTitle()andgetMetaData()declarestringreturn types, but the underlying properties may benullbefore the entity is hydrated or if not set. This will cause aTypeErrorin PHP 8.x when returningnullfrom a method declared to returnstring.Consider either:
- Adding null checks with exceptions for required fields, or
- Changing return types to
?stringProposed fix (nullable approach)
-public function getTitle(): string +public function getTitle(): ?string { return $this->title; }-public function getMetaData(): string +public function getMetaData(): ?string { return $this->metaData; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/CaseFile.php` around lines 198 - 226, getTitle() and getMetaData() currently declare non-nullable string returns but the backing properties can be null; update their signatures to return ?string (and update setTitle($title) and setMetaData($meta) to accept ?string) or alternatively throw a clear exception if null is unacceptable—modify the methods getTitle(), getMetaData(), setTitle(), and setMetaData() accordingly so return/parameter types match the nullable property state (or add null checks that raise exceptions) to avoid PHP 8 TypeError.src/Document.php (1)
347-354:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing null/empty check before array access.
getLinkedEntities()may return an empty array. Accessing$documentTypes[0]directly will trigger an "undefined array key" warning/error in PHP 8.x when no document type is linked.Proposed fix
public function getDocumentType() { if ($this->id && !$this->documentType) { $documentTypes = parent::getLinkedEntities($this, DocumentType::class); - $this->documentType = $documentTypes[0]; + $this->documentType = $documentTypes[0] ?? null; } return $this->documentType; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Document.php` around lines 347 - 354, The getDocumentType method assumes parent::getLinkedEntities($this, DocumentType::class) returns a non-empty array and directly uses $documentTypes[0], which can throw an undefined array key error; update getDocumentType to check that $documentTypes is non-empty (e.g., is_array and !empty) before assigning $this->documentType from the first element, and handle the empty case by leaving $this->documentType null (or returning null) so callers safely get null when no DocumentType is linked.src/Signer.php (1)
71-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDocblock and return type mismatch; missing null check on array access.
The docblock says
@return SigningRequest(non-nullable) but the method signature declares?SigningRequest. This inconsistency may confuse IDE tooling and static analyzers.Line 80 accesses
$requests[0]directly without checking if the array is empty, which will cause an "undefined array key" warning in PHP 8.x.Proposed fix
/** - * `@return` SigningRequest + * `@return` SigningRequest|null */ public function getSigningRequest(): ?SigningRequest { if ($this->signingRequest) { return $this->signingRequest; } $requests = parent::getLinkedEntities($this, SigningRequest::class); - return $requests[0]; + return $requests[0] ?? null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Signer.php` around lines 71 - 81, Update the getSigningRequest method to make the docblock match the nullable signature and guard against empty arrays: change the docblock to "@return SigningRequest|null", and after calling parent::getLinkedEntities($this, SigningRequest::class) check if $requests is non-empty (e.g., empty() or count()>0) before returning $requests[0]; otherwise return null. Ensure you still prefer $this->signingRequest when present and keep references to getSigningRequest, SigningRequest, parent::getLinkedEntities and $this->signingRequest intact.
🧹 Nitpick comments (1)
tests/unit/OAuth/OAuthApiTest.php (1)
69-74: ⚡ Quick winAdd regression tests for the new OAuth guard clauses.
This keeps the
postApiKeyExchange()happy path green, but the new failure branches forgetTokens() === nulland emptygetApiSecret()still are not exercised. A small pair of tests here would lock in both the exception message and the “no HTTP call” behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/OAuth/OAuthApiTest.php` around lines 69 - 74, Add two unit tests inside the OAuthApiTest suite to exercise the new OAuth guard clauses: one where the mocked getTokens() returns null and one where getApiSecret() returns an empty string; for each test configure the relevant mocks (use the same pattern as testAPICallsUseCorrectHostname and the postApiKeyExchange setup), assert that the call to the API method under test throws the expected exception message (matching the guard clause) and verify that the HTTP client/transport mock is never invoked (no outbound HTTP call). Reference the existing testAPICallsUseCorrectHostname and the postApiKeyExchange(), getTokens(), and getApiSecret() mocks to locate where to add these cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ApiConnector.php`:
- Around line 162-167: Change the callServer signature and internal handling to
use the PSR-7 interface instead of the concrete Guzzle class: update the return
type of callServer(...) from ?Response to ?Psr\Http\Message\ResponseInterface,
accept and return ResponseInterface from Client::request(), and remove the
redundant instanceof Response check (and any Guzzle-specific type hints) around
the response since ResponseInterface exposes getHeader() and other needed
methods; ensure proper "use Psr\Http\Message\ResponseInterface" import is added.
In `@src/CaseFile.php`:
- Around line 228-246: getSendAt() and getExpireAt() currently construct a
\DateTime from possibly null epoch fields (sendAt/expireAt), producing
1970-01-01 when unset; change both getters to return ?\DateTime (nullable) and
return null when the underlying timestamp is null/empty, and update setSendAt()
and setExpireAt() to accept ?\DateTime (nullable) and set the internal
sendAt/expireAt to null when passed null (otherwise store getTimestamp()),
ensuring the property types/annotations are updated accordingly.
- Around line 403-411: getCreatedAt() and getSignIteration() are not null-safe:
they assume $created and $signIteration are always set which can cause TypeError
or invalid epoch when null; update getCreatedAt() to guard $this->created
(return null or throw a clear exception) and change its signature to return
?\DateTime (or keep \DateTime and throw) and update getSignIteration() to guard
$this->signIteration and change its signature to return ?int (or throw a
descriptive exception) so callers know how null is handled; reference the
methods getCreatedAt and getSignIteration and the properties $created and
$signIteration when making the changes and ensure any call sites are adjusted to
handle the nullable return or catch the exception.
In `@src/CustomerBranding.php`:
- Around line 134-140: The code in CustomerBranding attempts to resolve
$this->logo via self::findLinkedEntity($customer, Image::class, $this->imageId)
even when $this->imageId is null; add an early guard that returns null (or skips
the lookup) if $this->imageId is null before calling findLinkedEntity to avoid
unnecessary/invalid lookups—modify the block that checks $this->logo in the
CustomerBranding class to first check $this->imageId and only call
findLinkedEntity when it is non-null.
In `@src/Document.php`:
- Around line 284-303: These DateTime getters (getCreatedAt, getModifiedAt,
getCompletedAt) should guard against null timestamps: change their signatures to
return ?\DateTime and if the underlying property ($this->created,
$this->modified, $this->completed) is null return null, otherwise construct and
return new \DateTime('@' . $timestamp); follow the same nullable pattern used by
getSignedAt in SignatureLine to avoid returning the Unix epoch when timestamps
are missing.
---
Outside diff comments:
In `@src/CaseFile.php`:
- Around line 198-226: getTitle() and getMetaData() currently declare
non-nullable string returns but the backing properties can be null; update their
signatures to return ?string (and update setTitle($title) and setMetaData($meta)
to accept ?string) or alternatively throw a clear exception if null is
unacceptable—modify the methods getTitle(), getMetaData(), setTitle(), and
setMetaData() accordingly so return/parameter types match the nullable property
state (or add null checks that raise exceptions) to avoid PHP 8 TypeError.
In `@src/Document.php`:
- Around line 347-354: The getDocumentType method assumes
parent::getLinkedEntities($this, DocumentType::class) returns a non-empty array
and directly uses $documentTypes[0], which can throw an undefined array key
error; update getDocumentType to check that $documentTypes is non-empty (e.g.,
is_array and !empty) before assigning $this->documentType from the first
element, and handle the empty case by leaving $this->documentType null (or
returning null) so callers safely get null when no DocumentType is linked.
In `@src/OAuth/Tokens/PenneoTokens.php`:
- Around line 50-60: serialize() currently can return false|string from
json_encode which violates deserialize(string $tokens) expectations; update
PenneoTokens::serialize to declare/ensure a string return and use json_encode
with JSON_THROW_ON_ERROR so encoding failures throw an exception instead of
returning false. Specifically, modify the serialize() method in class
PenneoTokens to call json_encode([...], JSON_THROW_ON_ERROR), enforce a string
return type, and let the thrown JsonException bubble (or rethrow a clearer
exception) so callers never receive false.
In `@src/Signer.php`:
- Around line 71-81: Update the getSigningRequest method to make the docblock
match the nullable signature and guard against empty arrays: change the docblock
to "@return SigningRequest|null", and after calling
parent::getLinkedEntities($this, SigningRequest::class) check if $requests is
non-empty (e.g., empty() or count()>0) before returning $requests[0]; otherwise
return null. Ensure you still prefer $this->signingRequest when present and keep
references to getSigningRequest, SigningRequest, parent::getLinkedEntities and
$this->signingRequest intact.
In `@src/Validation.php`:
- Around line 41-45: getPdf() currently calls base64_decode on data from
parent::getAssets and returns it as a string but base64_decode() can return
false; update getPdf() to guard the result of base64_decode (and validate
parent::getAssets returned a non-empty array) and handle failure explicitly
instead of letting a TypeError occur—e.g., check that $data is an array with
index 0, call base64_decode, if the result === false throw a meaningful
exception (RuntimeException/InvalidArgumentException) describing invalid PDF
payload so callers can catch it; reference the getPdf() method and
parent::getAssets(...) call when making the change.
---
Nitpick comments:
In `@tests/unit/OAuth/OAuthApiTest.php`:
- Around line 69-74: Add two unit tests inside the OAuthApiTest suite to
exercise the new OAuth guard clauses: one where the mocked getTokens() returns
null and one where getApiSecret() returns an empty string; for each test
configure the relevant mocks (use the same pattern as
testAPICallsUseCorrectHostname and the postApiKeyExchange setup), assert that
the call to the API method under test throws the expected exception message
(matching the guard clause) and verify that the HTTP client/transport mock is
never invoked (no outbound HTTP call). Reference the existing
testAPICallsUseCorrectHostname and the postApiKeyExchange(), getTokens(), and
getApiSecret() mocks to locate where to add these cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: cfe711c7-ad36-42ad-b4ae-ca98e7aaeefe
📒 Files selected for processing (28)
.github/workflows/test.yamlCHANGELOG.mdREADME.mdcomposer.jsonpsalm-baseline.xmlpsalm.xmlsrc/ApiConnector.phpsrc/CaseFile.phpsrc/CopyRecipient.phpsrc/Customer.phpsrc/CustomerBranding.phpsrc/Document.phpsrc/Entity.phpsrc/Folder.phpsrc/LogEntry.phpsrc/Message.phpsrc/MessageTemplate.phpsrc/OAuth/ApiKeysMiddleware.phpsrc/OAuth/OAuthApi.phpsrc/OAuth/RefreshTokenMiddleware.phpsrc/OAuth/Tokens/PenneoTokens.phpsrc/OAuth/Tokens/PenneoTokensValidator.phpsrc/OAuth/Tokens/SessionTokenStorage.phpsrc/SignatureLine.phpsrc/Signer.phpsrc/SigningRequest.phpsrc/Validation.phptests/unit/OAuth/OAuthApiTest.php
Comment on lines
+162
to
+167
| public static function callServer( | ||
| string $url, | ||
| $data = null, | ||
| string $method = 'get', | ||
| array $options = array() | ||
| ): ?Response { |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Guzzle 7, what are the declared return types of GuzzleHttp\Client::request()andGuzzleHttp\Exception\RequestException::getResponse()—GuzzleHttp\Psr7\ResponseorPsr\Http\Message\ResponseInterface?
💡 Result:
In Guzzle 7, the declared return type of GuzzleHttp\Client::request is Psr\Http\Message\ResponseInterface [1][2]. The declared return type of GuzzleHttp\Exception\RequestException::getResponse is?Psr\Http\Message\ResponseInterface (nullable ResponseInterface) [3]. Guzzle uses PSR-7 interfaces throughout, not its concrete Psr7 classes in method signatures [4][5][6].
Citations:
- 1: https://github.com/guzzle/guzzle/blob/7.8.2/src/ClientInterface.php
- 2: https://github.com/guzzle/guzzle/blob/master/src/Client.php
- 3: https://github.com/guzzle/guzzle/blob/master/src/Exception/RequestException.php
- 4: https://docs.guzzlephp.org/en/6.5/psr7.html
- 5: https://docs.guzzlephp.org/en/stable
- 6: http://www.php-fig.org/psr/psr-7
🏁 Script executed:
cat -n src/ApiConnector.php | sed -n '155,200p'Repository: Penneo/sdk-php
Length of output: 1940
🏁 Script executed:
head -50 src/ApiConnector.phpRepository: Penneo/sdk-php
Length of output: 1382
🏁 Script executed:
rg "interface ResponseInterface" --type php -A 20Repository: Penneo/sdk-php
Length of output: 40
🌐 Web query:
Does PSR-7 ResponseInterface have getHeader() method? What methods are in ResponseInterface?
💡 Result:
Yes. PSR-7 ResponseInterface (Psr\Http\Message\ResponseInterface) has getHeader($name) method (inherited from MessageInterface) [1][2]. Methods in Psr\Http\Message\ResponseInterface - From MessageInterface: - getProtocolVersion [1][2] - withProtocolVersion($version) [2][1] - getHeaders [3][4] - hasHeader($name) [1][2] - getHeader($name): array [1][2] - getHeaderLine($name): string [2][1] - withHeader($name, $value) [2] - withAddedHeader($name, $value) [3][4] - withoutHeader($name) [3][4] - getBody [4] - withBody(StreamInterface $body) [4] - Response-specific: - getStatusCode [3][4] - withStatus($code, $reasonPhrase = '') [3][4] - getReasonPhrase [3][4]
Citations:
- 1: https://github.com/php-fig/http-message/blob/master/src/MessageInterface.php
- 2: https://www.php-fig.org/psr/psr-7/
- 3: https://github.com/php-fig/http-message/blob/master/docs/PSR7-Interfaces.md
- 4: https://phpdoc.moodledev.io/4.1/df/d72/interface_psr_1_1_http_1_1_message_1_1_response_interface.html
🏁 Script executed:
rg "Response" src/ApiConnector.phpRepository: Penneo/sdk-php
Length of output: 199
Return ResponseInterface, not the concrete Guzzle response class.
Client::request() is typed against PSR-7 ResponseInterface. Narrowing callServer() to ?Response hard-codes the implementation class and can trip a runtime TypeError if a different PSR-7 response object comes back through the handler stack. The instanceof Response check on line 189 is also unnecessary—ResponseInterface includes the getHeader() method.
Suggested fix
-use GuzzleHttp\Psr7\Response;
use GuzzleHttp\RequestOptions;
use Penneo\SDK\OAuth\Config\Environment;
use Penneo\SDK\OAuth\OAuth;
use Psr\Http\Message\RequestInterface;
+use Psr\Http\Message\ResponseInterface;
use Psr\Log\LoggerInterface;
use Psr\Log\NullLogger;
@@
public static function callServer(
string $url,
$data = null,
string $method = 'get',
array $options = array()
- ): ?Response {
+ ): ?ResponseInterface {
@@
- if ($response instanceof Response) {
- // some logging implementation might not print the context, we put the request id in the log message
- // because it is important and we want to make sure it gets seen
- self::$logger->debug(
- 'response requestId=' . implode('', $response->getHeader('X-Penneo-Request-Id')),
- ['method' => $method, 'url' => $url]
- );
- }
+ // some logging implementation might not print the context, we put the request id in the log message
+ // because it is important and we want to make sure it gets seen
+ self::$logger->debug(
+ 'response requestId=' . implode('', $response->getHeader('X-Penneo-Request-Id')),
+ ['method' => $method, 'url' => $url]
+ );
return $response;Also applies to: 189-197
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ApiConnector.php` around lines 162 - 167, Change the callServer signature
and internal handling to use the PSR-7 interface instead of the concrete Guzzle
class: update the return type of callServer(...) from ?Response to
?Psr\Http\Message\ResponseInterface, accept and return ResponseInterface from
Client::request(), and remove the redundant instanceof Response check (and any
Guzzle-specific type hints) around the response since ResponseInterface exposes
getHeader() and other needed methods; ensure proper "use
Psr\Http\Message\ResponseInterface" import is added.
Comment on lines
+228
to
246
| public function getSendAt(): \DateTime | ||
| { | ||
| return new \DateTime('@' . $this->sendAt); | ||
| } | ||
|
|
||
| public function setSendAt(\DateTime $sendAt) | ||
| public function setSendAt(\DateTime $sendAt): void | ||
| { | ||
| $this->sendAt = $sendAt->getTimestamp(); | ||
| } | ||
|
|
||
| public function getExpireAt() | ||
| public function getExpireAt(): \DateTime | ||
| { | ||
| return new \DateTime('@' . $this->expireAt); | ||
| } | ||
|
|
||
| public function setExpireAt(\DateTime $expireAt) | ||
| public function setExpireAt(\DateTime $expireAt): void | ||
| { | ||
| $this->expireAt = $expireAt->getTimestamp(); | ||
| } |
There was a problem hiding this comment.
DateTime getters may return misleading epoch dates when timestamps are null.
Similar to Document, getSendAt() and getExpireAt() will return 1970-01-01 when the underlying timestamp is null. For dates that may not be set (e.g., sendAt is optional), this behavior silently returns incorrect data.
Proposed fix
-public function getSendAt(): \DateTime
+public function getSendAt(): ?\DateTime
{
+ if ($this->sendAt === null) {
+ return null;
+ }
return new \DateTime('@' . $this->sendAt);
}
-public function getExpireAt(): \DateTime
+public function getExpireAt(): ?\DateTime
{
+ if ($this->expireAt === null) {
+ return null;
+ }
return new \DateTime('@' . $this->expireAt);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/CaseFile.php` around lines 228 - 246, getSendAt() and getExpireAt()
currently construct a \DateTime from possibly null epoch fields
(sendAt/expireAt), producing 1970-01-01 when unset; change both getters to
return ?\DateTime (nullable) and return null when the underlying timestamp is
null/empty, and update setSendAt() and setExpireAt() to accept ?\DateTime
(nullable) and set the internal sendAt/expireAt to null when passed null
(otherwise store getTimestamp()), ensuring the property types/annotations are
updated accordingly.
Comment on lines
+403
to
411
| public function getCreatedAt(): \DateTime | ||
| { | ||
| return new \DateTime('@' . $this->created); | ||
| } | ||
|
|
||
| public function getSignIteration() | ||
| public function getSignIteration(): int | ||
| { | ||
| return $this->signIteration; | ||
| } |
There was a problem hiding this comment.
Same null-safety concerns for getCreatedAt() and getSignIteration().
getCreatedAt() has the same epoch-date issue if $created is null. getSignIteration() returns int but the property may be null, causing a TypeError.
Proposed fix
-public function getCreatedAt(): \DateTime
+public function getCreatedAt(): ?\DateTime
{
+ if ($this->created === null) {
+ return null;
+ }
return new \DateTime('@' . $this->created);
}
-public function getSignIteration(): int
+public function getSignIteration(): ?int
{
return $this->signIteration;
}📝 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.
Suggested change
| public function getCreatedAt(): \DateTime | |
| { | |
| return new \DateTime('@' . $this->created); | |
| } | |
| public function getSignIteration() | |
| public function getSignIteration(): int | |
| { | |
| return $this->signIteration; | |
| } | |
| public function getCreatedAt(): ?\DateTime | |
| { | |
| if ($this->created === null) { | |
| return null; | |
| } | |
| return new \DateTime('@' . $this->created); | |
| } | |
| public function getSignIteration(): ?int | |
| { | |
| return $this->signIteration; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/CaseFile.php` around lines 403 - 411, getCreatedAt() and
getSignIteration() are not null-safe: they assume $created and $signIteration
are always set which can cause TypeError or invalid epoch when null; update
getCreatedAt() to guard $this->created (return null or throw a clear exception)
and change its signature to return ?\DateTime (or keep \DateTime and throw) and
update getSignIteration() to guard $this->signIteration and change its signature
to return ?int (or throw a descriptive exception) so callers know how null is
handled; reference the methods getCreatedAt and getSignIteration and the
properties $created and $signIteration when making the changes and ensure any
call sites are adjusted to handle the nullable return or catch the exception.
Comment on lines
134
to
+140
| if ($this->logo === null) { | ||
| // Fetch the logo url. | ||
| $this->logo = self::findLinkedEntity($this->customer, Image::class, $this->imageId); | ||
| $customer = $this->customer; | ||
| if ($customer === null) { | ||
| return null; | ||
| } | ||
| $this->logo = self::findLinkedEntity($customer, Image::class, $this->imageId); | ||
| } |
There was a problem hiding this comment.
Add an early null guard for imageId before linked lookup.
At Line 139, findLinkedEntity(...) is called even when $this->imageId is null. Adding a guard avoids unnecessary/invalid lookup work.
Suggested patch
if ($this->logo === null) {
+ if ($this->imageId === null) {
+ return null;
+ }
$customer = $this->customer;
if ($customer === null) {
return null;
}
$this->logo = self::findLinkedEntity($customer, Image::class, $this->imageId);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/CustomerBranding.php` around lines 134 - 140, The code in
CustomerBranding attempts to resolve $this->logo via
self::findLinkedEntity($customer, Image::class, $this->imageId) even when
$this->imageId is null; add an early guard that returns null (or skips the
lookup) if $this->imageId is null before calling findLinkedEntity to avoid
unnecessary/invalid lookups—modify the block that checks $this->logo in the
CustomerBranding class to first check $this->imageId and only call
findLinkedEntity when it is non-null.
Comment on lines
284
to
303
| public function getCreatedAt() | ||
| { | ||
| return new \Datetime('@' . $this->created); | ||
| return new \DateTime('@' . $this->created); | ||
| } | ||
|
|
||
| /** | ||
| * @return \DateTime | ||
| */ | ||
| public function getModifiedAt() | ||
| { | ||
| return new \Datetime('@' . $this->modified); | ||
| return new \DateTime('@' . $this->modified); | ||
| } | ||
|
|
||
| /** | ||
| * @return \DateTime | ||
| */ | ||
| public function getCompletedAt() | ||
| { | ||
| return new \Datetime('@' . $this->completed); | ||
| return new \DateTime('@' . $this->completed); | ||
| } |
There was a problem hiding this comment.
Potential null timestamp issue: DateTime methods may return misleading epoch date.
When $this->created, $this->modified, or $this->completed is null (e.g., document not yet created/completed), new \DateTime('@' . null) evaluates to new \DateTime('@') which creates a DateTime for Unix epoch (1970-01-01 00:00:00). This silently returns a misleading date instead of indicating absence.
Consider returning nullable ?\DateTime with a guard, consistent with getSignedAt() in SignatureLine:
Proposed fix for getCreatedAt (apply similar pattern to others)
/**
- * `@return` \DateTime
+ * `@return` \DateTime|null
*/
-public function getCreatedAt()
+public function getCreatedAt(): ?\DateTime
{
+ if ($this->created === null) {
+ return null;
+ }
return new \DateTime('@' . $this->created);
}📝 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.
Suggested change
| public function getCreatedAt() | |
| { | |
| return new \Datetime('@' . $this->created); | |
| return new \DateTime('@' . $this->created); | |
| } | |
| /** | |
| * @return \DateTime | |
| */ | |
| public function getModifiedAt() | |
| { | |
| return new \Datetime('@' . $this->modified); | |
| return new \DateTime('@' . $this->modified); | |
| } | |
| /** | |
| * @return \DateTime | |
| */ | |
| public function getCompletedAt() | |
| { | |
| return new \Datetime('@' . $this->completed); | |
| return new \DateTime('@' . $this->completed); | |
| } | |
| /** | |
| * `@return` \DateTime|null | |
| */ | |
| public function getCreatedAt(): ?\DateTime | |
| { | |
| if ($this->created === null) { | |
| return null; | |
| } | |
| return new \DateTime('@' . $this->created); | |
| } | |
| /** | |
| * `@return` \DateTime|null | |
| */ | |
| public function getModifiedAt(): ?\DateTime | |
| { | |
| if ($this->modified === null) { | |
| return null; | |
| } | |
| return new \DateTime('@' . $this->modified); | |
| } | |
| /** | |
| * `@return` \DateTime|null | |
| */ | |
| public function getCompletedAt(): ?\DateTime | |
| { | |
| if ($this->completed === null) { | |
| return null; | |
| } | |
| return new \DateTime('@' . $this->completed); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Document.php` around lines 284 - 303, These DateTime getters
(getCreatedAt, getModifiedAt, getCompletedAt) should guard against null
timestamps: change their signatures to return ?\DateTime and if the underlying
property ($this->created, $this->modified, $this->completed) is null return
null, otherwise construct and return new \DateTime('@' . $timestamp); follow the
same nullable pattern used by getSignedAt in SignatureLine to avoid returning
the Unix epoch when timestamps are missing.
yeu1
force-pushed
the
annotations
branch
2 times, most recently
from
95ed7ab to
18a8371
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Document.php (1)
113-119:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPopulate the
signatureLinescache before returning it.
getSignatureLines()fetches the collection but never assigns it to$this->signatureLines, so every call re-hits the API andfindSignatureLine()never benefits from the cached path.Suggested fix
public function getSignatureLines() { - if ($this->signatureLines !== null) { - return $this->signatureLines; - } - return parent::getLinkedEntities($this, SignatureLine::class); + if ($this->signatureLines === null) { + $this->signatureLines = parent::getLinkedEntities($this, SignatureLine::class); + } + + return $this->signatureLines; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Document.php` around lines 113 - 119, getSignatureLines currently returns parent::getLinkedEntities($this, SignatureLine::class) without storing it, so the $signatureLines cache is never populated; update getSignatureLines to call parent::getLinkedEntities($this, SignatureLine::class), assign the result to $this->signatureLines, and then return $this->signatureLines so subsequent calls (and findSignatureLine) use the cached collection.src/CaseFile.php (2)
203-255:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd type hints to setters to match the public API contract and prevent invalid internal state.
The setters for
setTitle(),setMetaData(), andsetVisibilityMode()accept untyped parameters while their corresponding getters promise to returnstring,string, andintrespectively. This allows invalid values to enter the entity, violating the type contract.Proposed fix
-public function setTitle($title): void +public function setTitle(string $title): void { $this->title = $title; } -public function setMetaData($meta): void +public function setMetaData(string $meta): void { $this->metaData = $meta; } -public function setVisibilityMode($visibilityMode): void +public function setVisibilityMode(int $visibilityMode): void { $this->visibilityMode = $visibilityMode; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/CaseFile.php` around lines 203 - 255, The setters setTitle, setMetaData and setVisibilityMode accept untyped parameters which lets invalid values into the object; update their signatures to enforce the public API types (setTitle(string $title): void, setMetaData(string $meta): void, setVisibilityMode(int $visibilityMode): void) and adjust any call sites/tests that pass other types; ensure the internal assignments remain the same and add/keep type-safe PHPDoc or strict typing if used in the class.
154-180:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse strict ID comparison in finder methods.
Lines 158 and 174 use loose
==comparison, which permits unintended matches across numeric-string edge cases (e.g.,5 == "05"evaluates totrue). Cast both operands to string and use strict equality to prevent this.Proposed fix
- if ($signer->getId() == $id) { + if ((string) $signer->getId() === (string) $id) { return $signer; } - if ($recipient->getId() == $id) { + if ((string) $recipient->getId() === (string) $id) { return $recipient; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/CaseFile.php` around lines 154 - 180, The finder methods findSigner and findCopyRecipient use loose equality (==) for ID comparison which allows unintended matches; update both methods so the comparison casts both operands to string and uses strict equality (===) when comparing $signer->getId() and $recipient->getId() to the $id parameter (keep the existing null-check logic and fallback to parent::findLinkedEntity(Signature/Class) intact).
🧹 Nitpick comments (2)
.github/workflows/test.yaml (1)
15-66: Consider dependency/cache optimization across CI jobs.You can likely shave meaningful runtime by caching Composer downloads (and optionally Docker build layers) across matrix runs, since each job currently rebuilds/install from scratch.
Also applies to: 67-120
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yaml around lines 15 - 66, CI jobs "format" and "psalm" rebuild Docker and re-run "composer install" every matrix job which wastes time; add caching for Composer and optional Docker layer cache: use GitHub Actions cache keyed by composer.lock (and matrix.php-version for psalm) to persist composer cache directories used by composer install, and add a Docker build cache solution (e.g., docker/build-push-action with cache-from/cache-to or actions/cache for ~/.cache/docker) so the docker build --build-arg "PHP_VERSION=…" in the Build PHP container step can reuse layers; update the steps in the "format" and "psalm" jobs (and other similar jobs) to restore cache before running composer install, save cache after, and wire docker build to use the cache to reduce redundant downloads and rebuilds.composer.json (1)
30-31: 💤 Low valueConsider version constraint flexibility for long-term maintenance.
The Psalm version is pinned exactly (
6.16.1), which ensures reproducibility but requires manual bumps for security patches. The guzzler constraint2.0.2|>=2.2skips versions 2.0.3–2.1.x—if this is intentional (e.g., avoiding a known issue), a brief inline comment would help future maintainers understand why.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@composer.json` around lines 30 - 31, The composer.json currently pins "vimeo/psalm" to an exact version and uses a non-obvious gap in "blastcloud/guzzler" ("2.0.2|>=2.2"); update the constraints to be more maintainable (for example change "vimeo/psalm" to a range like "^6.16" or ">=6.16 <7.0" to allow patch/minor security fixes) and either broaden or explicitly justify the guzzler constraint (e.g., allow ">=2.0.2 <3.0" or add an inline JSON comment/adjacent composer.json comment in your repo explaining why 2.0.3–2.1.x are excluded) so future maintainers understand the intent; modify the "vimeo/psalm" and "blastcloud/guzzler" entries accordingly in composer.json.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/OAuth/ApiKeysMiddleware.php`:
- Around line 28-38: The Authorization header is emitted even when
getAccessToken() returns an empty string; update the ApiKeysMiddleware logic to
validate the access token from
$this->tokenStorage->getTokens()->getAccessToken() and throw a
PenneoSdkRuntimeException (with a clear message like "Access token is empty;
complete OAuth/API key exchange") instead of returning a request with "Bearer "
when the token is empty or null; ensure you reference tokenStorage->getTokens(),
getAccessToken(), and the existing PenneoSdkRuntimeException class when adding
the guard.
---
Outside diff comments:
In `@src/CaseFile.php`:
- Around line 203-255: The setters setTitle, setMetaData and setVisibilityMode
accept untyped parameters which lets invalid values into the object; update
their signatures to enforce the public API types (setTitle(string $title): void,
setMetaData(string $meta): void, setVisibilityMode(int $visibilityMode): void)
and adjust any call sites/tests that pass other types; ensure the internal
assignments remain the same and add/keep type-safe PHPDoc or strict typing if
used in the class.
- Around line 154-180: The finder methods findSigner and findCopyRecipient use
loose equality (==) for ID comparison which allows unintended matches; update
both methods so the comparison casts both operands to string and uses strict
equality (===) when comparing $signer->getId() and $recipient->getId() to the
$id parameter (keep the existing null-check logic and fallback to
parent::findLinkedEntity(Signature/Class) intact).
In `@src/Document.php`:
- Around line 113-119: getSignatureLines currently returns
parent::getLinkedEntities($this, SignatureLine::class) without storing it, so
the $signatureLines cache is never populated; update getSignatureLines to call
parent::getLinkedEntities($this, SignatureLine::class), assign the result to
$this->signatureLines, and then return $this->signatureLines so subsequent calls
(and findSignatureLine) use the cached collection.
---
Nitpick comments:
In @.github/workflows/test.yaml:
- Around line 15-66: CI jobs "format" and "psalm" rebuild Docker and re-run
"composer install" every matrix job which wastes time; add caching for Composer
and optional Docker layer cache: use GitHub Actions cache keyed by composer.lock
(and matrix.php-version for psalm) to persist composer cache directories used by
composer install, and add a Docker build cache solution (e.g.,
docker/build-push-action with cache-from/cache-to or actions/cache for
~/.cache/docker) so the docker build --build-arg "PHP_VERSION=…" in the Build
PHP container step can reuse layers; update the steps in the "format" and
"psalm" jobs (and other similar jobs) to restore cache before running composer
install, save cache after, and wire docker build to use the cache to reduce
redundant downloads and rebuilds.
In `@composer.json`:
- Around line 30-31: The composer.json currently pins "vimeo/psalm" to an exact
version and uses a non-obvious gap in "blastcloud/guzzler" ("2.0.2|>=2.2");
update the constraints to be more maintainable (for example change "vimeo/psalm"
to a range like "^6.16" or ">=6.16 <7.0" to allow patch/minor security fixes)
and either broaden or explicitly justify the guzzler constraint (e.g., allow
">=2.0.2 <3.0" or add an inline JSON comment/adjacent composer.json comment in
your repo explaining why 2.0.3–2.1.x are excluded) so future maintainers
understand the intent; modify the "vimeo/psalm" and "blastcloud/guzzler" entries
accordingly in composer.json.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 01539c88-c23a-4981-a3c0-cf3eca08302f
📒 Files selected for processing (28)
.github/workflows/test.yamlCHANGELOG.mdREADME.mdcomposer.jsonpsalm-baseline.xmlpsalm.xmlsrc/ApiConnector.phpsrc/CaseFile.phpsrc/CopyRecipient.phpsrc/Customer.phpsrc/CustomerBranding.phpsrc/Document.phpsrc/Entity.phpsrc/Folder.phpsrc/LogEntry.phpsrc/Message.phpsrc/MessageTemplate.phpsrc/OAuth/ApiKeysMiddleware.phpsrc/OAuth/OAuthApi.phpsrc/OAuth/RefreshTokenMiddleware.phpsrc/OAuth/Tokens/PenneoTokens.phpsrc/OAuth/Tokens/PenneoTokensValidator.phpsrc/OAuth/Tokens/SessionTokenStorage.phpsrc/SignatureLine.phpsrc/Signer.phpsrc/SigningRequest.phpsrc/Validation.phptests/unit/OAuth/OAuthApiTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Message.php
- CHANGELOG.md
- src/Validation.php
Comment on lines
+28
to
38
| $tokens = $this->tokenStorage->getTokens(); | ||
| if ($tokens === null) { | ||
| throw new PenneoSdkRuntimeException( | ||
| 'OAuth tokens are not available. Complete an OAuth or API key exchange flow first.' | ||
| ); | ||
| } | ||
|
|
||
| return $request->withHeader( | ||
| 'Authorization', | ||
| "Bearer {$this->tokenStorage->getTokens()->getAccessToken()}" | ||
| "Bearer {$tokens->getAccessToken()}" | ||
| ); |
There was a problem hiding this comment.
Guard against empty access tokens before emitting the Authorization header.
Line 37 will currently send Bearer if getAccessToken() is empty, which causes avoidable downstream auth failures with a less actionable error path.
Suggested patch
$tokens = $this->tokenStorage->getTokens();
if ($tokens === null) {
throw new PenneoSdkRuntimeException(
'OAuth tokens are not available. Complete an OAuth or API key exchange flow first.'
);
}
+ if (empty($tokens->getAccessToken())) {
+ throw new PenneoSdkRuntimeException(
+ 'OAuth access token is empty. Complete an OAuth or API key exchange flow first.'
+ );
+ }
return $request->withHeader(
'Authorization',
"Bearer {$tokens->getAccessToken()}"
);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/OAuth/ApiKeysMiddleware.php` around lines 28 - 38, The Authorization
header is emitted even when getAccessToken() returns an empty string; update the
ApiKeysMiddleware logic to validate the access token from
$this->tokenStorage->getTokens()->getAccessToken() and throw a
PenneoSdkRuntimeException (with a clear message like "Access token is empty;
complete OAuth/API key exchange") instead of returning a request with "Bearer "
when the token is empty or null; ensure you reference tokenStorage->getTokens(),
getAccessToken(), and the existing PenneoSdkRuntimeException class when adding
the guard.
a848d89 to
808815a
Compare
Contributor
Author
|
@coderabbitai re-review |
|
Tip For best results, initiate chat on the files or code changes.
[full_review] |
yeu1
force-pushed
the
annotations
branch
2 times, most recently
from
a6daddd to
ab722b7
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Signer.php (1)
74-81:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocblock mismatch and missing bounds check in
getSigningRequest().
- The
@return SigningRequestdocblock conflicts with the actual?SigningRequestreturn type.- Accessing
$requests[0]without checking if$requestsis empty will produce an undefined offset warning when no signing requests exist.Proposed fix
/** - * `@return` SigningRequest + * `@return` SigningRequest|null */ public function getSigningRequest(): ?SigningRequest { if ($this->signingRequest) { return $this->signingRequest; } $requests = parent::getLinkedEntities($this, SigningRequest::class); - return $requests[0]; + return $requests[0] ?? null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Signer.php` around lines 74 - 81, The getSigningRequest() method has a docblock/return-type mismatch and an unsafe array access; update the docblock to match the nullable return (?SigningRequest) and add a bounds check before returning $requests[0] in getSigningRequest(): call parent::getLinkedEntities($this, SigningRequest::class), verify the returned $requests is non-empty (and that its first element is a SigningRequest), and return that element or null; keep the existing $this->signingRequest short-circuit behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/Signer.php`:
- Around line 74-81: The getSigningRequest() method has a docblock/return-type
mismatch and an unsafe array access; update the docblock to match the nullable
return (?SigningRequest) and add a bounds check before returning $requests[0] in
getSigningRequest(): call parent::getLinkedEntities($this,
SigningRequest::class), verify the returned $requests is non-empty (and that its
first element is a SigningRequest), and return that element or null; keep the
existing $this->signingRequest short-circuit behavior intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 83d9be5a-8d7a-4bc4-a8fc-0c82865e8268
📒 Files selected for processing (31)
.github/workflows/test.yamlCHANGELOG.mdREADME.mdcomposer.jsondocs/cli-examples.mddocs/programmatic_oauth_example.phppsalm-baseline.xmlpsalm.xmlsrc/ApiConnector.phpsrc/CaseFile.phpsrc/CopyRecipient.phpsrc/Customer.phpsrc/CustomerBranding.phpsrc/Document.phpsrc/Entity.phpsrc/Folder.phpsrc/LogEntry.phpsrc/Message.phpsrc/MessageTemplate.phpsrc/OAuth/ApiKeysMiddleware.phpsrc/OAuth/OAuthApi.phpsrc/OAuth/RefreshTokenMiddleware.phpsrc/OAuth/Tokens/PenneoTokens.phpsrc/OAuth/Tokens/PenneoTokensValidator.phpsrc/OAuth/Tokens/SessionTokenStorage.phpsrc/SignatureLine.phpsrc/Signer.phpsrc/SigningRequest.phpsrc/Validation.phpsrc/WebhookSubscription.phptests/unit/OAuth/OAuthApiTest.php
✅ Files skipped from review due to trivial changes (13)
- src/WebhookSubscription.php
- src/OAuth/Tokens/SessionTokenStorage.php
- src/Message.php
- src/LogEntry.php
- src/CopyRecipient.php
- psalm-baseline.xml
- src/OAuth/Tokens/PenneoTokensValidator.php
- src/Folder.php
- psalm.xml
- src/MessageTemplate.php
- composer.json
- docs/cli-examples.md
- src/Validation.php
🚧 Files skipped from review as they are similar to previous changes (9)
- src/Customer.php
- src/OAuth/OAuthApi.php
- src/OAuth/RefreshTokenMiddleware.php
- .github/workflows/test.yaml
- src/OAuth/Tokens/PenneoTokens.php
- src/CustomerBranding.php
- src/SignatureLine.php
- src/SigningRequest.php
- src/Document.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.