Add a new phone number field type

Here tis.

Same here. IMO this is for Contrib

I was on the fence for this, but I thought about some criteria.

We need to migrate D6 profile data to normal entity/fields, so anything the Profile module had for its "fields" needs to be replicated in core. I just went and checked my D6 install and saw this list of possible fields:

• single-line textfield
• multi-line textfield
• checkbox
• list selection
• freeform list
• URL
• date

So I don't see a requirement that Telephone be in core from that aspect.

However, it would still be really useful since an actual <input type="tel"> widget would cause the appropriate telephone-based keyboard to appear on mobile devices. Also, there's no validation necessary since telephone numbers have no consistency internationally; see http://developers.whatwg.org/states-of-the-type-attribute.html#telephone...(type=tel)

The Telephone type does not enforce a particular syntax. This is intentional; in practice, telephone number fields tend to be free-form fields, because there are a wide variety of valid phone numbers.

Which means a tel.module would be trivially simple. So I would slightly prefer this be in core instead of contrib.

OK, core is fine for me. Anyone up for coding it?

Sure.

Patch #7 works as required

(though I wonder why mail and telephone modules have "dependencies[] = text" ?)

'default_widget' => 'tel_default',
Shouldn't the widget be prefixed with the full modules shortname telephone?

@Translation("Telephone Number"),
Number should be lowercase

I think the widget class name should be TelephoneWidget instead of TelephoneDefaultWidget.
The dependency to text has been deleted in the email module. This can be also for telephone, as the alter checks if text_plain exists.

// Edit
Dependency can't be removed, otherwise telephone wont have a formatter.

The text dependency is due to using the text_plain formatter. We could roll our own here, but what's the point? Text is a required module, and the text_plain widget works out of the box.

Corrected the widget name, and the capitalization error. Thanks!

Thanks for explanation and code :-)

seems legit, works, ready for me

tagging for our accessibility team.

Also, I would like to make a proposal (I am not a drupal expert developer). Since "telephone" doesn't use any validation rule, would it make sense to implement it as a widget on top of the "number" field?

@ zuuperman - This was a straight copy/paste from the email widget plugin conversion issue. I don't care either way, but it's consistent as TelephoneDefaultWidget.

@falcon03 - The number formatter would lose any formatting you enter. So if you entered (555) 555-5555, you'd end up with 5555555555.

One last question before this goes in... I chose a totally arbitrary 256 characters for storage. In hindsight, this should maybe be 255 for consistency with other fields. Does anyone else have an opinion?

mad testbot, resetting to rtbc

@patrickd - Doesn't this need a screenshot or something before it's marked RTBC?

The Phone module does a relatively good job of accessibility - http://drupal.org/project/phone

I'm glad to see that you are just using a single input field.

Does it?

I think further validation can stay in contrib, I'd keep this as simply as it is

@patrickd: could I ask the reason why we cannot support validation of phone numbers?

If the validation code is already written in the phone module, couldn't we move it to core?
I think that if a novice starts using a phone field, he/she will expect that some validation of the phone number for his/her country could be possible...

#5 JohnAlbin
The Telephone type does not enforce a particular syntax. This is intentional; in practice, telephone number fields tend to be free-form fields, because there are a wide variety of valid phone numbers.

I think if we really want validation into core, it should be reliable and consistent all over the world, no matter where drupal is used.
As telephone numbers are very inconsistent all over the world - we can not guarantee reliability for validation.
If someone really needs validation -> go into contrib and check whether there's a module that validates the number correctly for his purposes.

It's a UI thing on some levels. Good to have the visuals in principal, if nothing else.

I did try the patch. It applied nicely, but in trying to add it to an existing content type I didn't find it in the list.

So this essentially is just adding the attribute type="tel" to the input form as per http://www.html5tutorial.info/html5-contact.php

Shouldn't the HTML output follow:
http://demosthenes.info/blog/536/Adding-Phone-Numbers-To-Web-Pages-With-...

At at least be wrapped in <a href="tel:+13174562564">317-456-2564</a>?

I know that if I enabled a telephone module on a mobile ready version of Drupal I'd expect the output to be something other than plain text wrapped in a div.

@falcon03 - having looked at how the two competing contrib phone modules have attempted to deal with validation, I'd say this isn't something that can be brought into core. There's a lot of variety and I don't think that either of the two modules out there now are really doing a good job providing a global solution.

@mgifford
You need to enable 'telephone' module

If we want a tel-link we need to add another formatter

I think it's worth adding a formatter for tel-link.

Ya, on enabling the module. Missed that the first time, but did get it by #21.

okay, I'll give it a try

I thought about using the link-fields formatter for a sec, but it's formatter settings don't really fit to telephone fields needs.

Added a TelephoneLinkFormatter formatter class.
id: "telephone_link"
label: "Telephone link"

Formatter option:

type: 'textfield',
title:  'The links title to display',
description: 'Leave blank to use the telephone number itself instead.',

plain_text formatter is still the default one

Looks really good. Just some minor things. I leave the status on NR to get some other reviews.

+++ b/core/modules/field/modules/telephone/lib/Drupal/telephone/Plugin/field/formatter/TelephoneLinkFormatter.phpundefined
@@ -0,0 +1,105 @@
+      $element[$delta] = array(
+        '#type' => 'link',
+        '#title' => $item['title'],
+        '#href' => $link,
+      );

Is it a good idea to add '#options' => array('external' => TRUE') or should this depend on drupal_strip_dangerous_protocols()?

+++ b/core/modules/field/modules/telephone/lib/Drupal/telephone/Plugin/field/formatter/TelephoneLinkFormatter.phpundefined
@@ -0,0 +1,105 @@
\ No newline at end of file

Missing new line.

+++ b/core/modules/field/modules/telephone/telephone.infoundefined
@@ -0,0 +1,7 @@
+dependencies[] = text

+++ b/core/modules/field/modules/telephone/telephone.moduleundefined
@@ -0,0 +1,35 @@
+      'default_formatter' => 'text_plain',

We removed the dependency to text module for the email field in #1770172: Convert email module widgets and formatters to Plugin system. Just look at the last patch. Should be done here too.

+++ b/core/modules/field/modules/telephone/telephone.moduleundefined
@@ -0,0 +1,35 @@
+}

Missing newline at eof.

Is it a good idea to add '#options' => array('external' => TRUE') or should this depend on drupal_strip_dangerous_protocols()?
Your right, any protocols entered as telephone number haven't effects as they get prefixed with 'tel:'

Removed dependency on text_plain and set telephone_link formatter as default one
Though I left the telephone_field_formatter_info_alter() to add telephone field to text_plain formatter (just as it is the case in the email patch too)

Patch of #29 seems to work as suggested.

Question: Do we need a dummy project "telephone" on d.o to avoid namespace conflicts with contrib modules?

Found one wrong comment and add telephone_field_info_alter() to overwrite the default formatter if text module is available.

Patch applies nicely. The telephone field now produces the <a href="tel:+13174562564">317-456-2564</a> as expected.

Needs tests. But it looks really nice :)

#31: telephone-1740470-31.patch queued for re-testing.

Nice work. Would love to see this in core.

+++ b/core/modules/field/modules/telephone/lib/Drupal/telephone/Plugin/field/formatter/TelephoneLinkFormatter.php
@@ -0,0 +1,106 @@
+  public function settingsForm(array $form, array &$form_state) {
+    $elements['title'] = array(
+      '#type' => 'textfield',
+      '#title' => t('The links title to display'),
+      '#default_value' => $this->getSetting('title'),
+      '#description' => t('Leave blank to use the telephone number itself instead.'),
+    );

Hm. The label for this configuration option could use some love. An optimal label will make the description entirely obsolete.

Let me provide a first suggestion, which you can optimize further:

"Link text to show instead of telephone number"

I also considered

"Show static link text instead of telephone number"

...but that sounds more like a checkbox label.

Also, I think we should rename "link title" everywhere to "link text" (also in the settings summary), since that is how it's called formally.

+++ b/core/modules/field/modules/telephone/lib/Drupal/telephone/Plugin/field/formatter/TelephoneLinkFormatter.php
@@ -0,0 +1,106 @@
+    else {
+      $summary = t('Telephone number as link title.');
+    }

I'm not sure whether the else case is necessary to output in the formatter summary - thus far, I think we only output customized, non-default settings in the formatter summary, and the phone number itself without custom title is the default presentation.

+++ b/core/modules/field/modules/telephone/lib/Drupal/telephone/Plugin/field/formatter/TelephoneLinkFormatter.php
@@ -0,0 +1,106 @@
+  public function prepareView(array $entities, $langcode, array &$items) {
+    foreach ($entities as $id => $entity) {
+      $settings = $this->getSettings();

Do we need to retrieve $settings for every entity? I think they are always the same, so this line can be moved before the loop?

+++ b/core/modules/field/modules/telephone/lib/Drupal/telephone/Plugin/field/formatter/TelephoneLinkFormatter.php
@@ -0,0 +1,106 @@
+      foreach ($items[$id] as $delta => &$item) {

$delta seems to be unused here.

+++ b/core/modules/field/modules/telephone/lib/Drupal/telephone/Plugin/field/formatter/TelephoneLinkFormatter.php
@@ -0,0 +1,106 @@
+    $settings = $this->getSettings();

It looks like $settings is not used here?

+++ b/core/modules/field/modules/telephone/lib/Drupal/telephone/Plugin/field/formatter/TelephoneLinkFormatter.php
@@ -0,0 +1,106 @@
+      // Prepend 'tel:' to the telephone number.
+      $link = 'tel:' . $item['value'];

1) I'm not familiar with browser support for tel: — don't we have to strip all white-space from the value? (and potentially other characters, such as ()-_/, too?)

2) Can we rename the $link variable to $href?

+++ b/core/modules/field/modules/telephone/telephone.module
@@ -0,0 +1,45 @@
+function telephone_field_info_alter($info) {

&$info needs to be taken by reference.

@sun from http://tools.ietf.org/html/rfc3966

"tel" URIs MUST NOT use spaces in visual separators to avoid excessive escaping.

So yep, we have to strip whitespace, marking as needs work as a result.

If I get RFC 3966 right, then we also need to rawurlencode() the telephone number value in $item['value'], before it is assigned to $href; i.e.,

  $href = 'tel:' . rawurlencode(preg_replace($item['value'], '/\s+/', ''));

that's my reading too, however RFC's aren't always the easiest thing in the world to read ;)

I have performed the recommendations as per #35 and #37. I have also setup tests.

Woops. Resubmitting.

I've reviewed the code and tested the functionality. This looks pretty good. I altered some of the Test file just for code consistency and to get rid of reference to 'und' in favor of 'LANGUAGE_NOT_SPECIFIED'

Here's an interdiff for the patch above.

#43: 1740470-43-telephone.patch queued for re-testing.

First up great work for kicking this along again, especially adding the tests.

Some minor changes to the test to increase speed and improve coverage as follows:

+++ b/core/modules/field/modules/telephone/lib/Drupal/telephone/Tests/TelephoneFieldTest.phpundefined
@@ -0,0 +1,84 @@
+  protected $profile = 'standard';

We need this to go, standard is too slow. See comments below.

+++ b/core/modules/field/modules/telephone/lib/Drupal/telephone/Tests/TelephoneFieldTest.phpundefined
@@ -0,0 +1,84 @@
+  public static $modules = array('telephone');

Once profile drops back to testing, add 'field', 'field_sql_storage' and 'node' here.

+++ b/core/modules/field/modules/telephone/lib/Drupal/telephone/Tests/TelephoneFieldTest.phpundefined
@@ -0,0 +1,84 @@
+
+    $this->article_creator = $this->drupalCreateUser(array('create article content', 'edit own article content'));

Before here add a $this->drupalCreateContentType(array('type' => 'article')); then we no longer need profile standard

+++ b/core/modules/field/modules/telephone/lib/Drupal/telephone/Tests/TelephoneFieldTest.phpundefined
@@ -0,0 +1,84 @@
+    $edit = array(
+      'title' => $this->randomName(),
+      'field_telephone[' . LANGUAGE_NOT_SPECIFIED . '][0][value]' => $value,
+    );
+
+    $this->drupalPost('node/add/article', $edit, t('Save'));
+
+    $node = $this->drupalGetNodeByTitle($edit['title']);

We need to duplicate this hunk and create a second node with $value '1234 56789', the assert should verify that the output value is <a href="tel:123456789"> ie with the whitespace stripped.

+++ b/core/modules/field/modules/telephone/lib/Drupal/telephone/Tests/TelephoneFieldTest.phpundefined
@@ -0,0 +1,84 @@
+    $this->drupalPost('node/add/article', $edit, t('Save'));

this will return a node, so no need for the next line (and its extra query)

Thanks for the great advice larowlan! I have made the changes and its loads faster.

+++ b/core/modules/field/modules/telephone/lib/Drupal/telephone/Tests/TelephoneFieldTest.phpundefined
@@ -70,15 +74,22 @@ function testTelephoneField() {
+    $this->assertRaw('<a href="tel:123456789">', 'A telephone link is stripping whitespace for the link value.');

This assert text seems wayward. Can we use 'Telephone link is output with whitespace removed' instead?

Sorry
Getting to very. minor. nitpicks. now.

Yep, not my best english. Cheers!

Assuming bot comes back green, I think this is RTBC!

Great. Committed to 8.x. Thanks!

It seems worthy a CHANGELOG.txt entry.

CHANGELOG.txt entry

Also, MAINTAINERS.txt? See #1823042: Add maintainers and d.o components for all field type modules

There is no field type maintainers info in MAINTAINERS.txt

I don't think it was intentional that this module was committed as a submodule of field.module? Because every other field type module is now a top-level module.
There was also a bunch of weird whitespace and Definition of Drupal\… instead of Contains \Drupal\… etc things.

In short: almost all of these are small conventions that have changed ever since the email module got added, and since this code was developed by looking at the email module, these nitpicky problems exist. First attached patch only fixes code nitpicks, the second also does the move.
Not sure if this should be in this issue or a follow-up issue, so just posting here.

Here's @Wim's patch with the move as renames as well as the patch from #53
Do we need/want a change notice for this one?

I have volunteered to be the maintainer of telephone module.

http://drupal.org/node/1823042#comment-6957066

#58: D'oh. Thanks!

Thanks Wim Leers and Larowlan!!!

I have applied the patch from #58 and have rerun the tests and manually created the fields. All working!

Jumped the gun, tests haven't come back, will rtbc after they come back (green!).

Tests have gone green.

Committed and pushed to 8.x. Thanks!

Wouldn't make sense here to add a setting for the size of the textfield just like the default text widget have?