Now that we have #1174634: Add new HTML5 FAPI element: telephone landed for FAPI, we should really add the ability to have a phone number field type. It should probably have one column 'value' of type SQL varchar(255), and a field widget using the new HTML5 FAPI type, and a default formatter of just 'plain text' that outputs the raw value of the phone number.
Comment | File | Size | Author |
---|---|---|---|
#58 | telephone-cleanup-1740470.58.patch | 6.12 KB | larowlan |
#57 | 1740470-nitpicks-57.patch | 4.6 KB | Wim Leers |
#57 | 1740470-nitpicks_and_moved-57.patch | 19.66 KB | Wim Leers |
#54 | 1740470-53.patch | 482 bytes | jibran |
#50 | 1704864-50-telephone.patch | 9.51 KB | nick_schuch |
Comments
Comment #1
larowlanHere tis.
Comment #2
larowlanComment #3
Dave ReidAfter #1668332: Add an E-mail field type into core I think it's probably best if we split this out into a separate 'tel' module rather than re-use text module.
Comment #4
moshe weitzman CreditAttribution: moshe weitzman commentedSame here. IMO this is for Contrib
Comment #5
JohnAlbinI 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:
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)Which means a tel.module would be trivially simple. So I would slightly prefer this be in core instead of contrib.
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commentedOK, core is fine for me. Anyone up for coding it?
Comment #7
rbayliss CreditAttribution: rbayliss commentedSure.
Comment #8
patrickd CreditAttribution: patrickd commentedPatch #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
Comment #9
nils.destoop CreditAttribution: nils.destoop commentedI 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.
Comment #10
rbayliss CreditAttribution: rbayliss commentedThe 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!
Comment #11
patrickd CreditAttribution: patrickd commentedThanks for explanation and code :-)
seems legit, works, ready for me
Comment #12
falcon03 CreditAttribution: falcon03 commentedtagging for our accessibility team.
Comment #13
falcon03 CreditAttribution: falcon03 commentedAlso, 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?
Comment #14
rbayliss CreditAttribution: rbayliss commented@ 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?
Comment #16
patrickd CreditAttribution: patrickd commentedmad testbot, resetting to rtbc
Comment #17
mgifford@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.
Comment #18
patrickd CreditAttribution: patrickd commentedDoes it?
I think further validation can stay in contrib, I'd keep this as simply as it is
Comment #19
falcon03 CreditAttribution: falcon03 commented@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...
Comment #20
patrickd CreditAttribution: patrickd commented#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.
Comment #21
mgiffordIt'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.phpShouldn'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.
Comment #22
mgifford@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.
Comment #23
patrickd CreditAttribution: patrickd commented@mgifford
You need to enable 'telephone' module
Comment #24
patrickd CreditAttribution: patrickd commentedIf we want a tel-link we need to add another formatter
Comment #25
mgiffordI 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.
Comment #26
patrickd CreditAttribution: patrickd commentedokay, I'll give it a try
Comment #27
patrickd CreditAttribution: patrickd commentedI 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:
plain_text formatter is still the default one
Comment #28
webflo CreditAttribution: webflo commentedLooks really good. Just some minor things. I leave the status on NR to get some other reviews.
Is it a good idea to add
'#options' => array('external' => TRUE')
or should this depend ondrupal_strip_dangerous_protocols()
?Missing new line.
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.
Missing newline at eof.
Comment #29
patrickd CreditAttribution: patrickd commentedIs 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)
Comment #30
C-LogemannPatch 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?
Comment #31
webflo CreditAttribution: webflo commentedFound one wrong comment and add telephone_field_info_alter() to overwrite the default formatter if text module is available.
Comment #32
mgiffordPatch applies nicely. The telephone field now produces the
<a href="tel:+13174562564">317-456-2564</a>
as expected.Comment #33
penyaskitoNeeds tests. But it looks really nice :)
Comment #34
mgifford#31: telephone-1740470-31.patch queued for re-testing.
Comment #35
sunNice work. Would love to see this in core.
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.
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.
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?
$delta seems to be unused here.
It looks like $settings is not used here?
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?
&$info needs to be taken by reference.
Comment #36
larowlan@sun from http://tools.ietf.org/html/rfc3966
So yep, we have to strip whitespace, marking as needs work as a result.
Comment #37
sunIf 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.,Comment #38
larowlanthat's my reading too, however RFC's aren't always the easiest thing in the world to read ;)
Comment #39
nick_schuch CreditAttribution: nick_schuch commentedI have performed the recommendations as per #35 and #37. I have also setup tests.
Comment #41
nick_schuch CreditAttribution: nick_schuch commentedWoops. Resubmitting.
Comment #42
nick_schuch CreditAttribution: nick_schuch commentedComment #43
lliss CreditAttribution: lliss commentedI'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'
Comment #44
lliss CreditAttribution: lliss commentedHere's an interdiff for the patch above.
Comment #46
jibran#43: 1740470-43-telephone.patch queued for re-testing.
Comment #47
larowlanFirst 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:
We need this to go, standard is too slow. See comments below.
Once profile drops back to testing, add 'field', 'field_sql_storage' and 'node' here.
Before here add a $this->drupalCreateContentType(array('type' => 'article')); then we no longer need profile standard
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.this will return a node, so no need for the next line (and its extra query)
Comment #48
nick_schuch CreditAttribution: nick_schuch commentedThanks for the great advice larowlan! I have made the changes and its loads faster.
Comment #49
larowlanThis assert text seems wayward. Can we use 'Telephone link is output with whitespace removed' instead?
Sorry
Getting to very. minor. nitpicks. now.
Comment #50
nick_schuch CreditAttribution: nick_schuch commentedYep, not my best english. Cheers!
Comment #51
larowlanAssuming bot comes back green, I think this is RTBC!
Comment #52
Dries CreditAttribution: Dries commentedGreat. Committed to 8.x. Thanks!
Comment #53
Dries CreditAttribution: Dries commentedIt seems worthy a CHANGELOG.txt entry.
Comment #54
jibranCHANGELOG.txt entry
Comment #55
tim.plunkettAlso, MAINTAINERS.txt? See #1823042: Add maintainers and d.o components for all field type modules
Comment #56
jibranThere is no field type maintainers info in MAINTAINERS.txt
Comment #57
Wim LeersI 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 ofContains \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.
Comment #58
larowlanHere'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?
Comment #59
nick_schuch CreditAttribution: nick_schuch commentedI have volunteered to be the maintainer of telephone module.
http://drupal.org/node/1823042#comment-6957066
Comment #60
Wim Leers#58: D'oh. Thanks!
Comment #61
nick_schuch CreditAttribution: nick_schuch commentedThanks Wim Leers and Larowlan!!!
I have applied the patch from #58 and have rerun the tests and manually created the fields. All working!
Comment #62
nick_schuch CreditAttribution: nick_schuch commentedJumped the gun, tests haven't come back, will rtbc after they come back (green!).
Comment #63
nick_schuch CreditAttribution: nick_schuch commentedTests have gone green.
Comment #64
webchickCommitted and pushed to 8.x. Thanks!
Comment #65
pcambraWouldn't make sense here to add a setting for the size of the textfield just like the default text widget have?