Because I separately wrote code to create a phone-7.x-2.x branch (drupal.org/sandbox/nephele/1928666), I inevitably made multiple different decisions about various details of the code. I thought it would be best to start to a separate issue to ask various resulting questions. These are probably mostly going to be obscure details that only cdale and I have thought about.

So, starting with the guts of the schema:

  • I opted to make all of the fields have fixed sizes, whereas cdale has made the field sizes be set by the user as a field setting. I think most users have no idea when they first install the module about what sizes make sense for fields like the phone number. Furthermore, what happens if users set the field to a size that is unrealistically small, then can't fix the field size when they run into problems? Are there any real advantages to allowing users to set number_size and extension_size?
  • Is $settings['number_size'] also used to determine the size of the input textfield for phone numbers? I can't immediately find it in cdale's code, however, this issue is another reason why I decided to eliminate the number_size setting. The input textfield must be larger in size than the schema text string, because users expect to be able to enter the number with spaces/punctuation, e.g. as "(555) 555-5555" instead of "5555555555" -- this problem exists in phone-7.x-1.x.
  • I thought that the schema should include indexes for country code and the phone type, since they might realistically be used as filters.
CommentFileSizeAuthor
#2 phone-schema-1928688-2.patch8.09 KBcdale
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cdale’s picture

From the other issue.

Things I can see right away that your code has that mine does not, or that is different/might cause problems in regards to migrate code:

- Support for the content_migrate module.
- Your feeds implementation provides an "all as one field" option, however it does not seem to support the type?
- Your use 'type' for the number type, I originally used 'comment', however there is a patch at #1928382: Rename "comment" field that currently looks at changing this to numbertype.
- You add indexes in your schema. Probably should have done that.
- You have an upgrade path for phone 7.1.x
- Your token implementation looks a lot cleaner and easier to grok than mine, though I based my code off what was in the name module. I like your implementation much better here.
- You allow adjusting the position of the country name and type in the formatter, as well as customising the extension prefix, where as I just rely on libphonenumber to format the extension.
- I like the labels in your commented out hcard_phone_types() list better than mine
- Your schema seems to suggest that multiple types are supported per number. Is this the case? It didn't seem to be implemented. If it is, this is also a nice to have difference I think.
- You have a locater module included to set country based on IP. My question on this, is what if the country detected is not in the allowed countries list?
- You have cck_convert support
- You have migrate support (or have started) for other versions of the phone module.

This is what I can see from scanning the code. There might be some items I've missed. Hopefully it provides a starting point.

cdale’s picture

I think you're right about the size of the schema field vs the widget size. At present, that field setting is used for both.

I think your points are valid, and that it'd be a good idea to have the schema fixed, but allow the widget size to change.
As pointed out, indexes are always good, in-particular for views support.

I've attached a patch for those points because I do agree with them. This patch will require #1928382: Rename "comment" field first however.

Nephele’s picture

Responding to some specific points:
- Feeds implementation. I think the all-in-one-field option is covered in phone_feeds_set_target with "if (strpos($target, ':')===FALSE)" and then "if (!isset($sub_field))", but I haven't done any testing of it yet and it's been a couple years since I worked with feeds.
- Token implementation. I've done basic tests of the tokens and it works. I know it's a different approach than some of the other field modules, but it's what I came up with for some custom modules I've written. I don't know how much of the complexity in other modules derives from taking drupal6 code and upgrading it (plus including backwards-compatible support).
- I've seen #1928382: Rename "comment" field, and agree that having a field specifically intended for the phone type (with future support for hcard, etc) makes more sense than merging the information into comment.
- I have a lot of ideas for how to follow up with the phone number type and hcard integration, but it was starting to turn into too much of a digression into a feature that wasn't immediately needed. So that's why there are commented-out hcard functions. And at the moment there is no support for multiple types, just a theoretical ability to combine them into one field without having to make schema changes later.
- Locator module. I didn't do much more than take a useful-looking patch from a cck_phone issue and make cosmetic changes. Your point about handling non-allowed countries is a good one, and should be incorporated.

I think the migrate/update code I've written is probably what would be most useful to get into your code, so that existing users have some ability to start experimenting with this branch. I'll follow up more at #1928420: Upgrade path.

Also, FYI, I understand your motivation for just diving in writing code -- I was in the same place a couple weeks ago. It's just bad luck that our timing was so similar! But our work doesn't all overlap, so I think once we sort out / merge our efforts the result will be a much stronger module. Assuming we don't get too lost in dozens of overlapping/contradictory/conflicting patches...

cdale’s picture

While were here, I'm not 100% happy with how the separation of field/instance/widget settings currently works. Here's a summary of what they currently are, and what I'm thinking they should be.

Current settings
- Field settings
-- numbertype_allowed_values_type
-- numbertype_allowed_values

- Widget settings
-- country_options
-- enable_default_country
-- default_country
-- all_country_codes
-- country_codes
-- hide_single_cc
-- country_selection
-- country_code_position
-- enable_numbertype
-- numbertype_allowed_values_position
-- enable_extension
-- number_size
-- extension_size
-- use_tel_input

Proposed settings
- Field settings
-- numbertype_allowed_values_type
-- numbertype_allowed_values

- Instance settings
-- enable_extension
-- enable_numbertype

- Widget settings
-- country_options
-- enable_default_country
-- default_country
-- all_country_codes
-- country_codes
-- hide_single_cc
-- country_selection
-- country_code_position
-- numbertype_allowed_values_position
-- number_size
-- extension_size
-- use_tel_input

Thoughts? The only reason I think the enable options should be on the instance, is as they also affect the formatters. It's kind of a small thing I know, but I'm curious what other peoples thoughts are. All the other widget options only affect the output/display/options in the widget itself.

Nephele’s picture

First, another difference with my module is that I have two different widgets. Basically one is more phone-7.x-1.x style (a single text input to type in the entire number) and the other is more cck_phone-7.x-1.x style (separate inputs for country code, number, extension). Which influences my thinking about what settings belong in the widget -- only settings that apply to a single widget are implemented as widget settings.

Second, I also have formatter-level settings, although I think those are settings that are unique to my code.

Third, I think field settings should be limited to settings that influence the database schema or are tied to the cardinality. I've cursed when I've tried to re-use an existing field on a new content type only to discover some minor difference that was coded at the field level, forcing me to create a whole new redundant field unnecessarily.

So the upshot is I've put nearly everything in instance settings. I think the country information is "bigger picture" than the widgets. The default country determines which numbers are local and which are international, therefore affecting formatting. The allowed countries control what information is contained in the field -- so the field might be a "US phone number" field or an "Other countries" field (and incidentally, the field's label and description are set at the instance level -- is that another way to think about what belongs in the instance?).

In my case, the only widget setting is country_code_position, but it looks like you've added some other settings that are also widget-specific: number_size, extension_size.

Also, I considered whether to move hide_single_cc to be a widget setting (because it only applies to my phone_combo, not my phone widget), but logically separating it from the other instance settings was too awkward.

So those are my off-the-cuff thoughts.

Nephele’s picture

I want to put a possibly crazy idea out here because I think it needs to be considered.

As background, I think cdale and I were each able to take phone_cck and completely revamp the code relatively quickly because we tackled it as a complete rewrite. We didn't have to work out separate patches for each separate issue, or even discuss the issues. Instead we made arbitrary decisions, knowing that any problems could be discussed and fixed later, after the overall code structure was was in place.

Now, we need to merge the two versions, and I'm doubting whether it's possible to do that with a series of issues, discussions, and patches -- at least without having the process take months because every patch will impact a dozen other, overlapping patches. We've made a huge amount of progress in the last few days, and I'd hate to see the progress grind to a halt over the mechanics of how to merge the versions.

I'm wondering whether the most effective way forward is for either cdale or me to again do a complete rewrite of the module, or at least a major rewrite, this time to create a merged version combining the features offered by each of our separate versions. Then post the new version in our sandbox, or else give cweagans a mega-patch that basically overwrites most of what's currently in 7.x-2.x.

cdale’s picture

I'm not opposed to that idea. I'd like cweagans input though I think seen as he is the one making the commits. :) We've both obviously got our own little issues that we want to see solved, it's just a question of which path to take and what issues or features if solved/added, would actually be acceptable for the module.

I agree about field settings, it can be a real pain, but the type options are closely related to the data, which is why they belong in the field settings. This also matches what core does in the list module.

The only items I can see that are really outstanding, is the merging of this "single widget", and the general upgrade stuff, is that fair to say?

The upgrade stuff I didn't even touch, so that should be easier to handle. It's going to be more the extra features you've added.

In regards to the single vs combo widget however, couldn't that be a widget setting that disables the country dropdown, much like the extension or numbertype? The rest of the code bar an error message that might need updating will work and handle that just fine. It would also still allow the "country selection" to be done per widget, allow a single widget that only allows certain countries.

Nephele’s picture

Re: single vs combo widget. It could also be handled by widget settings. However, I think it's cleaner having two separate widgets. For example, if the single vs combo choice is changed, most of the other settings need to be refreshed (is country_code_position needed? what is a good default value for number_size? is extension_size needed?). That's easy to handle if the decision is made when you select the widget, but requires ajax calls if it's made on the settings form.

Re: field settings. I don't see why numbertype_allowed_values is considered to be more closely related to the data than the allowed countries. Plus, it's confusing having numbertype_allowed_values be a field setting when enable_numbertype is an instance setting -- it means that enable_numbertype has to be true by default (whereas I think it should be false by default), and users are forced to make decisions about numbertype_allowed_values before having the option of turning the feature off. If the concern is potential loss of data, there are other ways to handle that. There are significant differences between numbertype and a list field (numbertype is an optional part of the field, not the entire list field; hcard means that there are a limited number of options for numbertype).

Re: outstanding issues. I think the general upgrade stuff is the biggest issue. But the upgrade code needs to be edited to match the available settings (and whether they are field/instance/widget/etc settings), the schema, the field names, etc., which I suppose is what's leading to my sense of paralysis.

cdale’s picture

The #states stuff in D7 is a great way to handle changing settings without the need for an ajax call. Turning off countrycode would disable all country related stuff, and if people still want a separate extension field, I don't see why we would not let them. Making a new widget would not allow that use case. I personally think a single widget provides much more flexibility and allows people to have things set up how they want.

I copied the allowed values stuff from core, and where possible, I like to follow core. The main reason it's a field setting from what I could see, is to allow them to actually hook in with $has_data when the settings are validated, and see if they're going to cause loss of data. I agree that it's confusing having the enable option in a different place to the actual settings. Perhaps if we just move the enable option down. The only time core disallows the saving of field settings, is when it detects a schema change, so as long as we don't do that, it should be fine. And I don't see the number type being that different to a list field. We still provide a custom option, and it's still data integrity, whether it's a sub-field, or the whole field.

I'd like cweagans input on this one. It seems we disagree a little on how this should be used, and as he's the one who will be committing, I say we let him decide. :)

cweagans’s picture

I'm sorry...I don't have time to go through the entire issue right now. One of you guys want to write a quick summary of what needs my feedback? I'll answer ASAP.

cdale’s picture

Difference of opinion.

1. Nephele wrote two widgets: A single widget that collects the whole number, and a second widget that is like what is in 2.x now. (all the subparts). I feel that a single widget with the option to disable country collection (as libphonenumber will work provided +1/+61/+44 etc.. is provided). This would also enable more flexibility in what people actually want in a widget, and still allow country level restriction. I'm not sure what benefits a second single widget would provide over this. Thoughts? Nephele?

2. Currently, number type allowed values are in field settings. I did this following the list field allowed values. Nephele's point about the "enable number type" option being in the widget settings confusing is valid, and I agree. I feel moving the enable option into the field settings is the best option to combat this. Nephele feels the type list of options should be moved to instance settings and data integrity handled some other way, instead of how the core list field module does it. The main argument here, is how is country collection information further away from the data than the type. I believe that the country selection stuff merely controls what countries are available. A country is always stored regardless, and the code that gets stored is already defined. Whereas for the number type, the user is defining what key actually goes into the database.

I'm not sure I've summarised enough, and my view is obviously biased. I think the single widget is the way to go, but I'm in two minds about where to keep the numbertype options. Field or instance? Currently field as that's what core does with allowed values, granted they are allowed values for whole fields, not sub/part/optional fields.

Nephele’s picture

One other question that we could use extra input on is:
- Should we merge the modules one patch at a time, or do one last revamp-of-the-whole-code to get a merged module (see #6)?

I'm really OK with any decision that gets made -- especially in the interest of moving forward with merging the modules as quickly as possible. I was just trying to provide my feedback on the choices that I made and why.

The widget choice is nearly a coin-toss. Both approaches can work, and it's probably personal preference as much as anything.

As far as settings, I prefer to not put settings at the field level that are unrelated to the schema, because it's too inflexible. Overall, I think most of the settings belong at the instance level. But ultimately just a choice that needs to be made one way or the other before the upgrade functions can be rewritten -- because the upgrade functions update all of the settings.

cweagans’s picture

This would also enable more flexibility in what people actually want in a widget, and still allow country level restriction. I'm not sure what benefits a second single widget would provide over this. Thoughts? Nephele?

I think I'd be okay with two separate widgets. The only condition there is that the all in one text field would need to be smart enough to figure out the different parts of the phone number for a lot of different inputs (i.e., we need to test the hell out of that with all sorts of weird input).

About the instance settings vs widget settings, I don't have an opinion other than the allowed values for the comment dropdown should not be editable when there is data in the field. That's definitely a data integrity issue, and I don't want to allow people to shoot themselves in the foot (and the people that want to do it anyways generally have the technical knowledge to do it anyways).

Should we merge the modules one patch at a time, or do one last revamp-of-the-whole-code to get a merged module (see #6)?

Well, there's advantages to both approaches. If you guys want to copy the git repository over into a sandbox, you guys can both commit, and when you're ready for me to take a look, I can just pull in all the changes in the sandbox. Does that work for you? My only stipulation is that there needs to be fairly descriptive commits (one commit per change, etc). Does that help? Or would you rather do it differently? I know you guys do good work, so I'm willing to work with you on a way to get your work into phone module fairly quickly.

Thanks for your continued work on this! You guys rock!

cdale’s picture

I think I'd be okay with two separate widgets. The only condition there is that the all in one text field would need to be smart enough to figure out the different parts of the phone number for a lot of different inputs (i.e., we need to test the hell out of that with all sorts of weird input).

Libphonenumber takes care of that for us already, so an all in one widget is easy to do, as libphonenumber allows to extract parts. So you can enter +61112345678;ext=555 and we can get back countrycode = AU, number = 112345678, extension = 555.

We can make it into two widgets, though I was just about to push a commit that allowed disabling country selection dropdown. :)

I've been making some updates in my own sandbox, and was about to commit a first run of Nephele's update patch. Happy to git Nephele commit access to my sandbox and we can go from there.

cdale’s picture

@Nephele: I've give you VCS access to http://drupal.org/sandbox/cdale/1925578.

I think we work there to get something up and running quickly, and then get cweagans input when we think it's good. We could probably use the issue queue on the sandbox for extra question too, to avoid noise here.

I've just pushed up a bunch of stuff, including your initial update patch. I'm not too fused on the code, so long as the general functionality remains, so have at any re-factoring that you think is useful, single/combo widgets etc. One key feature I had that you did not, was the ability to input/output alpha/vanity numbers (1800 CALL ME). This is something I've needed, and libphonenumber supports. The only point of note around them, is that I've disallowed extensions for vanity numbers. Personally I think that's acceptable. It's a vanity number. If you want the extension, then just enter the actual number. Aside: The tel: links always print out the real number in the href.

Nephele’s picture

Thanks! I've reciprocated in my sandbox, although we probably want to stick to just working on the one sandbox. And using your sandbox's issue queue for more obscure coding questions is a good idea, too.