Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

swentel’s picture

Status: Postponed » Active
plopesc’s picture

Taking this one...

plopesc’s picture

Status: Active » Needs review
FileSize
4.36 KB

Hello.

First approach.

Regards

mordonez’s picture

The last patch does not apply

error: patch failed: core/modules/email/email.module:19
error: core/modules/email/email.module: patch does not apply
mordonez’s picture

Status: Needs review » Needs work
mordonez’s picture

Status: Needs work » Needs review
Issue tags: -Field API

#4: field_type_email-2015703-4.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Field API

The last submitted patch, field_type_email-2015703-4.patch, failed testing.

mordonez’s picture

Status: Needs review » Needs work
FileSize
4.37 KB

I re-roll a new version with the changes from plopesc, no credit please.

Thanks

mordonez’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Field API

The last submitted patch, convert-to-typed-data-plugin-email-2015703-8.patch, failed testing.

mordonez’s picture

Status: Needs work » Needs review
Issue tags: +Field API
klausi’s picture

We already have an EmailItem in core/lib/Drupal/Core/Entity/Plugin/DataType/EmailItem.php - I think that should be unified or at least extended?

plopesc’s picture

Hello

I'm not sure, but I think that it is not necessary, for example, Drupal\text\Plugin\field\field_type\TextItem does not inherits from Drupal\Core\Entity\Plugin\DataType\StringItem. However, it could be possible that I'm wrong.

What do you think?

Regards

nils.destoop’s picture

Don't think that should be unified. core/lib/Drupal/Core/Entity/Plugin/DataType/EmailItem.php is the plugin for datatypes, this one is the plugin for field types. 2 different types of plugins.

I see you are using string as type in getPropertyDefinitions. Shouldn't that be type 'email'?

yched’s picture

The goal is to make Item classes be usable by both configurable fields and base fields.
Meaning, the end goal is to have the same EmailItem class used for both email fields added through the UI, and by user.mail, for example.

In order to do this, the FieldItem classes should:
- not rely on definition structures specific to configurable fields ($field / $instance), but use only methods on FieldDefinitionInterface.
- implement ConfigFieldItemInterface (or extend ConfigFieldItemBase) so that they can be used by Field UI

In the case of this patch, this just means:
$this->getInstance()->label
should be replaced by
$this->getFieldDefinition()->getFieldLabel()

In reply to #13 specifically, though:

We already have an EmailItem in core/lib/Drupal/Core/Entity/Plugin/DataType/EmailItem.php - I think that should be unified or at least extended?

I can't seem to find that class. user.mail currently uses a mere 'type' => 'string_field' (which is not for this patch to change).
So there's nothing to unify ? :-)

But the patch should remove the old /core/modules/email/lib/Drupal/email/Type/EmailItem.php class

Also :

- email_field_is_enpty() should be removed

-

 -      // The maximum length of an e-mail address is 254 characters. RFC 3696
-      // specifies a total length of 320 characters, but mentions that
-      // addresses longer than 256 characters are not normally useful. Erratum
-      // 1690 was then released which corrected this value to 254 characters.
-      // @see http://tools.ietf.org/html/rfc3696#section-3
-      // @see http://www.rfc-editor.org/errata_search.php?rfc=3696&eid=1690

This comment should be preserved in the new code.

Since the "254" max length is hardcoded in several places in the code, I'd suggest making it a class constant in EmailItem, and re-adding the explanation there.

plopesc’s picture

Status: Needs work » Needs review
FileSize
3.3 KB
5.35 KB

Hi

Including comments in #16

However, two points:

  • old field /core/modules/email/lib/Drupal/email/Type/EmailItem.php didn't exists
  • old module implementation didn't use hook_field_is_empty(), then, should we implement isEmpty() in our EmailItem?

Regards.

yched’s picture

Status: Needs review » Needs work

Wow, that's weird, I don't know what version of HEAD, I was looking at when writing #16, sorry for the confusion.

There is no core/modules/email/lib/Drupal/email/Type/EmailItem.php in HEAD currently.
There definitely is a /core/lib/Drupal/Core/Entity/Plugin/DataType/EmailItem.php file in HEAD right now, as @klausi pointed in #13.

It is currently only used for the 'mail' field of "contact message" entities, not for user.mail, oddly enough.
It "should" be ok to remove this class and use 'type' => 'field_type:email' in MessageStorageController::baseFieldDefinitions(). But then it means contact.module needs a requirement on email.module.
This seems best suited for a followup IMO. Can you open a separate issue, postponed on this one ? "use email.module's field type in contact.module" ?

And yup, there is no email_field_is_empty() to remove.

 function email_field_info_alter(&$info) {
-  if (Drupal::moduleHandler()->moduleExists('text')) {
+  if (\Drupal::moduleHandler()->moduleExists('text')) {

No, functional code should write Drupal::, not \Drupal::

+   * Defines the max length for an email address
+   *
+   * The maximum length of an e-mail address is 254 characters. RFC 3696
+   * specifies a total length of 320 characters, but mentions that
+   * addresses longer than 256 characters are not normally useful. Erratum
+   * 1690 was then released which corrected this value to 254 characters.
+   * @see http://tools.ietf.org/html/rfc3696#section-3
+   * @see http://www.rfc-editor.org/errata_search.php?rfc=3696&eid=1690
+   */
+  protected static $emailMaxLength = 254;

I was more thinking of a const EMAIL_MAX_LENGTH = 254:, this value is not supposed to change at runtime.
Also, the doc is fine here, no need to repeat it in schema()

klausi’s picture

/core/lib/Drupal/Core/Entity/Plugin/DataType/EmailItem.php should not be removed, because otherwise comment module and user module would need a dependency to email module (which we don't want). Email module should only provide the configurable part of email fields, the rest should be built-in core API. Same for UrlItem.php vs. link field module, right?

yched’s picture

Email module should only provide the configurable part of email fields

Hm. I guess that might be debated and agreed in a generic "strategy" issue about field types currently provided by separate modules, and their use in base fields in core entity types.

For now, what about keeping both classes then ?

klausi’s picture

Keeping both classes is fine with me, although email module should try to extend EmailItem already provided in the core API.

plopesc’s picture

Status: Needs work » Needs review
FileSize
4.57 KB
4.25 KB

New round where Drupal\email\Plugin\field\field_type\EmailItem extends from Drupal\Core\Entity\Plugin\DataType\EmailItem.

As I commented in #14 should TextItem class extend from StringItem?

Now it has both classes until the decission about how to manage fields provided by more than one module.

Regards

yched’s picture

Thanks @plopesc.

 /**
+ * Defines the max length for an email address
+ *
+ * The maximum length of an e-mail address is 254 characters. RFC 3696
+ * specifies a total length of 320 characters, but mentions that
+ * addresses longer than 256 characters are not normally useful. Erratum
+ * 1690 was then released which corrected this value to 254 characters.
+ * @see http://tools.ietf.org/html/rfc3696#section-3
+ * @see http://www.rfc-editor.org/errata_search.php?rfc=3696&eid=1690
+ */
+define(EMAIL_MAX_LENGTH, 254);

Nope, I mean a class constant :-)

class EmailItem ... {

  /**
   * phpdoc...
   */
  const EMAIL_MAX_LENGTH = 254;

  ...
}

Then used as static::EMAIL_MAX_LENGTH in the class code.

+class EmailItem extends \Drupal\Core\Entity\Plugin\DataType\EmailItem {

No good, the namespace needs to be "use"d at the top of the class.
Which then brings the fact that the two classes are named the same, so we'd need to rename the new one to ConfigurableEmailItem.

... Which then just becomes ridiculous IMO. That ConfigurableFieldItem has *nothing* more than FieldItem, except it has a schema() method that lets it be used for custom fields.

We end up defining two separate field type classes, thus cluttering our field type info with two separate types, just because the "configurable" one is shipped by a module on which we don't want to add dependencies.

OK, let's do this for now to unblock this, I guess, but we really need to determine a strategy for those cases, we'll have the same for any basic field type (text, number, link, ...), and separating each in two separate types for base fields and configurable fields is just not going to fly - especially if we intend to use widgets and formatters on base fields...

yched’s picture

plopesc’s picture

Hi

Sorry for the mistake about defining the constant :(

I used the namespace in the class declaration line because it was the way to inherit from one class with the same name. Changing the name breaks the consistency between classes defining FieldItem, should we rename all the FieldItemClasses to ConfigurableFieldItem classes?

IMHO, if a module uses certain field type, it should depend on the module that provides that field type. Otherwise, we are doing the same in two different places, creating inconsistencies and annoying to developers. That's my opinion and maybe I'm obviating some point. Anyway, it will be discussed in #2052135: Determine how to deal with field types used in base fields in core entities.

Regards.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @plopesc.

I fully agree with #25, but right, let's settle that in #2052135: Determine how to deal with field types used in base fields in core entities.
Let's get this in.

yched’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed cd62f0b and pushed to 8.x. Thanks!

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