Skip to content

fix: harden Messenger validation and review feedback - #124

Open
deepshekhardas wants to merge 1 commit into
utopia-php:mainfrom
deepshekhardas:pr-115
Open

fix: harden Messenger validation and review feedback#124
deepshekhardas wants to merge 1 commit into
utopia-php:mainfrom
deepshekhardas:pr-115

Conversation

@deepshekhardas

Copy link
Copy Markdown

Harden Messenger validation based on review feedback. Improves input validation and error handling across message processing.

@greptile-apps

greptile-apps Bot commented Jun 7, 2026

Copy link
Copy Markdown

Greptile Summary

Adds a multi-adapter Messenger with ordered failover and validation.

  • Documents multi-adapter failover usage in the README.
  • Adds compatibility checks, message-type validation, aggregate errors, and adapter metadata accessors.
  • Adds tests for success, failover, validation, metadata, and failure payload behavior.

Confidence Score: 3/5

The PR is not yet safe to merge because adapter errors can still terminate failover and preserved-key adapter arrays can still fail during construction.

The current implementation catches only \Exception around adapter sends and directly accesses key zero after accepting arbitrary array keys, so two previously reported runtime failures remain reachable.

Files Needing Attention: src/Utopia/Messaging/Messenger.php

Important Files Changed

Filename Overview
src/Utopia/Messaging/Messenger.php Introduces the Messenger orchestration, validation, failover, and aggregate error behavior.
tests/Messaging/MessengerTest.php Adds coverage for adapter ordering, exception failover, constructor validation, metadata, and returned failure payloads.
README.md Documents construction and use of a Messenger with multiple SMS adapters.

Reviews (5): Last reviewed commit: "feat: add Messenger class with failover ..." | Re-trigger Greptile

Comment on lines +103 to +109
} catch (\Exception $e) {
$errors[] = $adapter->getName()
.' (adapter '
.($index + 1)
.'): '
.$e->getMessage();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 The catch clause only intercepts \Exception, so any adapter that throws a subclass of \Error (e.g. \TypeError from a bad API response shape, \ValueError, or an unchecked \AssertionError) will propagate immediately and skip every remaining adapter — silently breaking the failover chain that is the entire purpose of this class. Using \Throwable ensures all adapter failures trigger the fallback path.

Suggested change
} catch (\Exception $e) {
$errors[] = $adapter->getName()
.' (adapter '
.($index + 1)
.'): '
.$e->getMessage();
}
} catch (\Throwable $e) {
$errors[] = $adapter->getName()
.' (adapter '
.($index + 1)
.'): '
.$e->getMessage();
}

Comment on lines +57 to +60
.' is '
.\get_debug_type($adapter)
.'.'
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Index-base inconsistency in error messages

The constructor loop emits "element N" using the raw 0-based key (e.g. "element 1" for the second item), while validateAdapters emits "adapter N" using a 1-based offset ($index + 1). A caller who encounters both errors will see different numbering conventions for what is logically the same position in the array. Choosing one scheme (consistently 0-based or consistently 1-based) would make the messages unambiguous.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@greptile-apps

greptile-apps Bot commented Jun 13, 2026

Copy link
Copy Markdown

Want your agent to iterate on Greptile's feedback? Try greploops.

Comment on lines +64 to +66
$this->validateAdapters($adapters);

$this->adapters = $adapters;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Normalize adapter keys

The constructor accepts any adapter array, but validateAdapters() reads $adapters[0] directly. PHP arrays preserve caller keys, so a valid list with preserved keys, such as the result of filtering adapters or [1 => $adapter], passes the earlier empty() and instanceof checks and then fails with an undefined key/null dereference instead of constructing a single-adapter messenger. Reindex the array before validation, or explicitly reject non-list arrays, so valid adapter lists do not depend on having key 0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant