Update Driver.php

[TDannhauer]

Handles access to Collections properties more defensive as they are optional according Protocol (..stated the internet... ;) )

[Copilot]

Pull request overview

This PR adds defensive checks for optional properties in the $collection array when accessing bodyprefs and mimesupport fields. The changes prevent potential undefined index errors by using !empty() checks with appropriate default values.

Changes:

• Added !empty() checks for bodyprefs and mimesupport collection properties across calendar, contacts, tasks, notes, and mail export functions
• Standardized truncation handling for tasks and notes to match the pattern used in calendar and contacts
• Updated error logging to include the same defensive checks for consistency

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[TDannhauer]

@amulet1 : can you review this PR? Does it make sense?

[amulet1]

I am not a fan of empty() after I discovered that it considers string '0' (or number 0) to be empty.

This can potentially cause serious issues (maybe not in this particular case though).

I think the idea here is to suppress warnings if some of the parameters were not defined by replacing them with default values. If so, I think isset() is a better option to use than empty().

(1) 'bodyprefs' => !empty($collection['bodyprefs']) ? $collection['bodyprefs'] : []
can then be written as
(2) 'bodyprefs' => isset($collection['bodyprefs']) ? $collection['bodyprefs'] : []

The difference is that (1) would convert '0' (or 0 or empty string) to empty array. In general case this is probably undesired. (2) would leave them intact and only use empty array for undefined values or null values, which looks more reasonable.

There is an even cleaner way. Use null coalescing operator and it becomes this:
(3) 'bodyprefs' => $collection['bodyprefs']) ?? []

I think (3) is what we should use going forward as a preferred way to fix potential issues with undefined variables.

[TDannhauer]

@amulet1 thanks for the review, adapted it accordingly. To be tested later and then I'll merge

[amulet1]

I am also wondering if maybe these defensive checks should also be done at the destination (i.e. in the corresponding Api.php files) as there could be other callers of API functions.

[TDannhauer]

@amulet1 I think you are right, my appròach was the cheap one to get rid of the warnings. The final approach would be to stabilize the EAS16.0 procotol implementation , but it requires deeper knowledge of the protocol and the current implementation.
My current time contraints do not allow me the kneel deep enough into protocol and code.
PRs and deeper involvment in ActiveSync is highly welcomed.

That is your recommendation?

Should we abandon this changes torwards a deeper solution?
Is ActiveSync in your focus area you are interested to go deeper into it?

[amulet1]

Should we abandon this changes torwards a deeper solution?

I think these are good changes and they should be merged. Even if this is not a complete fix, at the very least it reduces errors/warnings in the logs.

Is ActiveSync in your focus area you are interested to go deeper into it?

Yes, but I am also limited on available time to devote to Horde. Currently I am mostly focused on obvious bug-fixes, reviving CI with unit tests and stability improvements (same as you are, I suppose).

If we get a full control over the repository (and related things) we could reevaluate priorities and focus on supporting new things (such as modernizing javascript libraries, EAS, modern PHP standards, etc.).