After #2047229: Make use of classes for entity field and data definitions is in, all EntityType's base fields definitions should be converted to object.

Remaining Taks:

  • Convert base field definitions from array to object for EntityType 'user', 'node', etc. Take 'Term' for converting reference.
  • Remove FieldDefinition::createFromOldStyleDefinition and DataDefinition::createFromOldStyleDefinition.
  • Remove array support for base field definitions at hook_entity_field_info()/hook_entity_field_info_alter(), e.g., content_translation_entity_field_info_alter(). Update associated hook docu.
  • Remove left 'list_class', but use 'list_type' instead.
  • Update remaining array access to definition objects to use methods.

Comments

Berdir’s picture

Add

- Update remaining array access to definition objects to use methods

to that list :)

This places might not be obvious at first, but where for example talking about all $this->defintion within field item classes. I'm sure there are places where we still use it as an array.

Berdir’s picture

Issue summary:View changes

Add remaining 'list_type'

smiletrl’s picture

added.

smiletrl’s picture

Issue summary:View changes

Updated issue summary.

plach’s picture

following

yched’s picture

Title:Follow up: Convert and clean base field definitions.» Convert and clean base field definitions.
Issue summary:View changes
Status:Postponed» Active

Looks like we can unpostpone now ?

amateescu’s picture

Status:Active» Needs review
StatusFileSize
new70.84 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Looks like we even have a patch now! Quite a tedious one if I may say, so please don't change this again :)

A quick observation after all this is that we need a Timestamp field, or the existing Date field needs to comunicate its purpose better.

Status:Needs review» Needs work

The last submitted patch, 5: 2112239.patch, failed testing.

amateescu’s picture

Status:Needs work» Needs review
StatusFileSize
new117.69 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new47.9 KB

No, it did not take the whole day to write this, why would anyone think that?!

The last submitted patch, 7: 2112239-7.patch, failed testing.

amateescu’s picture

StatusFileSize
new127.49 KB
FAILED: [[SimpleTest]]: [MySQL] 33,994 pass(es), 1,596 fail(s), and 34,599 exception(s).
[ View ]
new13.47 KB

And not even half a night to investigate some crazy list behaviors..

One problem after removing createFromOldStyleDefinition() is that (for a FieldDefinition) in TypedDataManager::getPropertyInstance(), $definition['type'] was 'field_item:bla' before and now $definition->getDataType() is just 'list'.

The last submitted patch, 9: 2112239-9.patch, failed testing.

amateescu’s picture

StatusFileSize
new130.52 KB
FAILED: [[SimpleTest]]: [MySQL] 42,689 pass(es), 1,271 fail(s), and 48,032 exception(s).
[ View ]
new4.77 KB

After some hours of sleep and with a clear head, I figured this:

Since the data type of a ListDefinition and FieldDefinition is always 'list', the only thing we can do in TypedDataManager::getPropertyInstance() is to add another element to the "cache key", which is the data type of the item definition, like so:

--- a/core/lib/Drupal/Core/TypedData/TypedDataManager.php
+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
@@ -189,6 +189,9 @@ public function getInstance(array $options) {
   public function getPropertyInstance(TypedDataInterface $object, $property_name, $value = NULL) {
     $definition = $object->getRoot()->getDefinition();
     $key = $definition->getDataType();
+    if ($definition instanceof ListDefinition) {
+      $key .= ':' . $definition->getItemDefinition()->getDataType();
+    }
     if ($settings = $definition->getSettings()) {
       $key .= ':' . implode(',', $settings);
     }

The last submitted patch, 11: 2112239-11.patch, failed testing.

amateescu’s picture

StatusFileSize
new139.58 KB
FAILED: [[SimpleTest]]: [MySQL] 48,509 pass(es), 724 fail(s), and 47,903 exception(s).
[ View ]
new14.05 KB

After talking a bit with @fago in IRC, it seems that we want to use both the data type and the settings of the item definition, if the root definition is a list. Also added a few helper methods: getSetting(), setSetting() and getConstraint().

Looking at the TypedConfig stuff.. it's probably best if we bring back ArrayAccess just for that and convert everything related to TypedConfig in a followup, otherwise this can easily go over 200K :(

The last submitted patch, 13: 2112239-13.patch, failed testing.

amateescu’s picture

StatusFileSize
new140.25 KB
FAILED: [[SimpleTest]]: [MySQL] 54,039 pass(es), 554 fail(s), and 45,899 exception(s).
[ View ]
new3.78 KB

Some brown paper bag fixes, but that CommentFieldName stuff is a tough nut..

amateescu’s picture

StatusFileSize
new140.57 KB
FAILED: [[SimpleTest]]: [MySQL] 57,261 pass(es), 95 fail(s), and 51,729 exception(s).
[ View ]
new1.12 KB

This seems to work.

The last submitted patch, 15: 2112239-15.patch, failed testing.

The last submitted patch, 16: 2112239-16.patch, failed testing.

amateescu’s picture

StatusFileSize
new150.18 KB
FAILED: [[SimpleTest]]: [MySQL] 58,447 pass(es), 13 fail(s), and 51,671 exception(s).
[ View ]
new11.5 KB

Getting there :)

The last submitted patch, 19: 2112239-19.patch, failed testing.

The last submitted patch, 19: 2112239-19.patch, failed testing.

fago’s picture

getPropertyInstance() code looks good, I'd suggest adding the item definition settings to the $key also though. Properties of an item could vary by its settings.

  1. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -42,7 +45,7 @@ public function getFieldName() {
    -   * @return self
    +   * @return static

    Interesting - is that the approach do it now? Do you have a pointer to an issue discussing it, or guidelines? It seems to be missing from https://drupal.org/node/1354#types.

  2. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -24,6 +24,9 @@ class FieldDefinition extends ListDefinition implements FieldDefinitionInterface
    +   *
    +   * @todo Type-hint the return value with the interface when setters are added
    +   *   in https://drupal.org/node/2143297.

    I don't think we should do that, when you do YourClass::create() type-hinting should be aware of everything in YourClass - not only about the stuff in the interface.

  3. +++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
    @@ -23,14 +24,18 @@
    +      if (!is_object($definition)) {
    +        echo '<pre>';
    +        print_r($definition);
    +      }

    Debugging code - config-type definitions should be the only array-definitions flying around here. Cleaning that up is not trivial and should probably be done over at #1928868: Make typed config valid typed data.
    If we could move it to work with objects based on array access here, that would be a good first step imho. Else, we cannot start type-hinting on DataDefinitionInterface.

  4. +++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
    @@ -188,20 +193,6 @@ public function onChange($property_name) {
    -      $constraints[] = \Drupal::typedData()->getValidationConstraintManager()
    -        ->create('ComplexData', $this->definition['property_constraints']);
    -    }

    Imo this shows nicely how class based definitions can help simplifying things :-)

  5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
    @@ -41,45 +42,41 @@ class EntityReferenceItem extends FieldItemBase {
    +    $settings = $this->definition->getSettings();
    +    $target_type = $settings['target_type'];

    hm. We'll be using PHP 5.4 so we could do $this->definition->getSettings()['target_type'] in future?
    But we cannot do it yet, so maybe we could add a getSetting() just for now and note and will be removed just right now + search/replace it later on?

  6. +++ b/core/lib/Drupal/Core/TypedData/DataDefinitionInterface.php
    @@ -112,4 +123,17 @@ public function getSettings();
    +   * Returns a validation constraint.
    +   *
    +   * See \Drupal\Core\TypedData\TypedDataManager::getConstraints() for details.
    +   *
    +   * @param string $constraint_name
    +   *   The name of the the constraint, i.e. its plugin id.
    +   *
    +   * @return \Symfony\Component\Validator\Constraint
    +   *   A validation constraint.
    +   */
    +  public function getConstraint($constraint_name);
    +

    hm, we've got several issues here.
    a) It right now does not return a constraint object, but the array-based constraint definition which is later on turned to constraint objects.

    Then, a return value of NULL does not necessarily mean the array-key is not set, i.e. the constraint could be set with NULL for $options what works -- see Constraint::__construct().

    Moving to constraint objects as return value would make the read/write API a bit different. We could go with write by objects as well, but creating constraint objects is no nice OO API as constraint objects require *complete* constraint options for construction, or they throw exceptions. Thus, creation probably has to stay with plugin-id + options array.

  7. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Entity/Item.php
    @@ -80,52 +81,47 @@ public static function preDelete(EntityStorageControllerInterface $storage_contr
    +    // @todo Convert to a text field?
    ...
    +    // @todo Candidate for timestamp.
    ...
    +    // @todo Why not a UUID field?

    good questions, but out of scope here. Maybe let's move all of this in separate issue?
    In regard to the timestamp fields, I've recently created #2145103: Provide non-configurable field types for entity created, changed and timestamp fields. We probably want to add a general timestamp field, plus two special ones for created and changed dates.

  8. +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/Field/FieldType/DateTimeItem.php
    @@ -43,20 +44,17 @@ class DateTimeItem extends ConfigFieldItemBase implements PrepareCacheInterface
    +        ->setSettings(array(
               'date source' => 'value',
    -        ),

    should probably use setSetting()

  9. +++ b/core/modules/field/lib/Drupal/field/Entity/Field.php
    @@ -789,6 +801,13 @@ public function getConstraints() {
    +  public function getConstraint($constraint_name) {
    +    return NULL;
    +  }

    hm, seeing this I think we should move ConfigFieldItemList::getConstraints() content here. Separate issue though.

amateescu’s picture

StatusFileSize
new151.54 KB
FAILED: [[SimpleTest]]: [MySQL] 59,181 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
new18.56 KB

getPropertyInstance() code looks good, I'd suggest adding the item definition settings to the $key also though. Properties of an item could vary by its settings.

It's already added to the key because I'm switching the whole thing to an ItemDefinition if the root definition is a list:

    if ($definition instanceof ListDefinition) {
      $definition = $definition->getItemDefinition();
    }
    $key = $definition->getDataType();
    if ($settings = $definition->getSettings()) {
      $key .= ':' . implode(',', $settings);
    }
  1. I haven't seen it used in other places so I guess I'm a pioneer here, but it should be the standard. I noticed this problem when I was converting field definitions and, after using a setter from DataDefinition, the following autocomplete hints were also from DataDefinition instead of FieldDefinition, hence.. useless :)
  2. Agreed and fixed.
  3. I tried almost an entire day various approaches to make typed config play nice with object-based data definitions with no luck. Every approach ended up getting screwed by config_translation which is heavily based on array-style definitions, so fixing this whole entangled mess will be a huge task on its own. With great sadness, I had to revert the relevant parts from the patch :(
  4. Yep.
  5. I *did* introduce a getSetting() method in DataDefinition. I chose not to use it here because another setting is used below ('target_bundles'), so we can save a function call.
  6. I also though it was weird when I wrote it by copying the doxygen from getConstraints(), fixed both of them.
  7. Updated the TODOs related to timestamp/created/changed and I will create followup issues for the other two.
  8. Fixed.
  9. Separate issue++ :)

The last submitted patch, 23: 2112239-23.patch, failed testing.

amateescu’s picture

StatusFileSize
new152.46 KB
PASSED: [[SimpleTest]]: [MySQL] 59,075 pass(es).
[ View ]
new947 bytes

The last submitted patch, 25: 2112239-25.patch, failed testing.

fago’s picture

23: 2112239-23.patch queued for re-testing.

amateescu’s picture

25: 2112239-25.patch queued for re-testing.

The last submitted patch, 23: 2112239-23.patch, failed testing.

amateescu’s picture

Yay for a green patch, finally!

amateescu’s picture

StatusFileSize
new152.52 KB
FAILED: [[SimpleTest]]: [MySQL] 58,936 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 31: 2112239-31.patch, failed testing.

amateescu’s picture

Status:Needs work» Needs review
StatusFileSize
new152.35 KB
PASSED: [[SimpleTest]]: [MySQL] 59,212 pass(es).
[ View ]
new688 bytes

The node title field doesn't need its specific constraint anymore, it's added automatically by the field type now.

fago’s picture

Status:Needs review» Needs work

ok, reviewed this monster (and dreditor at it once, grrr - so going without now)

Patch does not apply any more (ValidReferenceConstraintValidator).

-    $properties['roles'] = array(
-      'label' => t('Roles'),
-      'description' => t('The roles the user has.'),
-      // @todo Convert this to entity_reference_field, see
-      // https://drupal.org/node/2044859

The @todo seems to be lost.

+      ->setDescription(t('The ID of the aggregator feed.'))
+      ->setReadOnly(TRUE);
+
+    // @todo Why does this entity type not have an UUID field?

Imo, unsure todos - aka todos that are questions - shouldn't be in the code. We should create issues for them, then either re-phrase them to be absolute (if clear), or just leave them out in favor of the issue.

+    // @todo Convert to a text field?
+    $fields['description'] = FieldDefinition::create('string')
+      ->setLabel(t('Description'))
+      ->setDescription(t('The body of the feed item.'));
+
+    // @todo Convert to a "timestamp" field in https://drupal.org/node/2145103.
+    $fields['timestamp'] = FieldDefinition::create('integer')
+      ->setLabel(t('Posted timestamp'))
+      ->setDescription(t('Posted date of the feed item, as a Unix timestamp.'));
+
+    // @todo Why not a UUID field?
+    $fields['guid'] = FieldDefinition::create('string')

Two times again.

+
+    // @todo Change to an entity_reference field?
+    $fields['field_id'] = FieldDefinition::create('string')
+      ->setLabel(t('Field ID'))

Again.

+    // @todo In what world does it make sense for a 'filesize' property to be a boolean field?
+    $fields['filesize'] = FieldDefinition::create('boolean')

Again.

-      // @todo Convert this to entity_reference_field, see
-      // https://drupal.org/node/2044859
-      'type' => 'string_field',

Looks like this got lost ;-)

amateescu’s picture

Title:Convert and clean base field definitions.» Convert base field and property definitions
Priority:Normal» Major
Status:Needs work» Needs review
StatusFileSize
new152.56 KB
PASSED: [[SimpleTest]]: [MySQL] 59,198 pass(es).
[ View ]
new4.11 KB
yched’s picture

Just a side question: I never really understood the static::$propertyDefinitions pattern used in getPropertyDefinitions() (or I was explained and forgot)
Is it still needed ? It's esp. weird to use a static here since the actual content can depend on $this->definition ?

fago’s picture

Status:Needs review» Reviewed & tested by the community

ad #35: I see, thanks!

+++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
@@ -430,7 +430,8 @@ public static function baseFieldDefinitions($entity_type) {
+    // @todo Convert to aa entity_reference field in

small typo - aa

Anyway, this is ready to go, so let's move on before it doesn't apply anymore.

ad #36: When you have X instances of an entity, the same field item with the same definition will be instantiated X times. For that we want/need to avoid having X instances of the same property definitions. Update: But yes, that static has to be keyed by any definition settings that might apply, e.g. ER fields do so.

yched’s picture

@fago / getPropertyDefinitions() / static::$propertyDefinitions:
Right, makes sense.
I opened #2150555: Change field types to define their property definitions statically for what we talked about in Vienna, with additional considerations on this static:: pattern.

chx’s picture

Issue tags:+beta blocker
catch’s picture

Title:Convert base field and property definitions» Change notice: Convert base field and property definitions
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

This looks great. Removes a bc layer and with it lots of cruft.

This hunk made me grin:

+++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
@@ -278,7 +303,7 @@ public function addConstraint($constraint_name, $options = NULL) {
-   * @todo: Remove once https://drupal.org/node/2112239 is in.
+   * @todo: Remove in https://drupal.org/node/1928868.

;)

Committed/pushed to 8.x!

I think there's already a change notice, but we might want to add that the bc layer has been removed?

Berdir’s picture

Title:Change notice: Convert base field and property definitions» Convert base field and property definitions
Status:Active» Fixed
Issue tags:-Needs change record

#2047229: Make use of classes for entity field and data definitions didn't have a change notice before this landed, and that BC layer only existed for a short time and wasn't mentioned, I don't think we need to do something here.

mradcliffe’s picture

Title:Convert base field and property definitions» Change notice: Convert base field and property definitions
Status:Fixed» Active
Issue tags:+Needs change record

I think this either needs a change notification or the previous change notification regarding data definitions needs to be updated. There's also a handbook page about it too.

- Change notice: http://drupal.org/node/2064123
- Documentation: http://drupal.org/node/2112677

mradcliffe’s picture

Status:Active» Needs review

I updated the existing change record and here is a draft of a new change record.

--
Title: Property definitions are now objects

ComplexDataInterface::getPropertyDefinitions() now returns an array of DataDefinition objects. This effects modules implementing FieldType plugins and TypedData. This changes the API referred to in change notice Field types are now plugins, which has been updated as well. This notice

Previously in Drupal 8

<?php
 
/**
   * {@inheritdoc}
   */
 
public function getPropertyDefinitions() {
    if (!isset(static::
$propertyDefinitions)) {
      static::
$propertyDefinitions['value'] = array(
       
'type' => 'string',
       
'label' => t('Telephone number'),
      );
    }
    return static::
$propertyDefinitions;
  }
?>

Changed to in Drupal 8

<?php
 
/**
   * {@inheritdoc}
   */
 
public function getPropertyDefinitions() {
    if (!isset(static::
$propertyDefinitions)) {
      static::
$propertyDefinitions['value'] = DataDefinition::create('string')
        ->
setLabel(t('Telephone number'));
    }
    return static::
$propertyDefinitions;
  }
?>
Berdir’s picture

Oh, completely forgot about that part, sorry. I also updated a bunch of documentation pages like https://drupal.org/node/2128865 and https://drupal.org/node/2112677, there are probably more of those still.

The change notice looks good, as we create one anyway, you might want to mention that this affects base fields as well and point to https://drupal.org/node/1806650 for details on that.

Also, make sure to reference both this issue and #2047229: Make use of classes for entity field and data definitions as the related issues.

Then set this to fixed again :)

amateescu’s picture

I don't think we've done this kind of HEAD to HEAD change notices before..

In case we want to make an exception here (I don't see why though), we might want to wait for #2002134: Move TypedData metadata introspection from data objects to definition objects which will also change the way of declaring property definitions a bit (removes the ugly / sometimes confusing statics).

Berdir’s picture

I've done a few of those, I think it's ok. It's meant as a notice for people that are already starting to upgrade their modules and tell them where to look for details.

For example, we already have *two* detailed tutorials on how to create/upgrade a field type implementation on d.o, one of those coming from a blog post that should be updated as well now.

amateescu’s picture

Okay then, change notice it is :)

klonos’s picture

xjm’s picture

Issue tags:+Missing change record

Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release

Berdir’s picture

Title:Change notice: Convert base field and property definitions» Convert base field and property definitions
Status:Needs review» Fixed
Issue tags:-Needs change record, -Missing change record

This was only kept open for an HEAD to HEAD change notice which will change once more in #2002134: Move TypedData metadata introspection from data objects to definition objects.

I think it's kind of pointless to do such a change notice 2 month after it happened and (hopefully) shortly before it changes again, so setting to fixed again. Make sure to follow the referenced issue to get notified when the upcoming change to the getPropertyDefinitions() lands. I can guarantee that you will like that change/simplification there and then this area will hopefully stabilize.

Status:Fixed» Closed (fixed)

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