Repository navigation
fix(psalm): resolve type issues flagged by Psalm 7 - #13932
Open
ChristophWurst wants to merge 2 commits into
Open
ChristophWurst wants to merge 2 commits into
ChristophWurst wants to merge 2 commits into
Conversation
Checked out the open Psalm v7 bump (#13243) and read its CI output to find issues Psalm v7 catches that are real, version-independent bugs rather than the new unused-code/purity detectors v7 turns on by default (those make up the bulk of the new report and aren't addressed here). Fixes: - HordeSyncTokenParser: base64_decode() can return false in strict mode, which was then passed into explode() unguarded. - ProxyController: file_get_contents() on the bundled blocked-image asset can return false; return a 500 instead of constructing a response with a false body. - MessageMapper (Db): mb_convert_encoding()/preg_replace() can return false/null during preview-text cleanup; fall back to the pre-transform value, matching the existing pattern in Html.php. - MessageMapper (IMAP): Horde_Mime_Part's own docblocks say setDispositionParameter()/setContentTypeParameter() take a string, but the implementation explicitly supports and relies on null to remove the parameter. Suppressed with a reference to the actual behavior. - IMAPMessage: same mb_convert_encoding() false-return gap, in keyword display-name normalization. - ContactsIntegration: newContact() already returns null when contacts integration is disabled; extended that to the case where the user has no address book at all, instead of calling the contacts API with a null address-book key. - The four FeatureExtraction classes and ImportanceClassifier: a message's sender can have a null email ("This should not happen" case in extract()), which doesn't actually corrupt the computed ratios since it would just miss the stats map as a non-matching key; suppressed instead of hardening with a guard. Also switched a debug json_encode() call to JSON_THROW_ON_ERROR, matching the pattern used everywhere else in the codebase. This should shrink the diff of the eventual Psalm v7 bump itself, following the pattern from #11224/#13542/#13340. The remaining Psalm v7 findings in Html.php, AutoConfig/IspDb.php, and a handful of zero-baseline files are left for a follow-up. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Christoph Wurst <1374172+ChristophWurst@users.noreply.github.com>
Signed-off-by: Christoph Wurst <1374172+ChristophWurst@users.noreply.github.com>
kesselb
approved these changes
Oct 25, 2026
This branch has not been deployed
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
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.
This pulls a handful of real type-safety fixes out of the open Psalm v7 bump (#13243), following the same pattern as #13542 and #13340 before the v6 upgrade: find the issues Psalm v7 catches, fix the ones that are real bugs independent of which Psalm version is running, and merge them separately so the eventual version-bump PR has less to carry.
Psalm v7's report on #13243 is huge (about 1800 errors), but the vast majority of that is new unused-code and purity-annotation detectors it turns on by default, which Psalm v5 doesn't check at all - those aren't addressed here, they're the v7 bump's own problem to solve via baseline or config. What's fixed here is the small set of ordinary null/false-argument issues: a few spots pass
mb_convert_encoding()'s possibly-false result orpreg_replace()'s possibly-null result into functions that need a plain string,base64_decode()in strict mode is used unguarded,file_get_contents()on a bundled asset isn't checked for failure, and a couple of Horde/contacts-API calls needed either a suppression (where the called library's docblock is simply wrong about accepting null) or a small guard.I ran the full unit test suite and
composer cs:checklocally, both clean, and added tests for the two spots where the new behavior needed one (the malformed sync-token case and the no-address-book case).Assisted-by: Claude:claude-sonnet-5