Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
#2643692: SMS User Redesign needs SMS message storage.
Sms_track' purpose is to provide temporary SMS storage. Consider integrating sms_track into core module, and converting storage from plain table to a full entity. This allows us to upgrade \Drupal\sms\Message\SmsMessage
to a full entity. Existing code will still work, we would be passing around unsaved stub entities.
Comment | File | Size | Author |
---|---|---|---|
#28 | sms-storage-2677684-5b3d5aa.patch | 35.62 KB | dpi |
#27 | sms-storage-2677684-7e7f917.diff | 29.54 KB | dpi |
#26 | sms-storage-2677684.patch | 15.55 KB | dpi |
Comments
Comment #2
dpiComment #3
almaudoh CreditAttribution: almaudoh commentedI had thought of this several times in the past, but it never seemed like a good idea to turn SMS message into entities.
It is not in all use cases that a website may want to keep records of sent messages. Secondly, the entity system is designed for the use cases where user interaction to add / remove fields and change their values is needed via UI. In the case of SmsMessageInterface, it is designed to be a lightweight value object to allow for storing huge volumes if needed (as in the case of bulk messaging sites) - the Entity system would be too heavy and too slow.
So:
sms_track
Comment #4
dpiI think you are underestimating how fast the entity system is when you dont have fields or controllers and junk :)
If you dont expose the possibility of fields or UI to the user, then you dont get the slowdown.
The benefit of the entity system in this case is we have the exact same object or derivative interface, you're passing around unsaved stub entity objects. Its just one command away ($obj->save()) from inserting into storage.
Anyways, if you can take a look at this: https://github.com/dpi/courier_sms/blob/8.x-1.x/src/Entity/SmsMessage.php and https://github.com/dpi/courier_sms/blob/8.x-1.x/src/SmsMessageInterface.php
The entity inherits the SMS Framework interface. In this case its SmsMessage (stdclass) -> SmsMessage (entity).
Comment #5
dpiIll do a benchmark tomorrow on actual time and memory creating unsaved courier_sms stub entities
Comment #6
almaudoh CreditAttribution: almaudoh commentedI'll be pleasantly surprised to find that D8 entities are as fast as value objects, with all the additional crud (pun not intended) of the entity system. That would perhaps explain why you like the entity system so much. (Yeah, I've checked out Courier and RNG)
Comment #7
dpiThats not what I said. Its still a SQL/ORM layer. You weigh the benefits of overhead versus having to develop and maintain a system which Drupal can give you for free.
In any case, how many theoretical SMS message objects are expected to exist during a single request?
Surely < 100 messages.
Even if more are required, that code block is aware of performance issues and should be adding messages to a #2677682: SMS message FIFO dispatch queue. And if required, unloading the entities from memory.
Comment #8
almaudoh CreditAttribution: almaudoh commentedOn my site, I sometimes have customized SMS messages sent to up to 5000 recipients, even though the custom module I use splits them into blocks of ~300 recipients.
The second concern is the additional storage space in the database which entities use for field information storage. Each field creates its own table. Of course, if we don't allow custom fields to be added, then that can be mitigated, but I've never tested that though.
Comment #9
dpiAgain, if you dont give the user an interface to add fields, there is no concern. If you want the field system, you must explicitly code for it.
I dont know how much you are aware of D8 entity system, but a entity type only needs one table: the base field table. This includes the ID field, and any other field defined in basefielddefinitions. My point is: a "sms message" entity uses no more data than the track module currently does:
courier_sms: Entity/SmsMessage.php
Let me be clear. The only overhead is memory and CPU: The Drupal entity system.
rhetorical So what is the limit, 50000, 1000000, 100000000 messages? If code has no upper limit, then it is its responsibility to implement batching.
I will return with perf benchmarks
Comment #10
dpiComment #11
dpiHad some thoughts regarding this today, subclass the current SmsMessage class into an entity:
The entity is storage for processing queue, with the following extra data:
Comment #12
dpiI've spent some time looking over what would be required to accommodate an integrated #2677682: SMS message FIFO dispatch queue and an SMS archival system (sms_track).
In the interest of moving forward (with #2677682: SMS message FIFO dispatch queue and #2677676: Automated Messages & Sleep), it would be great to catch up on IRC to discuss it.
Turns out I have taken a lot of inspiration from the existing {sms_track} table.
I have created an entity definition here. (branch: dpi/sms-storage-2677684)
Design changes
Gateway annotation: Gateway supports multiple recipients in a single request
Plugin definition should indicate whether the endpoint allows multiple recipients in a single request: When a message is added to the queue, if the gateway does not supports multiple recipients, then it will split the SMS message out into separate queue items.
New per gateway configuration:
edit: removed sql table def; d.o formats it incorrectly.
Comment #13
dpiadding bitmap of message schema
Comment #14
almaudoh CreditAttribution: almaudoh commentedWe could make this a max_recipients_per_request property. I have implemented split message sending in sms_gateway and I found different gateways have different limits.
Is the message schema for queue, track or general purpose? What's the purpose of entity target ids? What of the use cases where the sender or recipients are just phone numbers not attached to any entities, e.g. in some bulk SMS applications? I'm available on irc now. Was a bit under the weather for the last few days.
Comment #15
almaudoh CreditAttribution: almaudoh commentedThe other thing to consider is: are we having a 1-to-1 mapping between SMS messages (which could have multiple recipients) and the storage schema above (which makes room for only 1 recipient). The implementation at branch: dpi/sms-storage-2677684 assumes a single recipient.
The implementation of sms_track in HEAD also assumes only a single recipient, but I made some changes in the patch #2542790-38: Implement API for delivery reports for SMS Gateways that corrected that assumption.
Comment #16
dpiThis sounds great. I think the normal 0=unlimited (aka no known limit) magic number makes sense here.
Technically its for both. As in after a message is processed by the gateway, it will remain in the queue in an archived/expired state. So messages will be deleted if
{message retention seconds} == 0 || {now time} > {processed time} + {message retention seconds}
.Thats a DER field, they have two columns. Target ID and target entity type. This is used in place of the existing sms_track 'author' and 'recipient' uid fields.
The recipient_entity and sender_entity fields are optional, and are only used for administration UI (similar/same as sms_track 'author' and 'recipient'). The messages are always sent to phone numbers on the 'recipient' field.
If a message is sent to using the phone number service, then we calculate the destination phone number for the sms message. the entity is then only informational usage
actually the 'recipient' field on the entity has unlimited cardinality, which means it creates a new table, and does auto joins.
Thats where the 'max_recipients_per_request' property comes in. If the gateway doesnt support it, then we have to create a message for every recipient. if the gateway does support multiple, then we create one message object with a bunch recipients attached.
Comment #17
dpiI forgot to screenshot the other table for #13.
For the entity, there is a total of two tables. The base table, and the recipient table (the field has >1 cardinality)
Comment #18
almaudoh CreditAttribution: almaudoh commentedSo for storage purposes (ala sms_track), there needs to be a bunch of reports with corresponding delivery status and message ids matching that bunch of recipients as well. Take note that message ids are unique per message per recipient. The
reference
field is what I used initially to map to message ids. So in a multi-cardinality recipients field, we have to use something else to identify individual messages.Comment #19
dpiI see.
Of course we could just as well remove the cardinality on recipients, so one entity = one sms + one recipient so it is the same as {sms_track}.
Or it might make sense to have another new table with delivery reports data?
I guess we're back to the question for storage (and/or entity) for delivery reports.
Im deferring to you for domain advice on this one.
Comment #20
dpiI think the only things I need to know for this issue is whether to:
Comment #21
almaudoh CreditAttribution: almaudoh commentedLet's not do this.
I think this is a better idea. We can move the reference and delivery status fields to a new table that would hold just information on delivery. While information on sender, message, sendtime, etc. remains in the original table, since this would be the same for all recipients.
Comment #22
dpiI hope to return to this shortly. Im currently wrapping up ancillary modules, and a 1.0 for RNG.
Comment #23
dpiI have returned! It'd be great if you could take a look at this PR.
https://github.com/dpi/smsframework/pull/10
https://github.com/dpi/smsframework/pull/10/files
Comment #24
dpiI think we have reached a consensus on the schema. Ill commit this soon so we can refine it further in #2677682: SMS message FIFO dispatch queue. Don't hesitate to provide more feedback.
Comment #25
almaudoh CreditAttribution: almaudoh commentedDo we have any tests for the Entity. You should post a patch here just to ensure d.o testbots pass.
Comment #26
dpiNo tests yet.
Posting patch for testbot review. No changes since GH PR#10
Comment #27
dpiTesting patch for 7e7f917
Individual commits added to the PR. No features, just fixing bugs and adding tests.
Details in the commit messages.
Comment #28
dpiTesting patch for 5b3d5aa
Again no new features, adding tests for remaining untested methods on smsmessage entity. This should make it 100%
Comment #30
dpiCommited since there's full coverage. Feel free to provide more feedback.
Comment #31
almaudoh CreditAttribution: almaudoh commentedGreat work!!
Comment #36
dpi