Repository navigation
fix(psalm): resolve more type issues flagged by Psalm 7 - #13933
Open
ChristophWurst wants to merge 1 commit into
Open
ChristophWurst wants to merge 1 commit into
ChristophWurst wants to merge 1 commit into
Conversation
Second batch from the open Psalm v7 bump (#13243), same approach as the previous one (#13932): real, version-independent issues only, not the new unused-code/purity detectors v7 enables by default. - IspDb::getUrls() had a doc typo, @returns instead of @return, so Psalm never saw its string[] return type and had to infer a much looser one, which caused a possibly-invalid-cast complaint two methods away. Fixing the typo fixes the root cause. - Html::convertLinks()/parseMailBody(): mb_convert_encoding() and preg_split() can return false, unguarded. - JmapMailboxConnector, JmapMessageAdapter, AiIntegrationsService, ItineraryService: json_encode()/preg_replace() calls missing the JSON_THROW_ON_ERROR flag or a null-coalescing fallback, matching the pattern already used elsewhere in each of these files. - ConvertAddresses: Horde_Mail_Rfc822_List::current() is typed to possibly return null, which foreach can't actually hit once valid() has confirmed there's an item; suppressed like the similar Horde docblock gap fixed in #13932. - SubmitContentJob: simplified a nullsafe call and its fallback that Psalm proved is unreachable (the foreach it's in always runs at least once). - DelegationForbiddenException: missing #[\Override] on an overridden method. - AiIntegrations/Cache::getBackoffTtl(): suppressed rather than fixed - the obvious cast-earlier fix changes behavior for large $attempt values (2 ** $attempt overflows to a huge float before min() clamps it back down), which broke an existing test, so the possibly-float operand is intentional here. - GravatarSource: stream_get_contents() can return false, which the existing null/empty guard didn't account for. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Christoph Wurst <1374172+ChristophWurst@users.noreply.github.com>
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 is the second batch pulled out of the open Psalm v7 bump (#13243), following the same approach as #13932: find the real, version-independent issues in Psalm v7's report and fix those, leaving the new unused-code/purity detectors v7 turns on by default (the bulk of the report, not addressed by either of these two PRs) for the actual version bump to deal with.
The most interesting one here is
IspDb::getUrls(): it already had a@return string[]annotation, except it was typo'd as@returns, so Psalm silently ignored it and fell back to the barearraytype, which is what caused a confusing possibly-invalid-cast complaint two methods further down. Fixing the typo fixes the root cause directly.The rest are the same kind of thing as the first batch: a few
json_encode()/preg_replace()/mb_convert_encoding()calls that can return false or null without a guard, a Horde API whose iterator is typed to possibly return null in a spot whereforeachcan't actually hit that case, a missing#[\Override]attribute, and a nullsafe call plus its fallback that Psalm proved unreachable.One of these needed more care than expected:
AiIntegrations/Cache::getBackoffTtl()looked like the same "just cast it" fix as the others, but casting the intermediate2 ** $attemptresult to int before it reachesmin()actually changes behavior for large$attemptvalues, since2 ** $attemptcan overflow into an enormous float that onlymin()clamps back down to a sane range. I caught this because it broke an existing test for large attempt counts, so that one is suppressed instead of restructured.I ran the full unit test suite and
composer cs:checklocally, both clean.Assisted-by: Claude:claude-sonnet-5