Separate entity phone number from verification service for flexibility.

Great idea!

Mobile number however does not assume a single phone number field for an entity, so a field parameter would be needed. This is solved by SmsF with the phone number config entities that wrap around entities which identifies a single field per entity. Mobile number could work with that if SmsF allowed selection of Mobile Number fields when selecting the field for the entity in the phone number ui. Mobile Number has the 'value' property in common with telephone fields, so it should be simple integration, but there are a couple issues with that:

1. Mobile Number field type has it's own field settings, so the option for creation of the field from within the phone number ui would be less straight forward. I guess Mobile Number doesn't have to be an option for the quick field creation, but it should be made clear that a 'Telephone' field is created.
2. Verification settings, such as the verification message, are part of the phone number ui, which would create confusion about use with other verification systems. Perhaps a checkbox should be added to enable SmsF verification and show/hide fields with field states, but it should say that that can conflict with other verification systems. This would also make sure there is true separation between the SmsF phone numbers and the verification system.

Of course this is a one option, another is for Mobile Number to implement a similar single field per entity configuration, but that would create more duplication and confusion between the two modules.

I noticed that the for verification the SmsF widget is needed, perhaps the verification settings and functionality can be offloaded to the widget. This is what I did for Mobile Number in the 7.x branch, although verification is not really enabled for telephone fields.

We do not assume a phone number per entity. Hence the plural naming and return signature of PhoneNumberProvider::getPhoneNumbers. I believe the auto-field creation uses single phone number. Theoretically the events proposed by OP will allow both verification systems to co-exist.

I didn't mean a single phone number, I meant a single field is assumed, because the config entity allows only one field to be set per bundle.

I am not proposing that you have integration with our verification systems, auto field creation, administrative UI controls. Pretty much anything in PhoneNumberProvider other than getPhoneNumbers() and sendMessage() is ours and is customized to our usage.

Yes, I'm talking about getPhoneNumbers(), it either needs a field name parameter, or I would have to duplicate the single field per bundle config that you implemented. Or you could allow your config entity to support one more field type, which is what I proposed, perhaps through a hook/event.

You can keep your own verification system, including its own field type, verification messages etc. And optionally you could disable ours UI's via route access control, so it does not confuse admins.

I think the config setup in SmsF is properly implemented, I wouldn't want to override its routing, especially if it can work for other things, such that other modules may use PhoneNumberProvider.

Our system is based on telephone field. If we allowed other fields, then we may have to write code specifically on how to get/set field values (especially multi value/column fields). We cannot always expect to get/set a phone number with $entity->yourphonefield.

Indeed, that's why I think a hook / event is needed, one for allowed field types, and one for pulling number from the field. In the case of mobile number the latter won't be needed because it uses 'value' property similarly to telephone, while the other properties can be ignored.

Even if our system allowed multiple fields, why would it need a field name param? Why couldnt it just get all of them? I feel callers to getPhoneNumbers() shouldn't have to care about what field its getting data from.

In the case of 'Telephone', one field may be for a mobile number while another for a landline, ie 'work number', etc. For Mobile Number it may be a backup phone number or something similar. In both cases the order of field appearance in the entity (alphabetical?) may not correspond with main number being first, and you wouldn't want to send the sms to the other numbers. Or at least the return value for getPhoneNumbers() should specify the field name + delta for each number

Im not sure how you are going to handle setting phone numbers with multiple verification providers or multiple fields. sms_user already sets phone numbers in its account registration service, but assumes our verification system.

Yeah, you can't have both verification systems applied to the same field, which is why it should be separated from the more generic phone number config. It would seem the ideal place to put the verification settings is in the widget, this way the user has to choose either or. Although as it stands right now, my verification system can't work with telephone, and yours cant with Mobile Number although it's possible but the field settings would confuse the user because that is where the verification settings are. In the case of sms_user, again a hook/event would be useful here but not needed for Mobile Number because of the value property.

Phone number config doesn't have to support multiple fields per bundle, I think it actually simplifies things for the admin, but support for additional field types should be allowable with hooks.

Indeed, and thanks for taking my humble module into consideration. I will see how it can be integrated with the changes.

Tested patch on simplytest.me. No errors, but sms's do not seem to be sent.

Steps:
1) Install smsframework 8.x-1.x with patch, and telephone. Standard drupal profile.
2) Add telephone field to user
3) Create phone number config, only selecting bundle and field, no other changes, and save.
4) Go to admin user edit page, insert a phone number in telephone field (smsf's widget already applied).
5) Save user
6) Verification sms log does not appear in watchdog. Although it shows there are unverified numbers on admin/config/smsframework/phone_number

I tested with simpletest.me which creates a clean install Drupal environment in the cloud with a module of choice for testing. It uses only publicly available code, including the above patch.

Does the queue get cleared on cron? I tried running cron manually but that didn't help. I will try it on my dev as well. It could mean the module does not work with default config.

I'd done a quick review on the PR but forgot to submit it. Looks good.

Remaining task in this issue is implementation of the event.

This should not block the release of a beta then. I will go ahead and tag a beta by the end of tomorrow if nothing else comes up before then and if you don't beat me to it.

Still doing this in beta3? Also, phone number verification as a whole needs better documentation.

Sorry, I had been preoccupied with other matters and hadn't noticed you needed my input. The api looks good.