Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Problem/Motivation
The current implementation of the sms_user data attached to a user profile is as an undeclared class property.
Proposed resolution
Implement sms_user
data as a Drupal 8 field which can be attached to the User entity.
Attach the sms_user
field by default to the User entity using hook_entity_base_field_info()
to retain present functionality.
Remaining tasks
Patch
Reviews
Commit
User interface changes
None
API changes
TBD
Data model changes
The sms_user
database table will be removed and replaced by the D8 auto generated field tables for sms_user
field type.
Comment | File | Size | Author |
---|---|---|---|
#4 | sms_user_field.patch | 19.13 KB | almaudoh |
Comments
Comment #2
dpiI can work on this, are you OK with sms_user submodule relying on the Telephone module bundled with Drupal 8?
Or did you have an alternate plan in mind?
Comment #3
dpiI think it may be worth removing the fake $user property while I'm at it.
Comment #4
almaudoh CreditAttribution: almaudoh commentedI had started some work on this. Maybe I'll just post the patch here so you can continue. I was planning to actually make the fake sms_user property into a D8 field.
Since, you're taking this on, here's where I am at right now. Feel free to go with it or start something different. I'll review.
Comment #5
dpiI'm yet to take a look at your patch, but these are some thoughts I had in the time between #3-4:
Let me know what you think about these design changes. These dont necessarily need to be covered by this patch, but they are very relevant.
Comment #6
almaudoh CreditAttribution: almaudoh commented@dpi: all your points look good to me. I would only add that by default, we should attach the new field to the user entity when sms_user module is installed to maintain some backward compatibility with existing code (tried to do that in my patch).
Additionally, some of the other that should be incorporated to the phone number field:
However, the fields relating to sleep settings and opt-in to SMS messages should probably be implemented as separate fields (maybe re-using existing field types in D8 core).
Comment #7
dpiAh, I've taken a look at your patch.
Adding properties (status, code, gateway etc) to a single field may be overloading the field system. The downside to using this method is SMS Framework needs to implements its own widget, formatter, and Views integration for the field. In addition, custom field formatters and widgets need to be written in order to create custom displays of the data.
My proposal was to add Drupal primitive fields to the bundle, the same as adding fields to a bundle using Field UI. SMS Framework only needs to initially create the field instance and attach it. Then maintain which field the relevant data is stored. This way any existing formatters and widgets from can be used.
For example, Telephone field:
Create a field and instance, maintain association as config for each:
Comment #8
dpiSleep
I'd also like to add that perhaps the sleep system could be handled on a site level?
For example a site-global setting to only send SMS between time_start and time_end depending on users local time. SMS Framework can determine the Country (from the phone number) and/or timezone of the user, then localize the current time of the user.
I'm not sure if users actually want to micro-manage this kind of preference? Site admins should be responsible enough to not send SMS at inappropriate hours; a sensible default should be to enable the sleep functions with default range of 9am-5pm (which automatically adjusts to user local time). If at a later time a site actually needs user micromanagement; it shouldnt be much effort to create a third party module which hooks into SMSFwk and delays delivery.
Confirmation code
Code is only useful as temporary state. It may be better to maintain code as its own table row, allowing the cron system to clean up old entries more efficienty
Comment #9
dpiAny opinions on above?
Comment #10
almaudoh CreditAttribution: almaudoh commented#8:
Sleep: A good compromise I think is to have a system / global level setting like you suggest, but also allow users to override those settings if they need to.
Confirmation: A separate table makes sense too. Maybe even a separate service to handle the whole phone number confirmation business, that way we decouple the two concerns.
#7: I agree in principle. We don't need the EntityReference (gateway) field though as that simply complicates matters and is hardly used by anyone AFAIK.
Comment #11
dpiSearch for
BaseFieldDefinition::create('entity_reference')
in Drupal.entity_reference is used by entities that reference a bundle. for example: the 'type' field on node entities is an entity_reference. There is less overhead with ER than D7, and it is included as a *core* field type. (There is no longer a Entity Reference module)
Comment #12
almaudoh CreditAttribution: almaudoh commentedI meant the "gateway" part of
$user->sms_user
...Comment #13
dpiAh ok, interpreted that incorrectly.
We can remove it ;)
Comment #14
dpiIll tackle this issue as soon as #2642024: Gateway instances as config entities is resolved, as it is rather monolithic.
Comment #15
dpiComment #16
dpiComment #17
dpiDang wrong issue
Comment #18
almaudoh CreditAttribution: almaudoh commentedThe scope of this issue has already been done in the parent issue #2643692: SMS User Redesign