Convert base field and property definitions

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.

Add remaining 'list_type'

following

Looks like we can unpostpone now ?

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.

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

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

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);
     }

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 :(

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

This seems to work.

Getting there :)

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: Typed config incorrectly implements Typed Data interfaces.
   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.

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++ :)

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

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

Yay for a green patch, finally!

Rerolled after #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title.

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

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 ;-)

All those TODOs were formulated as questions because I didn't know if we want to pursue them in this issue or in followups. Now that we decided for followups, I opened issues for each one:

#2149841: The 'aggregator_feed' entity type doesn't have a UUID field
#2149845: Convert the description field of the 'aggregator_item' entity to a text field
#2149851: Remove todo about GUID field on the 'aggregator_item' entity and add UUID field
#2149859: Convert the 'field_id' base field on comment entities to an entity reference field
#2149877: The 'filesize' base field of field entities is a boolean?

Edit: fixed typo

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 ?

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.

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

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?

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

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

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

  /**
   * {@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

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

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 :)

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

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.

Okay then, change notice it is :)

...coming from #2039607: Change node type field to an entity reference

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

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.