Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Comment | File | Size | Author |
---|---|---|---|
#18 | d8_email_field_ng.patch | 7.29 KB | fago |
#7 | email-1839078-7.patch | 6.91 KB | tim.plunkett |
#7 | interdiff.txt | 4.45 KB | tim.plunkett |
#5 | 1839078_new_entity_field_api_email.patch | 6.85 KB | dasjo |
#5 | interdiff.txt | 3 KB | dasjo |
Comments
Comment #1
dasjoComment #2
dasjoimplemented a patch based on some inspiration from fago + #1839070: Implement the new entity field API for the taxonomy reference field type
Comment #3
dasjoAdded an email field type.
Comment #4
fagoGreat work! Only found some nitpics:
No line break needed after file.
Misses documentation.
This comment is wrong and should not be copied. See the updated term-reference patch.
Looks like we usually call it "e-mail".
Comment #5
dasjoThanks fago, new patch addresses comments in #4
Comment #6
fagoThanks. Good work!
Comment #7
tim.plunkettFixed some coding standards.
The self:: calls should all be static::, and 'field item class' should be 'field_item_class', but those are already wrong in other places.
Comment #8
fagoYes, underscores and namespaces in documentation references to interfaces need some work in the typed data API. I've created #1844112: Improve TypedData code and documentation style.
Is that part of the coding standards? My thinking was we are avoiding this sort of unneeded new lines?
Else improvements look good to me - thanks!
Comment #9
tim.plunkettThanks for opening the follow-up!
The blank line before the end of the class is indeed part of the coding standard.
Comment #10
fagohm, I'm unable to find it in the coding standards, do you have a pointer?
The class example over at http://drupal.org/node/608152 doesn't have this new line?
Comment #11
webchickSeems like sun might want to take a look at this before commit.
I don't immediately spot anything red-flaggy here though, other than the validate method is empty, but I searched in vain for an existing validate method in the Email field and the only thing I see there is help text that says it does it, though I cannot seem to figure out where. Hmmm.
Comment #12
fagovalidate() is still empty for all typed data implementations and will be implemented as part of #1845546: Implement validation for the TypedData API
Comment #13
sunMe? According to #1823042: Add maintainers and d.o components for all field type modules, @zuuperman signed up for E-mail module, but I can't assign him yet. ;)
Why not directly inject the one-liner from valid_email_address()?
Do we need a web test here? Looks like the test purely operates on API level, so it should technically work with DrupalUnitTestBase.
(The test in #1751210: Convert URL alias form element into a field and field widget might be helpful)
I don't really see where this class defines an 'email_field'...?
I don't think this reference can be resolved. Looks like you meant EmailItem instead of self::?
Comment #14
sunComment #15
fagoBecause the API for implementing it is not done yet. See #1845546: Implement validation for the TypedData API for doing that.
That's taken care of by field module right now, see field_entity_field_info(). Yes, we need to on unifying things here.
Yes. This comment happens to be in all the *Item classes. I'd suggest cleaning that and self:: -> static:: references up for all *Item classes in a follow-up issue.
Comment #16
chx CreditAttribution: chx commentedI added zuuperman to the core maintainers tab so he is now assigneable.
Comment #17
nils.destoop CreditAttribution: nils.destoop commentedLooks good. After changing the EmailItemTest to extend on DrupalUnitTestBase, this is rtbc. The patch in #1751210: Convert URL alias form element into a field and field widget (PathFieldCRUDTest) looks indeed like a good example.
Comment #18
fagoRe-rolled patch to apply again and converted the test case to an unit test.
Comment #19
chx CreditAttribution: chx commentedWell, then.
Comment #20
catchAny reason why this uses self:: rather than static::?
Comment #21
tim.plunkettThere are about 40 other usages of self:: instead of static:: in subclasses of \Drupal\Core\Entity\Field\FieldItemBase, as well as none of them specifying public/protected, so that should be a generic follow-up I think.
I'll open that up now.
Comment #22
tim.plunkett#1882146: Subclasses of \Drupal\Core\Entity\Field\FieldItemBase should use static:: instead of self:: is the follow-up.
Comment #23
catchOK. Committed/pushed to 8.x, thanks!