Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#32 | 1668332-32.patch | 465 bytes | swentel |
#29 | drupal8.email-field.29.patch | 6.9 KB | sun |
#29 | interdiff.txt | 5 KB | sun |
#26 | core_email_field-1668332-26.patch | 7.02 KB | rbayliss |
#26 | core_email_field-1668332-26-interdiff.txt | 1.31 KB | rbayliss |
Comments
Comment #1
Dave ReidMy 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).
Comment #3
Dave ReidComment #4
rbayliss CreditAttribution: rbayliss commentedHere's the most basic implementation. Wasn't sure whether this should be added to the text module or put on it's own.
Comment #5
rbayliss CreditAttribution: rbayliss commentedComment #6
Dave ReidWow, 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.
Comment #7
rbayliss CreditAttribution: rbayliss commentedMakes sense. What kind of test coverage do we need for this?
Comment #8
rbayliss CreditAttribution: rbayliss commentedHow about a version with no newline errors?
Comment #9
geerlingguy CreditAttribution: geerlingguy commentedI 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.Comment #10
rbayliss CreditAttribution: rbayliss commentedValidation happens in the email form element, so no field level validation is required. More specifically, it happens in form_validate_email().
Comment #11
geerlingguy CreditAttribution: geerlingguy commentedOh 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).)
Comment #12
sunWow, 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:
Let's add a trailing period here.
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.
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).
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?
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):
AFAIK, those empty default arrays and default behaviors do not need to be specified, so I think we can remove them.
"link" should be lowercase here, I think?
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.
Comment #13
moshe weitzman CreditAttribution: moshe weitzman commentedthis is so nice and simple. i don't even see anything that really merits a test.
Comment #14
xmacinfoResetting proper status.
Comment #15
Devin Carlson CreditAttribution: Devin Carlson commentedAn updated patch to address the coding concerns from #12.
Not addressed:
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'.
Comment #16
rbayliss CreditAttribution: rbayliss commentedThe '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.
Comment #17
sunThank 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:
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).
s/the email_field module./the E-mail module./
Can you quickly adjust the code for these two? :)
Comment #18
rbayliss CreditAttribution: rbayliss commentedSounds good. How's this?
Comment #20
sunAwesome! Thanks! :)
(guess it will come back green)
Comment #21
webchickWow, this is GREAT! Can't wait to commit this!
However, we need a few tests.
Comment #22
webchickOh. 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.
Comment #23
rbayliss CreditAttribution: rbayliss commentedHow does this look?
Comment #24
rbayliss CreditAttribution: rbayliss commentedThis time without the comments from NumberFieldTest.
Comment #26
rbayliss CreditAttribution: rbayliss commentedSame as #24, just renamed.
Comment #27
sunAwesome! :)
Comment #28
swentel CreditAttribution: swentel commentedExtreme 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 :)
Comment #29
sunQuickly helping you out there. Let's get this in! :)
Comment #30
geerlingguy CreditAttribution: geerlingguy commentedAnother RTBC here. Everything works great, and is super-simple.
Comment #31
webchickAwesome!
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?
Comment #32
swentel CreditAttribution: swentel commentedChangelog entry
Comment #33
pingers CreditAttribution: pingers commentedLooks good to me :)
Comment #34
mh86 CreditAttribution: mh86 commentedyeah, great to have the email field in core now :-)
Comment #35
mh86 CreditAttribution: mh86 commentedConcerning 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'.
Comment #36
sun@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?
Comment #37
klonos...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.
Comment #38
mh86 CreditAttribution: mh86 commentedJust opened a new issue for the upgrade path: #1773172: Provide a basic upgrade path for the D7 E-mail field module
Comment #39
klonosThanx Matthias ;)
Comment #40
webchickUh. 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!