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.
Depends on #1969728: Implement Field API "field types" as TypedData Plugins
Instructions are on #2014671: [META] Convert all field types to plugins
Comment | File | Size | Author |
---|---|---|---|
#27 | convert-to-typed-data-plugin-email-2015703-27.patch | 4.35 KB | yched |
#27 | interdiff.txt | 618 bytes | yched |
#25 | convert-to-typed-data-plugin-email-2015703-25.patch | 4.37 KB | plopesc |
#25 | interdiff_22-25.txt | 4.89 KB | plopesc |
#22 | convert-to-typed-data-plugin-email-2015703-22.patch | 4.25 KB | plopesc |
Comments
Comment #1
swentel CreditAttribution: swentel commentedCommit in sandbox, see http://drupalcode.org/sandbox/yched/1736366.git/commitdiff/b700a31a99605...
Comment #2
swentel CreditAttribution: swentel commentedComment #3
plopescTaking this one...
Comment #4
plopescHello.
First approach.
Regards
Comment #5
mordonez CreditAttribution: mordonez commentedThe last patch does not apply
Comment #6
mordonez CreditAttribution: mordonez commentedComment #7
mordonez CreditAttribution: mordonez commented#4: field_type_email-2015703-4.patch queued for re-testing.
Comment #9
mordonez CreditAttribution: mordonez commentedI re-roll a new version with the changes from plopesc, no credit please.
Thanks
Comment #10
mordonez CreditAttribution: mordonez commentedComment #12
mordonez CreditAttribution: mordonez commented#9: convert-to-typed-data-plugin-email-2015703-8.patch queued for re-testing.
Comment #13
klausiWe already have an EmailItem in core/lib/Drupal/Core/Entity/Plugin/DataType/EmailItem.php - I think that should be unified or at least extended?
Comment #14
plopescHello
I'm not sure, but I think that it is not necessary, for example,
Drupal\text\Plugin\field\field_type\TextItem
does not inherits fromDrupal\Core\Entity\Plugin\DataType\StringItem
. However, it could be possible that I'm wrong.What do you think?
Regards
Comment #15
nils.destoop CreditAttribution: nils.destoop commentedDon'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'?
Comment #16
yched CreditAttribution: yched commentedThe 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:
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
-
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.
Comment #17
plopescHi
Including comments in #16
However, two points:
Regards.
Comment #18
yched CreditAttribution: yched commentedWow, 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.
No, functional code should write Drupal::, not \Drupal::
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()
Comment #19
klausi/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?
Comment #20
yched CreditAttribution: yched commentedHm. 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 ?
Comment #21
klausiKeeping both classes is fine with me, although email module should try to extend EmailItem already provided in the core API.
Comment #22
plopescNew 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
Comment #23
yched CreditAttribution: yched commentedThanks @plopesc.
Nope, I mean a class constant :-)
Then used as static::EMAIL_MAX_LENGTH in the class code.
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...
Comment #24
yched CreditAttribution: yched commented@klausi: opened #2052135: Determine how to deal with field types used in base fields in core entities
Comment #25
plopescHi
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.
Comment #26
yched CreditAttribution: yched commentedThanks @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.
Comment #27
yched CreditAttribution: yched commentedJust removed the 'module' entry now that #2041423: Rely on 'provider' instead of 'module' for Field plugin types is in.
Comment #28
alexpottCommitted cd62f0b and pushed to 8.x. Thanks!