Open
Conversation
The default authentication type is "basic", but some CalDav servers have other authentication types (like "digest", in my case).
When creating a calendar you can provide a resource name in the MakeCalendarRequestVO, but this resource name was ignored when doing the actual call to the server. This commit checks if you have provided a resource name, and will try to create a calendar using that resource name if you did. If you did not provide a resource name, it will revert to the previous behaviour of using a UID for the calendar name.
It accepts a 'body' and (return) 'code' argument, but in the code here 'calendar_url' was erroneously passed in as first argument
In the GetCalendarRequest and GetCalendarsRequest you use the {http://calendarserver.org/ns/:} namespace, but that one was not declared at the top of the XML document.
Prevents SabreDAV from recognizing the request for a ctag
Allow authentication types other than 'basic'
Implement missing date range filter
No longer ignore user provided 'resourcename'
CalendarDeletedResponse accepts 2 arguments, not 3
Add omitted namespace
Some CalDAV servers only return namespaced XML, and simplexml_load_string deals with this kind of XML slightly differently, which means we can’t easily convert it to JSON without first stripping the namespaces (unfortunately).
Without this change the library doesn’t work in SabreDAV
The tests were very tailored for iCloud. I adjusted them to run on more generic CalDav servers as well.
To access the requested getetag prop from Facade/Requests/GetCalendarRequest.php
Accomodated for status being nested in propstat element in Apple ICalendar CalendarSyncInfoResponse ETagEntityResponse body
Yahoo returns the HTTP statuses in lowercase, which doesnt match with the uppercase HTTP statuses.
This commit consists of patches automatically generated for this project on https://scrutinizer-ci.com
- Add getSupportedComponents() method to parse component types - Add supportsEvents() helper to check VEVENT support - Enables filtering of VTODO-only calendars from event views
- Add current-user-privilege-set to GetCalendarsRequest - Add getCurrentUserPrivileges() method to parse user permissions - Add isWritable() helper to check write access - Enables UI indicators for read-only calendars
Similar to X-MASTER-RRULE, this captures the original DTSTART from the series master event before expansion and attaches it to each expanded instance. This allows frontends to correctly update recurring event series without accidentally shifting the series start date to the instance date being edited.
Add WebDAV MOVE method support to enable atomic event moves between CalDAV calendars. This is especially important for iCloud which uses account-unique UIDs and fails with DELETE+CREATE approach. - Add Move constant to HttpMethods - Add Destination and Overwrite headers to Headers - Add createMoveRequest to RequestFactory - Add EventMovedResponse class - Add moveEvent method to CalDavClient
…rnings Sabre VObject's __get returns null when no VEVENT components exist in the queried date range. Added `?? []` null coalesce on the foreach loop in expandWithRRulePreservation() to prevent "foreach() argument must be of type array|object, null given" PHP warnings. Updated inline comment and docstring to document the null case. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR introduces extensive CalDAV library enhancements including configurable authentication schemes (basic, digest, NTLM), event creation/update/move operations from raw iCalendar content with proper ETag handling, RFC 5545-compliant VALARM parsing with trigger normalization, recurring event expansion preserving RRULE metadata, and expanded calendar metadata accessors for permission checks and resource introspection, alongside configuration updates and comprehensive test coverage. Changes
Sequence DiagramsequenceDiagram
actor Client
participant CalDavClient
participant RequestFactory
participant WebDAV Server
participant Response Classes
Client->>CalDavClient: createEventFromICS(url, uid, ics_content)
CalDavClient->>RequestFactory: createPutRequest(url, ics_content, etag)
RequestFactory-->>CalDavClient: Request with Content-Length, Content-Type, If-Match headers
CalDavClient->>WebDAV Server: PUT request with iCalendar payload
WebDAV Server-->>CalDavClient: 201 Created + ETag + Location
CalDavClient->>Response Classes: new EventCreatedResponse(uid, etag, resource_url, body, 201)
Response Classes-->>CalDavClient: EventCreatedResponse object
CalDavClient-->>Client: EventCreatedResponse
Client->>CalDavClient: moveEvent(source_url, dest_url, uid, etag)
CalDavClient->>RequestFactory: createMoveRequest(source_url, dest_url, etag)
RequestFactory-->>CalDavClient: Request with Destination, Overwrite, If-Match headers
CalDavClient->>WebDAV Server: MOVE request
WebDAV Server-->>CalDavClient: 204 No Content + ETag
CalDavClient->>Response Classes: new EventMovedResponse(uid, etag, resource_url, body, 204)
Response Classes-->>CalDavClient: EventMovedResponse object (isSuccessFull() = true)
CalDavClient-->>Client: EventMovedResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The diff spans 25+ files with heterogeneous modifications including new exception classes, multiple response class enhancements with business logic (VALARM parsing, RRULE expansion), refactored authentication handling, request factory extensions, parameter order changes requiring cross-file verification, XML namespace corrections, and extensive test infrastructure. While many changes follow consistent patterns, the density of interconnected logic changes (particularly in Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Facade/Utils/RequestFactory.php (1)
29-80:⚠️ Potential issue | 🟠 Major
createPostRequest()currently drops all CalDAV headers.
createHeadersFor()has noHttpMethods::Postbranch, so this path falls through to[]. POST requests therefore miss at leastContent-Type: text/calendarand anyIf-Match, which is enough to break servers that validate calendar payload type or conditional updates.🛠️ Suggested fix
- case HttpMethods::Put: + case HttpMethods::Put: + case HttpMethods::Post: - $len = $params[0]; - $etag = $params[1]; + $len = $params[0]; + $etag = $params[1] ?? null; $headers = [ Headers::ContentLength => intval($len), Headers::ContentType => ContentTypes::Calendar, ]; @@ public static function createPostRequest($url, $body, $etag = null){ return new Request ( HttpMethods::Post, $url, - self::createHeadersFor(HttpMethods::Post, [$etag]), + self::createHeadersFor(HttpMethods::Post, [strlen($body), $etag]), $body ); }Also applies to: 194-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Facade/Utils/RequestFactory.php` around lines 29 - 80, createHeadersFor() lacks a branch for HttpMethods::Post so POST requests drop CalDAV headers; add a case for HttpMethods::Post (and ensure createPostRequest() uses it) that mirrors the Put behavior: extract $len = $params[0] and $etag = $params[1] (or accept nulls), include Headers::ContentType => ContentTypes::Calendar, set Headers::ContentLength => intval($len) when provided, and set Headers::IfMatch => $etag when non-empty; also verify the other occurrence referenced (lines ~194-200) uses the same header logic.
🧹 Nitpick comments (8)
src/Facade/Requests/AbstractPropFindWebDAVRequest.php (1)
33-37: Explicitly map the Apple iCal namespace in the namespace map.
GetCalendarsRequestrequests{http://apple.com/ns/ical/}calendar-colorbutAbstractPropFindWebDAVRequestdoes not include this namespace in itsnamespaceMap. While Sabre XML auto-generates prefixes for unmapped namespaces, explicit mapping improves readability and determinism by using a consistent prefix instead of auto-generated names like 'x1'.Suggested diff
$service->namespaceMap = [ 'DAV:' => 'D', 'urn:ietf:params:xml:ns:caldav' => 'C', 'http://calendarserver.org/ns/' => 'CS', + 'http://apple.com/ns/ical/' => 'A', ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Facade/Requests/AbstractPropFindWebDAVRequest.php` around lines 33 - 37, Add an explicit mapping for the Apple iCal namespace in AbstractPropFindWebDAVRequest by extending the $service->namespaceMap to include the URI "http://apple.com/ns/ical/" mapped to a stable prefix (e.g., 'IC' or 'ICAL'); this ensures GetCalendarsRequest's use of {http://apple.com/ns/ical/}calendar-color is serialized with a consistent prefix instead of an autogenerated one.src/Facade/Responses/UserPrincipalResponse.php (1)
22-27: Consider more specific return type in docblock.The method returns
UserPrincipalSingleResponse, which providesgetPrincipalUrl(). Using the specific type in the docblock would improve IDE autocompletion and type safety for callers.Proposed improvement
/** - * `@return` GenericSinglePROPFINDCalDAVResponse + * `@return` UserPrincipalSingleResponse */ protected function buildSingleResponse()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Facade/Responses/UserPrincipalResponse.php` around lines 22 - 27, The docblock for buildSingleResponse() currently declares a generic return type; update it to the concrete type returned so callers/IDEs see UserPrincipalSingleResponse methods. Change the `@return` annotation on buildSingleResponse() from GenericSinglePROPFINDCalDAVResponse to UserPrincipalSingleResponse, keeping the method body that returns new UserPrincipalSingleResponse() unchanged.src/Facade/Responses/UserPrincipalSingleResponse.php (1)
27-30: Document the iCloud quirk more explicitly.The fallback to
not_found_propsis an unusual pattern. Consider expanding the comment to clarify the specific iCloud behavior this addresses, which will help future maintainers understand why properties reported as "not found" might still contain valid data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Facade/Responses/UserPrincipalSingleResponse.php` around lines 27 - 30, Expand the inline comment above the fallback block to explicitly document the iCloud CalDAV quirk: explain that some iCloud responses list properties as "not found" in $this->not_found_props yet still include usable href values (specifically 'current-user-principal'), so we fall back to concatenating $this->server_url with $this->not_found_props['current-user-principal']['href']; mention that this guards against iCloud returning the principal href inside the not-found section and keep the reference to $url, $this->not_found_props and 'current-user-principal' so maintainers understand why the unusual fallback exists.tests/AllDayEventTest.php (1)
8-20: This is only an API-surface smoke test.
method_exists()won't catch broken ICS serialization, request construction, or incorrect response handling. Either exercise the local request-building path with a mock/stub or rename the test so it doesn't claim all-day coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/AllDayEventTest.php` around lines 8 - 20, The test AllDayEventTest::testCreateAllDayEvent only asserts API surface with method_exists and doesn't verify ICS serialization, request construction, or response handling; update the test to either (A) exercise the local request-building path by instantiating CalDavClient with a mocked HTTP layer and call createEventFromICS/updateEventFromICS with a representative all-day ICS payload asserting the generated request/headers/body and expected handling, referencing CalDavClient::createEventFromICS and CalDavClient::updateEventFromICS, or (B) if you want to keep it as a smoke/API-surface test, rename the test (e.g. testApiSurfaceMethodsExist) and update the test description to not claim all-day behavior so it no longer misleadingly implies full end-to-end coverage.tests/ResponseMethodsTest.php (1)
10-32: Assert response semantics, not method names.These cases can already verify
getCode()andisSuccessFull()directly, butmethod_exists()only proves the symbols are present. The tests will still pass if the success logic regresses.🧪 Stronger assertion pattern
public function testEventCreatedResponseHasCorrectMethods() { $response = new EventCreatedResponse('uid', 'etag', 'url', '', 201); - $this->assertTrue(method_exists($response, 'isSuccessFull')); - $this->assertTrue(method_exists($response, 'getCode')); + $this->assertSame(201, $response->getCode()); + $this->assertTrue($response->isSuccessFull()); $this->assertFalse(method_exists($response, 'getStatusCode')); }Apply the same pattern to the updated and deleted response tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ResponseMethodsTest.php` around lines 10 - 32, Replace assertions that only check for symbol existence with behavioral assertions: call EventCreatedResponse->isSuccessFull() and assertTrue, and assertEquals(201, EventCreatedResponse->getCode()) (remove method_exists checks including getStatusCode absence); likewise call EventUpdatedResponse->isSuccessFull() and assertEquals(204, EventUpdatedResponse->getCode()); and call EventDeletedResponse->isSuccessFull() and assertEquals(204, EventDeletedResponse->getCode()). Use the class names EventCreatedResponse, EventUpdatedResponse, EventDeletedResponse and the methods isSuccessFull() and getCode() to verify semantics rather than method_exists().src/Facade/Responses/GetCalendarResponse.php (1)
84-95: Remove unused loop variable$key.The
$keyvariable is never used inside the loop body.♻️ Suggested fix
- elseif (is_array($componentSet)) { - foreach ($componentSet as $key => $comp) { + elseif (is_array($componentSet)) { + foreach ($componentSet as $comp) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Facade/Responses/GetCalendarResponse.php` around lines 84 - 95, The foreach loop over $componentSet in GetCalendarResponse.php declares an unused $key variable; update the loop to remove the unused variable by iterating as foreach ($componentSet as $comp) (or foreach ($componentSet as $_ => $comp>) if you prefer an explicit unused key) so only $comp is declared and used in the body where $comp['@attributes']['name'] and $comp['name'] are checked.tests/FacadeTest.php (1)
222-236: UnusedgetCalendarresult and weak assertion.The
getCalendar()call on line 227 assigns to$resbut is immediately overwritten on line 229. Additionally,!empty($res)will always be true since$resis aCalendarDeletedResponseobject.♻️ Suggested fix
function testDeleteCalendar(){ $host = getenv("CALDAV_SERVER_HOST"); $calendar_url = $host . (str_replace($host, "", self::$calendar_created_by_phpunit)); - $res = self::$client->getCalendar($calendar_url); - $res = self::$client->deleteCalendar ( $calendar_url, "" ); - $this->assertTrue(!empty($res)); + $this->assertTrue($res->isSuccessFull()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/FacadeTest.php` around lines 222 - 236, The testDeleteCalendar contains a redundant call to getCalendar whose result in $res is immediately overwritten and the final assertion uses !empty($res) which is ineffective because deleteCalendar returns a CalendarDeletedResponse object; remove the unused getCalendar() call (or store it in a separate variable if you intended to assert pre-delete state) and replace the weak assertion with a stronger check against the delete response from deleteCalendar, e.g. assert that the returned object is an instance of CalendarDeletedResponse and/or assert a specific success property or status (use the deleteCalendar return value and methods on CalendarDeletedResponse to make the assertion).src/Facade/CalDavClient.php (1)
179-185: Consider handling unknown authentication types.Unknown auth types silently result in no authentication being configured, which could lead to confusing 401 errors if a user mistypes the auth type (e.g.,
"basci"instead of"basic").♻️ Optional: Add fallback handling
switch (strtolower(trim($this->authtype))) { case "basic": case "digest": case "ntlm": $options['auth'] = [$this->user, $this->password, $this->authtype]; break; + default: + // No authentication - could log a warning if authtype was set but unrecognized + break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Facade/CalDavClient.php` around lines 179 - 185, The switch on strtolower(trim($this->authtype)) in the CalDavClient class currently ignores unknown auth types; add a default branch after the existing cases to handle invalid values of $this->authtype (e.g., log a warning via the client logger or throw an InvalidArgumentException) so mis-typed values (like "basci") don't silently skip setting $options['auth']; ensure the message includes the invalid $this->authtype and that $options['auth'] is only set for the accepted cases ("basic","digest","ntlm").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 3: Change the changelog heading "## Unreleased (2025-01-13)" to remove
the date and leave it as "## Unreleased" so the unreleased section remains
undated until you tag/version a release; update the heading string in
CHANGELOG.md (look for the exact heading "## Unreleased (2025-01-13)") and
replace it with "## Unreleased".
In `@composer.json`:
- Around line 20-24: Add an explicit PHP platform constraint to composer.json's
"require" section: add "php": "^7.3" so Composer enforces a minimum PHP version
compatible with eluceo/ical ^0.15, phpunit/phpunit ^9.5, and sabre/vobject ^4.2;
update the "require" object (near the existing package entries) to include the
php constraint and then run composer update/validate to ensure the new
constraint is applied.
In `@src/Facade/Responses/AbstractCalDAVResponse.php`:
- Around line 79-92: The method stripNamespacesFromTags calls
simplexml_load_string($xml) and immediately dereferences it with
getNamespaces(), which can fatal if simplexml_load_string returns false; update
stripNamespacesFromTags to guard the parser result by checking that $obj is not
false (or instanceof SimpleXMLElement) before calling getNamespaces(), and if
parsing failed throw or return a clear parse error (consistent with existing
exception handling) so callers of stripNamespacesFromTags receive a handled
parse error instead of a fatal; ensure references to simplexml_load_string and
getNamespaces in stripNamespacesFromTags are the only changes.
In `@src/Facade/Responses/CalendarSyncInfoResponse.php`:
- Around line 42-43: The current loop appends $entity into $res and $deleted
regardless of type; ensure you only append when $entity is an instance of
ETagEntityResponse: check instanceof ETagEntityResponse before pushing into $res
or $deleted, and keep the existing status check (compare to
HttpResponse::HttpOKStatus) for $res; update the branches that write to $res and
$deleted (referencing the $entity variable, ETagEntityResponse,
HttpResponse::HttpOKStatus, and the $res/$deleted arrays) so
non-ETagEntityResponse objects are skipped and cannot leak into the returned
collections.
In `@src/Facade/Responses/GenericSinglePROPFINDCalDAVResponse.php`:
- Around line 49-56: The properties found_props and not_found_props (in class
GenericSinglePROPFINDCalDAVResponse) are documented as arrays but are being set
to null when the other status branch is taken; change those assignments so they
remain arrays (e.g., set to an empty array) instead of null in the branches that
call statusMatches with AbstractCalDAVResponse::HttpOKStatus and
::HttpNotFoundStatus so static analysis no longer reports nullable types.
In `@src/Facade/Responses/GetCalendarsResponse.php`:
- Around line 38-39: The code in getResponseByType() uses array_keys() on
$resource_types without checking if $resource_types is null, which can cause a
runtime error. Fix this by first verifying $resource_types is a non-null array
before calling array_keys(), for example by adding a guard condition to ensure
$resource_types is not null in the filter logic that includes the in_array()
check.
In `@src/Facade/Responses/UserPrincipalResponse.php`:
- Around line 19-28: The docblock for method buildSingleResponse() in class
UserPrincipalResponse incorrectly states `@return`
GenericSinglePROPFINDCalDAVResponse; update that docblock to reflect the actual
return type by changing it to `@return` UserPrincipalSingleResponse so the
annotation matches the method's returned object (UserPrincipalSingleResponse).
In `@src/Facade/Responses/UserPrincipalSingleResponse.php`:
- Around line 21-31: The docblock and signature for getPrincipalUrl()
incorrectly state it returns string while it can return null; update the
documentation and signature to reflect a nullable return by changing the
docblock to "@return string|null" and adding a nullable return type (e.g.
?string) to the getPrincipalUrl method declaration, ensuring references to
$this->found_props, $this->not_found_props and $this->server_url remain
unchanged.
In `@src/Facade/Responses/VCardEntityResponse.php`:
- Around line 21-29: The getVCard method in VCardEntityResponse can return null
but its docblock says "@return string"; update the docblock to reflect a
nullable return (e.g., "@return string|null") and, if desired, add a nullable
return type in the method signature (public function getVCard(): ?string) to
make the contract explicit; ensure references to getVCard and the
found_props['calendar-data'] checks remain unchanged.
In `@src/Facade/Utils/Headers.php`:
- Line 26: The constant IfNotMatch in Headers.php is malformed (it contains a
trailing double-quote) causing callers to send the wrong header name; update the
Headers::IfNotMatch constant value to the correct header string "If-None-Match"
(remove the extraneous quote) so any code referencing Headers::IfNotMatch will
send the proper header name.
In `@tests/CalDavClientTest.php`:
- Around line 23-36: The test currently relies on CalDavClient::getCredentials
returning the raw password; change the public API so getCredentials (or a new
public method such as getAuthHeader) does not expose the plaintext secret: have
CalDavClient::getCredentials return only the 'user' and an 'authorization' value
(e.g., an HTTP Basic Authorization header or a token) or simply omit the
password and add a getAuthHeader() method that builds the header from the stored
secret internally; update any consumers/tests to assert the presence of the
authorization/header value (or absence of a raw 'password' key) instead of
checking the plaintext password.
---
Outside diff comments:
In `@src/Facade/Utils/RequestFactory.php`:
- Around line 29-80: createHeadersFor() lacks a branch for HttpMethods::Post so
POST requests drop CalDAV headers; add a case for HttpMethods::Post (and ensure
createPostRequest() uses it) that mirrors the Put behavior: extract $len =
$params[0] and $etag = $params[1] (or accept nulls), include
Headers::ContentType => ContentTypes::Calendar, set Headers::ContentLength =>
intval($len) when provided, and set Headers::IfMatch => $etag when non-empty;
also verify the other occurrence referenced (lines ~194-200) uses the same
header logic.
---
Nitpick comments:
In `@src/Facade/CalDavClient.php`:
- Around line 179-185: The switch on strtolower(trim($this->authtype)) in the
CalDavClient class currently ignores unknown auth types; add a default branch
after the existing cases to handle invalid values of $this->authtype (e.g., log
a warning via the client logger or throw an InvalidArgumentException) so
mis-typed values (like "basci") don't silently skip setting $options['auth'];
ensure the message includes the invalid $this->authtype and that
$options['auth'] is only set for the accepted cases ("basic","digest","ntlm").
In `@src/Facade/Requests/AbstractPropFindWebDAVRequest.php`:
- Around line 33-37: Add an explicit mapping for the Apple iCal namespace in
AbstractPropFindWebDAVRequest by extending the $service->namespaceMap to include
the URI "http://apple.com/ns/ical/" mapped to a stable prefix (e.g., 'IC' or
'ICAL'); this ensures GetCalendarsRequest's use of
{http://apple.com/ns/ical/}calendar-color is serialized with a consistent prefix
instead of an autogenerated one.
In `@src/Facade/Responses/GetCalendarResponse.php`:
- Around line 84-95: The foreach loop over $componentSet in
GetCalendarResponse.php declares an unused $key variable; update the loop to
remove the unused variable by iterating as foreach ($componentSet as $comp) (or
foreach ($componentSet as $_ => $comp>) if you prefer an explicit unused key) so
only $comp is declared and used in the body where $comp['@attributes']['name']
and $comp['name'] are checked.
In `@src/Facade/Responses/UserPrincipalResponse.php`:
- Around line 22-27: The docblock for buildSingleResponse() currently declares a
generic return type; update it to the concrete type returned so callers/IDEs see
UserPrincipalSingleResponse methods. Change the `@return` annotation on
buildSingleResponse() from GenericSinglePROPFINDCalDAVResponse to
UserPrincipalSingleResponse, keeping the method body that returns new
UserPrincipalSingleResponse() unchanged.
In `@src/Facade/Responses/UserPrincipalSingleResponse.php`:
- Around line 27-30: Expand the inline comment above the fallback block to
explicitly document the iCloud CalDAV quirk: explain that some iCloud responses
list properties as "not found" in $this->not_found_props yet still include
usable href values (specifically 'current-user-principal'), so we fall back to
concatenating $this->server_url with
$this->not_found_props['current-user-principal']['href']; mention that this
guards against iCloud returning the principal href inside the not-found section
and keep the reference to $url, $this->not_found_props and
'current-user-principal' so maintainers understand why the unusual fallback
exists.
In `@tests/AllDayEventTest.php`:
- Around line 8-20: The test AllDayEventTest::testCreateAllDayEvent only asserts
API surface with method_exists and doesn't verify ICS serialization, request
construction, or response handling; update the test to either (A) exercise the
local request-building path by instantiating CalDavClient with a mocked HTTP
layer and call createEventFromICS/updateEventFromICS with a representative
all-day ICS payload asserting the generated request/headers/body and expected
handling, referencing CalDavClient::createEventFromICS and
CalDavClient::updateEventFromICS, or (B) if you want to keep it as a
smoke/API-surface test, rename the test (e.g. testApiSurfaceMethodsExist) and
update the test description to not claim all-day behavior so it no longer
misleadingly implies full end-to-end coverage.
In `@tests/FacadeTest.php`:
- Around line 222-236: The testDeleteCalendar contains a redundant call to
getCalendar whose result in $res is immediately overwritten and the final
assertion uses !empty($res) which is ineffective because deleteCalendar returns
a CalendarDeletedResponse object; remove the unused getCalendar() call (or store
it in a separate variable if you intended to assert pre-delete state) and
replace the weak assertion with a stronger check against the delete response
from deleteCalendar, e.g. assert that the returned object is an instance of
CalendarDeletedResponse and/or assert a specific success property or status (use
the deleteCalendar return value and methods on CalendarDeletedResponse to make
the assertion).
In `@tests/ResponseMethodsTest.php`:
- Around line 10-32: Replace assertions that only check for symbol existence
with behavioral assertions: call EventCreatedResponse->isSuccessFull() and
assertTrue, and assertEquals(201, EventCreatedResponse->getCode()) (remove
method_exists checks including getStatusCode absence); likewise call
EventUpdatedResponse->isSuccessFull() and assertEquals(204,
EventUpdatedResponse->getCode()); and call EventDeletedResponse->isSuccessFull()
and assertEquals(204, EventDeletedResponse->getCode()). Use the class names
EventCreatedResponse, EventUpdatedResponse, EventDeletedResponse and the methods
isSuccessFull() and getCode() to verify semantics rather than method_exists().
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99731e81-60d0-47ee-86b4-07235656e396
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
CHANGELOG.mdcomposer.jsonphpunit.xmlsrc/Facade/CalDavClient.phpsrc/Facade/Exceptions/ConflictException.phpsrc/Facade/Exceptions/ServerErrorException.phpsrc/Facade/Requests/AbstractPropFindWebDAVRequest.phpsrc/Facade/Requests/CalendarQueryFilter.phpsrc/Facade/Requests/CalendarQueryRequest.phpsrc/Facade/Requests/GetCalendarRequest.phpsrc/Facade/Requests/GetCalendarsRequest.phpsrc/Facade/Responses/AbstractCalDAVResponse.phpsrc/Facade/Responses/CalendarSyncInfoResponse.phpsrc/Facade/Responses/ETagEntityResponse.phpsrc/Facade/Responses/EventCreatedResponse.phpsrc/Facade/Responses/EventMovedResponse.phpsrc/Facade/Responses/GenericSinglePROPFINDCalDAVResponse.phpsrc/Facade/Responses/GetCalendarResponse.phpsrc/Facade/Responses/GetCalendarsResponse.phpsrc/Facade/Responses/UserPrincipalResponse.phpsrc/Facade/Responses/UserPrincipalSingleResponse.phpsrc/Facade/Responses/VCardEntityResponse.phpsrc/Facade/Utils/Headers.phpsrc/Facade/Utils/HttpMethods.phpsrc/Facade/Utils/RequestFactory.phptests/AllDayEventTest.phptests/CalDavClientTest.phptests/FacadeTest.phptests/GetCalendarResponseTest.phptests/RRulePreservationTest.phptests/ResponseMethodsTest.phptests/VAlarmParsingTest.phptests/fixtures/events/event-events_test.icstests/fixtures/events/event-with-absolute-alarm.icstests/fixtures/events/event-with-default-alarm.icstests/fixtures/events/event-with-mixed-alarm.icstests/fixtures/events/event-with-pt0s-alarm.icstests/fixtures/events/recurring-event-daily.ics
💤 Files with no reviewable changes (1)
- src/Facade/Exceptions/ServerErrorException.php
| @@ -0,0 +1,43 @@ | |||
| # Changelog | |||
|
|
|||
| ## Unreleased (2025-01-13) | |||
There was a problem hiding this comment.
Keep the unreleased bucket undated.
Unreleased (2025-01-13) reads like a shipped release. Leave it as plain Unreleased until you tag/version the release, then replace it with the actual version heading.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` at line 3, Change the changelog heading "## Unreleased
(2025-01-13)" to remove the date and leave it as "## Unreleased" so the
unreleased section remains undated until you tag/version a release; update the
heading string in CHANGELOG.md (look for the exact heading "## Unreleased
(2025-01-13)") and replace it with "## Unreleased".
Comment on lines
+20
to
+24
| "eluceo/ical": "^0.15.0" | ||
| }, | ||
| "require-dev": { | ||
| "phpunit/phpunit": "^5.7" | ||
| "phpunit/phpunit": "^9.5", | ||
| "sabre/vobject": "^4.2" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What are the minimum supported PHP versions for eluceo/ical ^0.15, phpunit/phpunit ^9.5, and sabre/vobject ^4.2, and what project-level Composer "require.php" constraint would satisfy all three?
💡 Result:
eluceo/ical ^0.15: PHP 7.0 phpunit/phpunit ^9.5: PHP 7.3 sabre/vobject ^4.2: PHP 7.1 Project-level "require": {"php": "^7.3"} satisfies all three, as it meets or exceeds each minimum (7.3 >= 7.3, 7.3 >= 7.1, 7.3 >= 7.0). A looser constraint like "^7.0 || ^8.0" would also work but ^7.3 is the tightest common minimum.
Citations:
- 1: https://packagist.org/packages/eluceo/ical
- 2: https://github.com/markuspoerschke/ical
- 3: https://phpunit.de/getting-started/phpunit-9.html
- 4: https://phpunit.readthedocs.io/en/8.5/installation.html
- 5: https://packagist.org/packages/sabre/vobject
- 6: https://github.com/sebastianbergmann/phpunit/tree/9.5.6
Add an explicit PHP constraint to the require section before upgrading these dependencies.
The packages being updated have specific PHP requirements: eluceo/ical ^0.15 requires PHP ≥7.0, phpunit/phpunit ^9.5 requires PHP ≥7.3, and sabre/vobject ^4.2 requires PHP ≥7.1. Without an explicit "php" constraint in composer.json, Composer will not enforce a minimum PHP version during installation, risking failures in environments with older PHP runtimes.
Add "php": "^7.3" to the require section to satisfy all three packages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composer.json` around lines 20 - 24, Add an explicit PHP platform constraint
to composer.json's "require" section: add "php": "^7.3" so Composer enforces a
minimum PHP version compatible with eluceo/ical ^0.15, phpunit/phpunit ^9.5, and
sabre/vobject ^4.2; update the "require" object (near the existing package
entries) to include the php constraint and then run composer update/validate to
ensure the new constraint is applied.
Comment on lines
+79
to
+92
| private function stripNamespacesFromTags($xml) { | ||
| // `simplexml_load_string` treats namespaced XML differently than non-namespaced XML, and | ||
| // calling `json_encode` on the results of a parsed namespaced XML string will only | ||
| // include the non-namespaced tags. Therefore, we remove the namespaces. | ||
| // | ||
| // Almost literally taken from | ||
| // https://laracasts.com/discuss/channels/general-discussion/converting-xml-to-jsonarray/replies/112561 | ||
|
|
||
|
|
||
| // We retrieve the namespaces from the XML code so we can check for | ||
| // them to remove them | ||
| $obj = simplexml_load_string($xml); | ||
| $namespaces = $obj->getNamespaces(true); | ||
| $toRemove = array_keys($namespaces); |
There was a problem hiding this comment.
Guard simplexml_load_string failure before dereferencing parser result.
Line 90 can return false; Line 91 then calls getNamespaces() on a non-object, causing a fatal error path instead of the expected parse exception handling.
Suggested diff
private function stripNamespacesFromTags($xml) {
@@
- $obj = simplexml_load_string($xml);
+ $obj = simplexml_load_string($xml);
+ if ($obj === false) {
+ throw new XMLResponseParseException();
+ }
$namespaces = $obj->getNamespaces(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Facade/Responses/AbstractCalDAVResponse.php` around lines 79 - 92, The
method stripNamespacesFromTags calls simplexml_load_string($xml) and immediately
dereferences it with getNamespaces(), which can fatal if simplexml_load_string
returns false; update stripNamespacesFromTags to guard the parser result by
checking that $obj is not false (or instanceof SimpleXMLElement) before calling
getNamespaces(), and if parsing failed throw or return a clear parse error
(consistent with existing exception handling) so callers of
stripNamespacesFromTags receive a handled parse error instead of a fatal; ensure
references to simplexml_load_string and getNamespaces in stripNamespacesFromTags
are the only changes.
Comment on lines
+42
to
+43
| if($entity instanceof ETagEntityResponse && $entity->getStatus() != HttpResponse::HttpOKStatus) continue; | ||
| $res[] = $entity; |
There was a problem hiding this comment.
Filter out non-ETagEntityResponse entries before appending.
Line 43 and Line 55 still append entities that are not ETagEntityResponse. That can leak unrelated response objects into both collections and violates the declared return type.
Proposed fix
public function getUpdates(){
$res = [];
foreach ($this->responses as $entity){
- if($entity instanceof ETagEntityResponse && $entity->getStatus() != HttpResponse::HttpOKStatus) continue;
- $res[] = $entity;
+ if(!$entity instanceof ETagEntityResponse) continue;
+ if($entity->getStatus() != HttpResponse::HttpOKStatus) continue;
+ $res[] = $entity;
}
return $res;
}
@@
public function getDeletes(){
$res = [];
foreach ($this->responses as $entity){
- if($entity instanceof ETagEntityResponse && $entity->getStatus() != HttpResponse::HttpNotFoundStatus) continue;
- $res[] = $entity;
+ if(!$entity instanceof ETagEntityResponse) continue;
+ if($entity->getStatus() != HttpResponse::HttpNotFoundStatus) continue;
+ $res[] = $entity;
}
return $res;
}Also applies to: 54-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Facade/Responses/CalendarSyncInfoResponse.php` around lines 42 - 43, The
current loop appends $entity into $res and $deleted regardless of type; ensure
you only append when $entity is an instance of ETagEntityResponse: check
instanceof ETagEntityResponse before pushing into $res or $deleted, and keep the
existing status check (compare to HttpResponse::HttpOKStatus) for $res; update
the branches that write to $res and $deleted (referencing the $entity variable,
ETagEntityResponse, HttpResponse::HttpOKStatus, and the $res/$deleted arrays) so
non-ETagEntityResponse objects are skipped and cannot leak into the returned
collections.
Comment on lines
+49
to
56
| if ($this->statusMatches($status, AbstractCalDAVResponse::HttpOKStatus)) { | ||
| $this->found_props = $this->content['response']['propstat']['prop']; | ||
| $this->not_found_props = null; | ||
| } | ||
| if ($status == AbstractCalDAVResponse::HttpNotFoundStatus) { | ||
| if ($this->statusMatches($status, AbstractCalDAVResponse::HttpNotFoundStatus)) { | ||
| $this->not_found_props = $this->content['response']['propstat']['prop']; | ||
| $this->found_props = null; | ||
| } |
There was a problem hiding this comment.
Keep found_props/not_found_props consistently typed as arrays.
Line 51 and Line 55 assign null to properties documented as array, which is causing static-analysis failures and avoidable null handling.
Suggested diff
if ($this->statusMatches($status, AbstractCalDAVResponse::HttpOKStatus)) {
$this->found_props = $this->content['response']['propstat']['prop'];
- $this->not_found_props = null;
+ $this->not_found_props = [];
}
if ($this->statusMatches($status, AbstractCalDAVResponse::HttpNotFoundStatus)) {
$this->not_found_props = $this->content['response']['propstat']['prop'];
- $this->found_props = null;
+ $this->found_props = [];
}📝 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
| if ($this->statusMatches($status, AbstractCalDAVResponse::HttpOKStatus)) { | |
| $this->found_props = $this->content['response']['propstat']['prop']; | |
| $this->not_found_props = null; | |
| } | |
| if ($status == AbstractCalDAVResponse::HttpNotFoundStatus) { | |
| if ($this->statusMatches($status, AbstractCalDAVResponse::HttpNotFoundStatus)) { | |
| $this->not_found_props = $this->content['response']['propstat']['prop']; | |
| $this->found_props = null; | |
| } | |
| if ($this->statusMatches($status, AbstractCalDAVResponse::HttpOKStatus)) { | |
| $this->found_props = $this->content['response']['propstat']['prop']; | |
| $this->not_found_props = []; | |
| } | |
| if ($this->statusMatches($status, AbstractCalDAVResponse::HttpNotFoundStatus)) { | |
| $this->not_found_props = $this->content['response']['propstat']['prop']; | |
| $this->found_props = []; | |
| } |
🧰 Tools
🪛 PHPStan (2.1.46)
[error] 51-51: Property CalDAVClient\Facade\Responses\GenericSinglePROPFINDCalDAVResponse::$not_found_props (array) does not accept null.
(assign.propertyType)
[error] 55-55: Property CalDAVClient\Facade\Responses\GenericSinglePROPFINDCalDAVResponse::$found_props (array) does not accept null.
(assign.propertyType)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Facade/Responses/GenericSinglePROPFINDCalDAVResponse.php` around lines 49
- 56, The properties found_props and not_found_props (in class
GenericSinglePROPFINDCalDAVResponse) are documented as arrays but are being set
to null when the other status branch is taken; change those assignments so they
remain arrays (e.g., set to an empty array) instead of null in the branches that
call statusMatches with AbstractCalDAVResponse::HttpOKStatus and
::HttpNotFoundStatus so static analysis no longer reports nullable types.
Comment on lines
+19
to
28
| final class UserPrincipalResponse extends GenericMultiCalDAVResponse | ||
| { | ||
|
|
||
| /** | ||
| * @return string | ||
| * @return GenericSinglePROPFINDCalDAVResponse | ||
| */ | ||
| public function getPrincipalUrl(){ | ||
| return isset($this->found_props['current-user-principal']) && isset($this->found_props['current-user-principal']['href']) ? | ||
| $this->server_url.$this->found_props['current-user-principal']['href'] : null; | ||
| protected function buildSingleResponse() | ||
| { | ||
| return new UserPrincipalSingleResponse(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for direct getPrincipalUrl() calls on UserPrincipalResponse
rg -n "getUserPrincipal\(\)" --type php -A 3 | rg -v "getResponses"Repository: smarcet/CalDAVClient
Length of output: 1788
🏁 Script executed:
# First, check what files are available in the repository
git ls-files | head -20Repository: smarcet/CalDAVClient
Length of output: 694
🏁 Script executed:
# Find all response-related files
git ls-files | grep -i response | head -30Repository: smarcet/CalDAVClient
Length of output: 1265
🏁 Script executed:
# Search for GenericSinglePROPFINDCalDAVResponse and GenericMultiCalDAVResponse definitions
rg -n "class GenericSinglePROPFINDCalDAVResponse|class GenericMultiCalDAVResponse" --type phpRepository: smarcet/CalDAVClient
Length of output: 319
🏁 Script executed:
# Search for getPrincipalUrl method definitions
rg -n "function getPrincipalUrl|public.*getPrincipalUrl" --type phpRepository: smarcet/CalDAVClient
Length of output: 159
🏁 Script executed:
# Search for UserPrincipalResponse usage and getPrincipalUrl calls on it
rg -n "UserPrincipalResponse.*getPrincipalUrl|getPrincipalUrl" --type php -B 2 -A 2Repository: smarcet/CalDAVClient
Length of output: 916
🏁 Script executed:
# Read UserPrincipalResponse.php full content
cat -n src/Facade/Responses/UserPrincipalResponse.phpRepository: smarcet/CalDAVClient
Length of output: 1236
🏁 Script executed:
# Read GenericSinglePROPFINDCalDAVResponse.php to understand its methods
cat -n src/Facade/Responses/GenericSinglePROPFINDCalDAVResponse.phpRepository: smarcet/CalDAVClient
Length of output: 4480
🏁 Script executed:
# Read GenericMultiCalDAVResponse.php to understand its methods
cat -n src/Facade/Responses/GenericMultiCalDAVResponse.phpRepository: smarcet/CalDAVClient
Length of output: 2747
🏁 Script executed:
# Read UserPrincipalSingleResponse.php to verify inheritance
cat -n src/Facade/Responses/UserPrincipalSingleResponse.phpRepository: smarcet/CalDAVClient
Length of output: 1772
Fix inaccurate return type annotation in docblock.
The buildSingleResponse() method's docblock at line 23 declares @return GenericSinglePROPFINDCalDAVResponse, but the method returns UserPrincipalSingleResponse. Update the docblock to reflect the actual return type:
/**
* `@return` UserPrincipalSingleResponse
*/
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Facade/Responses/UserPrincipalResponse.php` around lines 19 - 28, The
docblock for method buildSingleResponse() in class UserPrincipalResponse
incorrectly states `@return` GenericSinglePROPFINDCalDAVResponse; update that
docblock to reflect the actual return type by changing it to `@return`
UserPrincipalSingleResponse so the annotation matches the method's returned
object (UserPrincipalSingleResponse).
Comment on lines
+21
to
+31
| /** | ||
| * @return string | ||
| */ | ||
| public function getPrincipalUrl() { | ||
| $url = isset($this->found_props['current-user-principal']) && isset($this->found_props['current-user-principal']['href']) ? | ||
| $this->server_url.$this->found_props['current-user-principal']['href'] : null; | ||
| // check on not found one ( issue on caldav icloud imp) | ||
| if(empty($url)) | ||
| $url = isset($this->not_found_props['current-user-principal']) && isset($this->not_found_props['current-user-principal']['href']) ? | ||
| $this->server_url.$this->not_found_props['current-user-principal']['href'] : null; | ||
| return $url; |
There was a problem hiding this comment.
Update return type annotation to reflect nullable return.
The docblock declares @return string but the method returns null when the principal URL cannot be found in either found_props or not_found_props.
Proposed fix
/**
- * `@return` string
+ * `@return` string|null
*/
public function getPrincipalUrl() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Facade/Responses/UserPrincipalSingleResponse.php` around lines 21 - 31,
The docblock and signature for getPrincipalUrl() incorrectly state it returns
string while it can return null; update the documentation and signature to
reflect a nullable return by changing the docblock to "@return string|null" and
adding a nullable return type (e.g. ?string) to the getPrincipalUrl method
declaration, ensuring references to $this->found_props, $this->not_found_props
and $this->server_url remain unchanged.
Comment on lines
21
to
29
| /** | ||
| * @return string | ||
| */ | ||
| public function getVCard(){ | ||
| return isset($this->found_props['calendar-data']) ? $this->found_props['calendar-data'] : null; | ||
| public function getVCard() { | ||
| if (!isset($this->found_props['calendar-data'])) return null; | ||
| $calendar_data = $this->found_props['calendar-data']; | ||
| if (!is_string($calendar_data)) return null; | ||
| return $calendar_data; | ||
| } |
There was a problem hiding this comment.
Update return type annotation to reflect nullable return.
The method can return null in two cases, but the docblock declares @return string. This causes the PHPStan errors and could mislead callers expecting a guaranteed string.
Proposed fix
/**
- * `@return` string
+ * `@return` string|null
*/
public function getVCard() {📝 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
| /** | |
| * @return string | |
| */ | |
| public function getVCard(){ | |
| return isset($this->found_props['calendar-data']) ? $this->found_props['calendar-data'] : null; | |
| public function getVCard() { | |
| if (!isset($this->found_props['calendar-data'])) return null; | |
| $calendar_data = $this->found_props['calendar-data']; | |
| if (!is_string($calendar_data)) return null; | |
| return $calendar_data; | |
| } | |
| /** | |
| * `@return` string|null | |
| */ | |
| public function getVCard() { | |
| if (!isset($this->found_props['calendar-data'])) return null; | |
| $calendar_data = $this->found_props['calendar-data']; | |
| if (!is_string($calendar_data)) return null; | |
| return $calendar_data; | |
| } |
🧰 Tools
🪛 PHPStan (2.1.46)
[error] 25-25: Method CalDAVClient\Facade\Responses\VCardEntityResponse::getVCard() should return string but returns null.
(return.type)
[error] 27-27: Method CalDAVClient\Facade\Responses\VCardEntityResponse::getVCard() should return string but returns null.
(return.type)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Facade/Responses/VCardEntityResponse.php` around lines 21 - 29, The
getVCard method in VCardEntityResponse can return null but its docblock says
"@return string"; update the docblock to reflect a nullable return (e.g.,
"@return string|null") and, if desired, add a nullable return type in the method
signature (public function getVCard(): ?string) to make the contract explicit;
ensure references to getVCard and the found_props['calendar-data'] checks remain
unchanged.
| const ContentType = 'Content-Type'; | ||
| const ContentLength = 'Content-Length'; | ||
| const IfMatch = 'If-Match'; | ||
| const IfNotMatch = 'If-None-Match"'; |
There was a problem hiding this comment.
Fix the malformed If-None-Match header name.
The trailing quote makes this constant invalid, so any caller using it will send If-None-Match" instead of the real header.
🛠️ Suggested fix
- const IfNotMatch = 'If-None-Match"';
+ const IfNotMatch = 'If-None-Match';📝 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
| const IfNotMatch = 'If-None-Match"'; | |
| const IfNotMatch = 'If-None-Match'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Facade/Utils/Headers.php` at line 26, The constant IfNotMatch in
Headers.php is malformed (it contains a trailing double-quote) causing callers
to send the wrong header name; update the Headers::IfNotMatch constant value to
the correct header string "If-None-Match" (remove the extraneous quote) so any
code referencing Headers::IfNotMatch will send the proper header name.
Comment on lines
+23
to
+36
| public function testGetCredentialsReturnsUserAndPassword() | ||
| { | ||
| $client = new CalDavClient( | ||
| 'https://caldav.example.com', | ||
| 'testuser', | ||
| 'testpass' | ||
| ); | ||
|
|
||
| $credentials = $client->getCredentials(); | ||
|
|
||
| $this->assertArrayHasKey('user', $credentials); | ||
| $this->assertArrayHasKey('password', $credentials); | ||
| $this->assertEquals('testuser', $credentials['user']); | ||
| $this->assertEquals('testpass', $credentials['password']); |
There was a problem hiding this comment.
Don't freeze raw password exposure into the public API.
This test locks in src/Facade/CalDavClient.php:148-154 returning the stored password verbatim. That widens the accidental secret-leak surface and will be hard to remove once downstream code depends on it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/CalDavClientTest.php` around lines 23 - 36, The test currently relies
on CalDavClient::getCredentials returning the raw password; change the public
API so getCredentials (or a new public method such as getAuthHeader) does not
expose the plaintext secret: have CalDavClient::getCredentials return only the
'user' and an 'authorization' value (e.g., an HTTP Basic Authorization header or
a token) or simply omit the password and add a getAuthHeader() method that
builds the header from the stored secret internally; update any consumers/tests
to assert the presence of the authorization/header value (or absence of a raw
'password' key) instead of checking the plaintext password.
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
5 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.
Adds calendar-color prop in GetCalendarsRequest for iCloud caldav
Summary by CodeRabbit
Release Notes
New Features
Improvements