Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

My thoughts: Implement a *very basic* plain-text field (using SQL varchar(254) as per http://stackoverflow.com/questions/386294/maximum-length-of-a-valid-emai...), and add a default widget of html5 email using the new '#type' => 'mail' FAPI type, and plain text default formatter (and a mailto: formatter).

Dave Reid’s picture

Issue tags: +html5
rbayliss’s picture

Here's the most basic implementation. Wasn't sure whether this should be added to the text module or put on it's own.

rbayliss’s picture

Status: Active » Needs review
Dave Reid’s picture

Wow, awesome! We should make this module depend on text module explicitly. As per #1696946: Rename field modules to [type]_field I'm inclined to say we should namespace the module as 'email_field' as well.

rbayliss’s picture

Makes sense. What kind of test coverage do we need for this?

rbayliss’s picture

How about a version with no newline errors?

geerlingguy’s picture

Status: Needs review » Needs work

I like the idea of just having a really, really simple email field, but unless I'm mistaken, there's no validation (like, using valid_email_address()) when saving a new email address in an email field with this patch... I think that would be a minimum requirement, and the only other thing I'd want to see out of a core email field.

rbayliss’s picture

Status: Needs work » Needs review

Validation happens in the email form element, so no field level validation is required. More specifically, it happens in form_validate_email().

geerlingguy’s picture

Oh I see; I didn't notice all those new theme_[element] and form_validate_[element] bits in form.inc—when were those added? (And, in that case, I think this is good to go, would to RTBC, but I'm wondering if we need some tests (re: comment #7).)

sun’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Wow, thanks for jumping on this issue, @rbayliss!

I just had a brief look at email module's code, and it seems like we're covering the main aspects here. :)

Two major issues:

1) I think we should name the module just "email" for the time being and leave a potential later rename to #1696946: Rename field modules to [type]_field. There doesn't seem consensus on a new pattern yet, so the current pattern in HEAD would be just "email".

2) I'm not sure whether it is a good idea to continue adding field type modules into core/modules/field/modules/... and I actually have no idea why we started that in the first place. File, Image, and Taxonomy modules are good counter-examples. But then again, field-type-only modules are currently located within field module currently, so we might want to continue that for the time being.

Otherwise, this looks pretty much ready to fly, but there are some minor issues in the code:

+++ b/core/modules/field/modules/email_field/email_field.info
@@ -0,0 +1,7 @@
+description = Defines a field type for email addresses

Let's add a trailing period here.

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

+++ b/core/modules/field/modules/email_field/email_field.module
@@ -0,0 +1,87 @@
+      'default_formatter' => 'text_plain',

The only reason for the hard dependency on Text module seems to be the default formatter, which looks unnecessary to me.

What do you think of removing the dependency in favor of a conditional module_exists('text') default_formatter in email_field_info()?

That would be sorta in line with how the email field type is injected for text_plain in email_field_formatter_info_alter() in this patch.

+++ b/core/modules/field/modules/email_field/email_field.install
@@ -0,0 +1,17 @@
+<?php
+
+/**

+++ b/core/modules/field/modules/email_field/email_field.module
@@ -0,0 +1,87 @@
+<?php
+
+/**

Let's add the standard @file phpDoc blocks here. You can probably copy them from other core module files (there's standard description per file type AFAIK).

+++ b/core/modules/field/modules/email_field/email_field.install
@@ -0,0 +1,17 @@
+    'value' => array(

I wonder whether it wouldn't make more sense to use the key/column 'email' instead of 'value'? Do we have a standard for this?

+++ b/core/modules/field/modules/email_field/email_field.install
@@ -0,0 +1,17 @@
+      'type' => 'varchar',
+      'length' => 254,

Let's add a code comment here that explains the limit of 254 and also contains a final reference to the proper RFC (text to be wrapped properly at 80 chars, @see links may exceed that limit):

// The maximum length of an e-mail address is 254 characters. RFC 3696 specified 320, but will be adjusted.
// @see http://tools.ietf.org/html/rfc3696#section-3
// @see http://www.rfc-editor.org/errata_search.php?rfc=3696&eid=1690
+++ b/core/modules/field/modules/email_field/email_field.module
@@ -0,0 +1,87 @@
+      'settings' => array(),
+      'instance_settings' => array(),
...
+      'settings' => array(),
+      'behaviors' => array(
+        'multiple values' => FIELD_BEHAVIOR_DEFAULT,
+        'default value' => FIELD_BEHAVIOR_DEFAULT,
+      ),

AFAIK, those empty default arrays and default behaviors do not need to be specified, so I think we can remove them.

+++ b/core/modules/field/modules/email_field/email_field.module
@@ -0,0 +1,87 @@
+      'label' => t('Mailto Link'),

"link" should be lowercase here, I think?

+++ b/core/modules/field/modules/email_field/email_field.module
@@ -0,0 +1,87 @@
+      '#href' => 'mailto:' . $item['value']

Missing trailing comma for last array element.

Lastly, we also need to add some tests for asserting the base expectations for the email field. I guess those can really be very basic, since all the validation and other stuff is tested for the Form API #type email already.

moshe weitzman’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests

this is so nice and simple. i don't even see anything that really merits a test.

xmacinfo’s picture

Status: Reviewed & tested by the community » Needs work

Resetting proper status.

Devin Carlson’s picture

Status: Needs work » Needs review
FileSize
3.25 KB
3.71 KB

An updated patch to address the coding concerns from #12.

Not addressed:

  • Dependency on the Text module.

Adding a default "email" formatter should only take ~9 lines and would allow the removal of hook_field_formatter_info_alter() but I'm not familiar with default formatter best practices.

I also left the key/column as 'value' instead of 'email' as all of the other field modules use 'value'.

rbayliss’s picture

The 'value' key is required to use the text module's plain text formatter. I'll do whatever makes the most sense here, but it seems confusing to have the default formatter change depending on the status of another module (even if it is a required one). I'd say let's either have this module depend on text and use the text_default formatter as we currently are, or write an email specific plain text formatter and skip the text module altogether.

Attached patch is a reroll of #15 with the module name switched back to 'email'. No interdiff because it would be longer than the actual patch.

sun’s picture

Status: Needs review » Needs work

Thank you, @rbayliss!

I think we can discuss the Text module dependency in a separate follow-up - no need to hold up this nice patch for that :)

Now we're really almost ready to go -- just gave this another last look, and after fixing the following really minor issues, we're RTBC:

+++ b/core/modules/field/modules/email/email.info
@@ -0,0 +1,7 @@
+name = Email
+description = Defines a field type for email addresses.

Unless I'm mistaken, then our user interface text standard is "E-mail" (with hyphen). Let's make sure that we're consistently using that style/form everywhere in this patch (including PHP code comments).

+++ b/core/modules/field/modules/email/email.install
@@ -0,0 +1,28 @@
+ * Install, update and uninstall functions for the email_field module.

s/the email_field module./the E-mail module./

Can you quickly adjust the code for these two? :)

rbayliss’s picture

Status: Needs work » Needs review
FileSize
1.85 KB
3.56 KB

Sounds good. How's this?

Status: Needs review » Needs work

The last submitted patch, interdiff.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community

Awesome! Thanks! :)

(guess it will come back green)

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Wow, this is GREAT! Can't wait to commit this!

However, we need a few tests.

webchick’s picture

Oh. And we need a hook_help(). Copy/pasting one of the other field modules' hook_help() and adjusting slightly should be fine.

Moshe asked for clarification on the test. I picture something that creates a new entity with an email field, fills it out, submits it, and verifies the email address shows up formatted correctly.

rbayliss’s picture

Status: Needs work » Needs review
FileSize
3.81 KB
7.03 KB

How does this look?

rbayliss’s picture

This time without the comments from NumberFieldTest.

Status: Needs review » Needs work

The last submitted patch, core_email_field-1668332-24-interdiff.patch, failed testing.

rbayliss’s picture

Status: Needs work » Needs review
FileSize
1.31 KB
7.02 KB

Same as #24, just renamed.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Awesome! :)

swentel’s picture

+++ b/core/modules/field/modules/email/lib/Drupal/email/Tests/EmailFieldTest.phpundefined
@@ -0,0 +1,90 @@
+    //Check that the link is displayed:

Extreme nitpick maybe here.
Add a space between '//' and Check and use a dot at the end of the sentence.

I'll leave it RTBC though, maybe I'm to hard here :)

sun’s picture

Quickly helping you out there. Let's get this in! :)

geerlingguy’s picture

Another RTBC here. Everything works great, and is super-simple.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Awesome!

I'm taking the opportunity, since we are ever-so-briefly below thresholds, to add this most excellent feature! Great work, Rob!

Committed and pushed to 8.x. :D YAY!

Could we get a quick CHANGELOG.txt entry for this?

swentel’s picture

Status: Needs work » Needs review
FileSize
465 bytes

Changelog entry

pingers’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me :)

mh86’s picture

yeah, great to have the email field in core now :-)

mh86’s picture

Concerning the upgrade path from D7, should this be done in core or contrib? As far as I see only the database column name has changed from 'email' to 'value'.

sun’s picture

@mh86: You're probably right with that. :) I'm not exactly sure what our procedure for that is though. It's not a core-to-core upgrade of user data, and not all features of the contrib module have been moved into core. Perhaps it would make most sense to create a separate issue for discussing whether an upgrade path can be provided by core?

klonos’s picture

Title: Move E-mail field type into core » Add an E-mail field type into core

...this clarifies that not all features of the contrib module were implemented in the corresponding one found in D8 core (it goes in par with the 'Added E-mail field type to core.' changelog entry).

As far as it goes for the upgrade path, this does concern only people using D7 + contrib email.module going to D8 + core email.module (perhaps + contrib email.module if they use its extra features). I mean it seems like a CCK in D7 core case where the 7.x version of the contrib module only includes features that got excluded from the move to core. How did we handle that?

Anyways, please link here any new issue opened for discussing the upgrade path.

mh86’s picture

Just opened a new issue for the upgrade path: #1773172: Provide a basic upgrade path for the D7 E-mail field module

klonos’s picture

Thanx Matthias ;)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Uh. That mostly looks good, except that patch adds that to Drupal 7.0's CHANGELOG entry, instead of Drupal 8.0's. :)

For now, I added that as a top-level item though we'll probably want to stick it under a new $something eventually... not sure what.

Committed and pushed to 8.x. Woohoo!! Thanks again for your awesome work on this new feature!

Automatically closed -- issue fixed for 2 weeks with no activity.