-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Base architecture #7
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few suggestions:
- I would add all adapters under an
Adapters
folder - All categories of adapters could still be at this path fore example:
src/Utopia/Messaging/Adapters/Email/Mailgun.php
- With the new directory structure, specific abstract don't need a postfix. For example:
EmailAdapter.php
can just beEmail.php
Messages
could be an independent directory alongsideAdapters
so each entity is independent. No need for aMessage
postfix we can leverage namespaces for that with the new dir structure.- We should prefer shorter naming conventions when possible.
sendMessage()
can be justsend()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work Jake! Left a few comments
|
||
public function getMaxMessagesPerRequest(): int | ||
{ | ||
return PHP_INT_MAX; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will probably hit HTTP request size limit before that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case the exception will propagate to the caller, the service has no limit, unless we want to apply an arbitrary one ourselves instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what might be the best approach, but we would need to take payload size to consideration at some point, either here or in Appwrite itself.
What does this PR do?
Adapter
class