Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dasjo’s picture

Assigned: Unassigned » dasjo
dasjo’s picture

Status: Active » Needs review
FileSize
4.22 KB

implemented a patch based on some inspiration from fago + #1839070: Implement the new entity field API for the taxonomy reference field type

dasjo’s picture

Added an email field type.

fago’s picture

Great work! Only found some nitpics:

+++ b/core/modules/field/modules/email/lib/Drupal/email/Tests/EmailItemTest.php
@@ -0,0 +1,76 @@
+ * @file
+ *
+ * Definition of Drupal\email\Tests\EmailItemTest.

No line break needed after file.

+++ b/core/modules/field/modules/email/lib/Drupal/email/Tests/EmailItemTest.php
@@ -0,0 +1,76 @@
+
+  public function testEmailItem() {

Misses documentation.

+++ b/core/modules/field/modules/email/lib/Drupal/email/Type/EmailItem.php
@@ -0,0 +1,39 @@
+   * Field definitions of the contained properties.

This comment is wrong and should not be copied. See the updated term-reference patch.

+++ b/core/modules/system/system.module
@@ -2057,6 +2057,11 @@ function system_data_type_info() {
+      'label' => t('Email'),

Looks like we usually call it "e-mail".

dasjo’s picture

Thanks fago, new patch addresses comments in #4

fago’s picture

Status: Needs review » Reviewed & tested by the community

Thanks. Good work!

tim.plunkett’s picture

FileSize
4.45 KB
6.91 KB

Fixed 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.

fago’s picture

The self:: calls should all be static::, and 'field item class' should be 'field_item_class', but those are already wrong in other places.

Yes, 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.

+++ b/core/modules/field/modules/email/lib/Drupal/email/Type/EmailItem.php
@@ -36,4 +36,5 @@ public function getPropertyDefinitions() {
     }
     return self::$propertyDefinitions;
   }
+

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!

tim.plunkett’s picture

Thanks for opening the follow-up!

The blank line before the end of the class is indeed part of the coding standard.

fago’s picture

hm, 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?

webchick’s picture

Assigned: dasjo » sun

Seems 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.

fago’s picture

validate() is still empty for all typed data implementations and will be implemented as part of #1845546: Implement validation for the TypedData API

sun’s picture

Me? 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. ;)

+++ b/core/lib/Drupal/Core/TypedData/Type/Email.php
@@ -0,0 +1,24 @@
+  public function validate() {
+    // @todo Implement validate() method.
+  }

Why not directly inject the one-liner from valid_email_address()?

+++ b/core/modules/field/modules/email/lib/Drupal/email/Tests/EmailItemTest.php
@@ -0,0 +1,85 @@
+class EmailItemTest extends WebTestBase {

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)

+++ b/core/modules/field/modules/email/lib/Drupal/email/Type/EmailItem.php
@@ -0,0 +1,40 @@
+/**
+ * Defines the 'email_field' entity field item.
+ */
+class EmailItem extends FieldItemBase {

I don't really see where this class defines an 'email_field'...?

+++ b/core/modules/field/modules/email/lib/Drupal/email/Type/EmailItem.php
@@ -0,0 +1,40 @@
+   * @see self::getPropertyDefinitions()

I don't think this reference can be resolved. Looks like you meant EmailItem instead of self::?

sun’s picture

Assigned: sun » Unassigned
fago’s picture

Why not directly inject the one-liner from valid_email_address()?

Because the API for implementing it is not done yet. See #1845546: Implement validation for the TypedData API for doing that.

I don't really see where this class defines an 'email_field'...?

That's taken care of by field module right now, see field_entity_field_info(). Yes, we need to on unifying things here.

I don't think this reference can be resolved. Looks like you meant EmailItem instead of self::?

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.

chx’s picture

Assigned: Unassigned » nils.destoop

I added zuuperman to the core maintainers tab so he is now assigneable.

nils.destoop’s picture

Status: Reviewed & tested by the community » Needs work

Looks 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.

fago’s picture

Status: Needs work » Needs review
FileSize
7.29 KB

Re-rolled patch to apply again and converted the test case to an unit test.

chx’s picture

Status: Needs review » Reviewed & tested by the community

After changing the EmailItemTest to extend on DrupalUnitTestBase, this is rtbc.

class EmailItemTest extends DrupalUnitTestBase {

Well, then.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Any reason why this uses self:: rather than static::?

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

There 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.

tim.plunkett’s picture

catch’s picture

Status: Reviewed & tested by the community » Fixed

OK. Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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