Move TypedData metadata introspection from data objects to definition objects

wrong issue

Sounds like a plan :-)

+1 from me as well.

Since this is only about metadata, do we have another issue/discussion for ComplexDataInterface::getProperties()/getPropertyValues()/setPropertyValues() ?

Started some work on this. For now it does mostly #2150555: Change field types to define their property definitions statically and adds the interface and the base class.

Note that the patch won't apply as it's written on top of #2144327: Make all field types provide a schema() to prevent a painful reroll..

I'm not sure when I'll be able to get back to it, so anyone can feel free to pick it up :)

Hm, Map is used in LinkItem->attributes and menulink/shortcut introduce a MapItem that works the same dynamic way. This is necessary...

Based on discussion with @EclipseGc, this sounds like it may be impactful enough for contrib that we should try to sort it out by beta. @EclipseGc also suggested that it should be release-blocking, but I'm not entirely sure.

It would be helpful to get the issue summary updated with what this impacts and why, especially before-and-after consuming code examples if/when we've settled on an approach. Also, once that's here, I'd suggest reaching out to a core maintainer (possibly @catch) to ask for approval for the API change and get his feedback on whether this issue should be considered release-blocking or is merely important.

Yeah, I guess the real question here is with regard to the sort of "contrib sanity" we are aiming at. There are more places that might benefit from this, but we're definitely talking about the "super module" category of stuff like page_manager/rules/panels/et al. I'd really really like to see this in before 8.0, and I don't want to see us gloss over it because ONLY Rules and Page_manager need it. We need a solution for this in core so that there's no ambiguity about the API that should be utilized. Is it going to impact the average contrib project? ehhh... probably not, but we shouldn't ship a final release w/o a solution here imo.

Eclipse

Patch needs re-roll, but awesome! +1 to rtbc

This approach probably will finally allow comment field to build properly ER to parent entity via definition....

1. +++ b/core/modules/comment/lib/Drupal/comment/CommentFieldName.php
   @@ -16,26 +17,15 @@
   +  public static function propertyDefinitions(FieldDefinitionInterface $field_definition) {
   
   +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Field/FieldType/CommentItem.php
   @@ -33,39 +34,31 @@
   -  public function getPropertyDefinitions() {
   ...
   +  public static function propertyDefinitions(FieldDefinitionInterface $field_definition) {
   

   yay!

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -30,56 +31,42 @@
   +  public static function propertyDefinitions(FieldDefinitionInterface $field_definition) {
   +    $settings = $field_definition->getSettings();
   ...
   +        'EntityType' => $settings['target_type'],
   ...
   +    if (isset($settings['target_bundle'])) {
   
   +++ b/core/lib/Drupal/Core/TypedData/MapDefinition.php
   @@ -0,0 +1,74 @@
   +   *
   
   +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/ConfigurableEntityReferenceItem.php
   @@ -38,29 +39,20 @@ class ConfigurableEntityReferenceItem extends ConfigEntityReferenceItemBase impl
   +    // revisions.
   ...
   +  public static function propertyDefinitions(FieldDefinitionInterface $field_definition) {
   ...
   +    $properties = parent::propertyDefinitions($field_definition);
   

   Passing in a field settings only needed for ER fields but probable will help comment field to build ER to parent entity in definition

On visual review, patch looks good to me.

Thanks, @fago, for taking over :)

1. +++ b/core/lib/Drupal/Core/Field/FieldItemDefinition.php
   @@ -0,0 +1,63 @@
   +use Drupal\Core\TypedData\ComplexDataDefinitionBase;
   

   This one doesn't seem to be used in the file.

2. +++ b/core/modules/comment/comment.module
   @@ -895,7 +895,7 @@ function comment_translation_configuration_element_submit($form, &$form_state) {
   + * @see \Drupal\comment\Plugin\Field\FieldType\CommentItem::getPropertyDefinitions(propertyDefinitions
   

   :)

3. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
   @@ -201,4 +209,69 @@ public function getDefaultValue(EntityInterface $entity) {
   +    // Do not serialized the statically cached property definitions.
   

   ".. serialize .."

4. +++ b/core/lib/Drupal/Core/Field/FieldDefinitionInterface.php
   @@ -209,4 +209,36 @@ public function isMultiple();
   +  public function getPropertyDefinition($name);
   ...
   +  public function getPropertyDefinitions();
   ...
   +  public function getMainPropertyName();
   

   Shouldn't FieldDefinitionInterface extend ComplexDataDefinitionInterface in addition to ListDefinitionInterface?

I'm not sure I understand exactly what are the changes in Map, but we now have a MapItem field type from #2021779: Decouple shortcuts from menu links.

Reroll for #2132145: Rename 'typed_data' / Drupal::typedData() to 'typed_data_manager' / Drupal::typedDataManager

I think it's better to move on with an interim step which basically just covers #2150555: ComplexDataInterface::getPropertyDefinitions() should be static , then complete it in a follow-up

It seems like most of this patch is only implementing #2150555: Change field types to define their property definitions statically. I can't tell if it does more than that though: for example, are all the changes this patch makes to FieldDefinition really necessary just to make getPropertyDefinitions() static? How about moving most (all?) of this patch to #2150555: Change field types to define their property definitions statically and then letting this issue proceed with implementing the rest of its scope, instead of punting all of this issue title's stated goal to a follow up? :)

So I think best, we complete the metadata implementation for fields here by adding tests that work based on the new API + complement the public API here, while the remaining implementations of the separated metadata + the removal of the previous typed-data way to get metadata could be done in a separate issue

At this point in the dev cycle, I'm nervous about adding a new API without removing the old one in the same patch, especially in relation to Entity fields and Typed data, which are already hard to follow as-is. I know we all have the same end goal in mind in terms of cleaning up these APIs, so it's just a question of what is the best interim state to leave D8 in with each commit. Perhaps I'm just not fully getting what the benefit of getting this incremental piece in separately is? If it's just to keep this patch size manageable/reviewable, does #2150555-3: Change field types to define their property definitions statically help?

Shouldn't FieldDefinitionInterface extend ComplexDataDefinitionInterface in addition to ListDefinitionInterface? ... I don't think so. While it makes sense from a field API perspective, it doesn't from a typed data api perspective as if we'd do it, you would not be able to know whether it's a list or complex data.

I'm confused by that. Wouldn't typed data consumers be able to know if it is or isn't a list by checking if it's ListDefinitionInterface and/or calling isList()? So the only issue is if consuming code doesn't care that it's a list, and instead only checks if it's ComplexDataDefinitionInterface, and if so, then proceeds to treat the field as complex data. But since fields are designed to be successfully treated as complex data (by autorouting to the first item), wouldn't that be what we want? If you have a use case in mind where it's not, please explain it.

Semi related to #52: looks like even though FieldDefinitionInterface does extend DataDefinitionInterface, that it still redefines a bunch of its methods. That's a pre-existing condition in HEAD. Is there an open issue to clean that up, or should that be done here, or is there a reason to continue having that overlap?

Very partial review. Just starting with the new test:

1. +++ b/core/modules/system/lib/Drupal/system/Tests/TypedData/MetadataTest.php
   @@ -0,0 +1,170 @@
   + * @file
   + * Definition of Drupal\system\Tests\TypedData\MetadataTest.
   

   How about renaming the test to TypedDataDefinitionTest? Since we're now using the terminology of "definition" rather than "metadata" in the codebase.

2. +++ b/core/modules/system/lib/Drupal/system/Tests/TypedData/MetadataTest.php
   @@ -0,0 +1,170 @@
   +use Drupal\Core\Entity\EntityDefinition;
   +use Drupal\Core\Field\FieldDefinition;
   +use Drupal\Core\Field\FieldDefinitionInterface;
   

   I don't think it's good for a TypedData test to (conceptually) depend on higher-level components like entity and field. Can this test be limited to just testing the classes that are part of TypedData, and the entity and field tests be moved to their respective components?

3. +++ b/core/modules/system/lib/Drupal/system/Tests/TypedData/MetadataTest.php
   @@ -0,0 +1,170 @@
   +    $list = ListDefinition::create('string');
   +    $this->assertTrue($list instanceof ListDefinitionInterface);
   +    $item = $list->getItemDefinition();
   +    $this->assertTrue($item instanceof DataDefinitionInterface);
   

   I think an important benefit of this issue is that it helps to clarify the difference between data objects and definition objects. I think that DX improvement is diluted by then having local variable names lose that clarity. Can we rename these to $list_definition, $item_definition, etc. throughout the test?

4. +++ b/core/modules/system/lib/Drupal/system/Tests/TypedData/MetadataTest.php
   @@ -0,0 +1,170 @@
   +    $field = FieldDefinition::create('integer');
   

   I like that since we're calling create() on FieldDefinition, that the parameter can simply be 'integer', not 'field_item:integer'.

5. +++ b/core/modules/system/lib/Drupal/system/Tests/TypedData/MetadataTest.php
   @@ -0,0 +1,170 @@
   +    $entity = EntityDefinition::create('entity:node');
   

   So, can we do the same here: change 'entity:node' to just 'node'?

Retitling for clarity/precision

Sorry for the delay, I'll try to review shortly too...

Just plugging onto :

But since fields are designed to be successfully treated as complex data (by autorouting to the first item),

Might be related: #2164601: Stop auto-creating FieldItems on mere reading of $entity->field[N] #3 /#4 (try to get rid of "an "empty" FieldItemList implicitly has one Item that gets repeatedly autocreated & flushed back & forth")

Sorry for taking so long to come back to this issue. Here's a small review of the non-test parts:

1. +++ b/core/lib/Drupal/Core/Entity/EntityDefinition.php
   @@ -0,0 +1,107 @@
   +   * @return static
   +   *   A new MapDefinition object.
   

   Lies! :)

2. +++ b/core/lib/Drupal/Core/Entity/EntityDefinition.php
   @@ -0,0 +1,107 @@
   +        // @todo: Add support for handling multiple bundles.
   

   Let's open a followup issue for this and link it here, seems important enough.

3. +++ b/core/lib/Drupal/Core/Entity/EntityDefinitionInterface.php
   @@ -0,0 +1,55 @@
   +interface EntityDefinitionInterface extends ComplexDataDefinitionInterface {
   

   Related to #23.4, #24 and #52. So EntityDefinitionInterface extends ComplexDataDefinitionInterface but FieldDefinitionInterface doesn't/shouldn't? Colour me confused.. :/

4. +++ b/core/lib/Drupal/Core/Field/FieldDefinitionInterface.php
   @@ -209,4 +209,36 @@ public function isMultiple();
   +  public function getMainPropertyName();
   
   +++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
   @@ -24,6 +24,31 @@
   +  public static function getMainPropertyName();
   

   Seems a bit wrong for both FieldDefinitionInterface and FieldItemDefinitionInterface to have this method, they should return the same thing on both objects..

5. +++ b/core/lib/Drupal/Core/TypedData/ComplexDataDefinitionBase.php
   @@ -0,0 +1,58 @@
   +  public function getMainPropertyName() {
   

   Oh! So we want it for all complex data definitions. Is this really necessary?

6. +++ b/core/lib/Drupal/Core/TypedData/ListInterface.php
   @@ -19,6 +19,17 @@
   +  public static function createDataDefinition($data_type);
   

   This is already added to TypedDataInterface, which this one extends. Why are we adding it here too?

7. +++ b/core/lib/Drupal/Core/TypedData/TypedDataInterface.php
   @@ -15,6 +15,17 @@
   +   * @return \Drupal\Core\TypedData\DataDefinitionInterface
   

   A @return static should be better here as IDEs will typehint with the actual XDefinitionInterface used.

Barely started reviewing, but I'm a bit scared by the new EntityDefinition / FieldItemDefinition. Seems like they would add lots of confusion wrt Entity/EntityType & FieldDefinition respectively ?

FieldItemDefinition operates at a completely different level than FieldDefinition, doesn't it ?

Could we make it more specific in the class name that it's about TypedData ? and maybe move to a nested folder / namespace more related to TypedData ? (we already have 10s of classes at the root of Core\Entity & Core\Field...)

I just committed #2015689: Convert field type to typed data plugin for options module (which was critical and a beta blocker), which means this one needs to re-roll. On the plus side, though, this patch can now act on the full set of field types. :)

Rerolled.

Partial review, posting what I have for now.

1. What this does to propertyDefinitions() in Item classes is pure awesomeness :-)

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -473,11 +490,17 @@ public function getIterator() {
   -  public function getPropertyDefinition($name) {
   +  protected function getFieldDefinition($name) {
   

   The phpdoc for protected $fieldDefinitions in still mentions "ContentEntityBase::getPropertyDefinitions()", should be updated accordingly

3. +++ b/core/modules/system/lib/Drupal/system/Tests/TypedData/TypedDataTest.php
   @@ -423,10 +428,10 @@ public function testTypedDataMaps() {
   -    $this->assertEqual(array_keys($typed_data->getPropertyDefinitions()), array_keys($value));
   -    $definition = $typed_data->getPropertyDefinition('one');
   ...
   +    $this->assertEqual(array_keys($typed_data->getDefinition()->getPropertyDefinitions()), array_keys($value));
   ...
   -    $this->assertEqual($definition->getDataType(), 'any');
   -    $this->assertFalse($typed_data->getPropertyDefinition('invalid'));
   ...
   +    $definition = $typed_data->getDefinition()->getPropertyDefinition('one');
   +    $this->assertEqual($definition->getDataType(), 'string');
   +    $this->assertNull($typed_data->getDefinition()->getPropertyDefinition('invalid'));
   

   Minor : those asserts could stay as is, Map still has wrapper methods for those?

   (actually, Map has a wrapper method for getPropertyDefinitions(), but not for getPropertyDefinition($name) - would be worth adding ?)

4. +++ b/core/modules/system/lib/Drupal/system/Tests/TypedData/TypedDataTest.php
   @@ -450,8 +455,11 @@ public function testTypedDataMaps() {
   +    // Test setting a not defined property. It shouldn't show up in the
   +    // properties, but be kept in the values.
        $typed_data->setValue(array('foo' => 'bar'));
   -    $this->assertEqual(array_keys($typed_data->getProperties()), array('foo'));
   +    $this->assertEqual(array_keys($typed_data->getProperties()), array('one', 'two', 'three'));
   +    $this->assertEqual(array_keys($typed_data->getValue()), array('foo', 'one', 'two', 'three'));
   

   Looks like a change in behavior, not sure how that's related to the task here - setValue() used to *replace* the existing values, but it now *adds* to them ?

5. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutPath.php
   @@ -16,25 +17,14 @@
    class ShortcutPath extends StringItem {
   

   Not really related, but wouldn't this be better named ShortcutPathItem ?
   [edit: patch in #2175785: Rename ShortcutPath to ShortcutPathItem]

6. +++ b/core/modules/options/lib/Drupal/options/Plugin/Field/FieldType/ListTextItem.php
   @@ -53,13 +46,13 @@ public static function schema(FieldDefinitionInterface $field_definition) {
   +    $constraints = array('Length' => array('max' => 255));
   +    $properties['value'] = DataDefinition::create('string')
   +      ->setLabel(t('Text value'))
   +      ->setConstraints($constraints);
   

   Nitpick: the $constraint could be inlined.

7. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldType/LinkItem.php
   @@ -28,27 +29,19 @@
   +    $properties['attributes'] = MapDefinition::create('map')
   

   MapDefinition::create('map') - redundant ?
   (also in TypedDataTest)

8. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -31,56 +33,47 @@
   +    if ($target_type_info->isSubclassOf('\Drupal\Core\Entity\ContentEntityInterface')) {
   +      // @todo: Lookup the entity type's ID data type and use it here.
   +      // https://drupal.org/node/2107249
   +      $properties['target_id'] = DataDefinition::create('integer')
   +        ->setLabel(t('Entity ID'))
            ->setConstraints(array(
   -          'EntityType' => $settings['target_type'],
   +          'Range' => array('min' => 0),
            ));
   +    }
   +    else {
   +      $properties['target_id'] = DataDefinition::create('string')
   +        ->setLabel(t('Entity ID'));
   

   Nitpick: would be more readable with
   if () {
   $target_id_definition = ...
   }
   else {
   $target_id_definition = ...
   }
   $properties['target_id'] = $target_id_definition.

9. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -31,56 +33,47 @@
   +    $settings = $field_definition->getSettings();
        $target_type = $settings['target_type'];
   

   Code should be cleaned up, there are still places that call $settings['target_type'] instead of $target_type.

   No need for $settings IMO.
   $target_type = $field_definition->getSetting('target_type');
   $target_bundle = $field_definition->getSetting('target_bundle');

10. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/DateItem.php
    @@ -23,26 +23,15 @@
    +    $properties['value'] = array(
    +      'type' => 'date',
    +      'label' => t('Date value'),
    +    );
    

    flat array ? no DataDefinition::create(...) ?

Additional remarks:

1. As discussed with @fago on IRC:
We have more and more "Definition" classes. The ones manipulated here are definitions "as a piece of typed data" - those are not just "definitions", those are "data definitions".

It would thus be good to clearly relate them to the TypedData API, both:
- in the namespace / folders (...\TypedData\X.php)
- in the actual class & interface names, by standardizing on a *DataDefinition prefix
MapDefinition -> MapDataDefinition
ListDefinition -> ListDataDefinition
DataReferenceDefinition -> ReferenceDataDefinition
EntityDefinition -> EntityDataDefinition
FieldItemDefinition -> FieldItemDataDefinition
& same for interfaces

FieldDefinition would probably stay as is for now, since it's also the domain definition object for Field API. We'd want to split that to a decorator pattern too later.

2. DataReferenceDefinitionInterface doesn't extend DataDefinitionInterface ? It's the only one who doesn't.

3. FieldItemDefinition doesn't extend MapDefiniton ?

4. So DataDefinition::create() is used only for primitives (e.g 'integer') ?
Should there then be a PrimitiveDataDefinition::create() ? I.e DataDefinition would only be a parent class for the others, but without a create() method, you can only create() a specific child class.
Right now it feels I could do both MapDefinition::create() & DataDefinition::create('map'), but not sure what would work / break.

Other than that - reviewing a 150k patch without a proper patch summary in the OP is ... not too friendly :-)

I'm still trying to figure a "patch content" list so that can try to make sense of it, but the patch does a lot of things and touches a lot of code, besides the "simple" part about FieldItem::propertyDefinitions(). Not sure it was reasonable to do all this in one patch ?

Thanks for the replies. I mostly agree, remarks below.
Thanks for the updated summary too, I'll continue reviewing :-)

1. "no Data object has methods to access property definitions, Map::getPropertyDefinitions() is just left to ease transition" :
The rule is fine by me, but then not clear if/when we remove Map::getPropertyDefinitions() ?

2. #75.6 :
That $constraints variable still seems totally extraneaous and readable-- to me :-), but well, no biggie.

3.

Maybe we should point to the factory at DataDefininition::create() though, i.e. document it there? Or, we make it using the factory by default via \Drupal + instantiate the class directly in TypedData::createDataDefinition()? So DataDefinition::create() would always work. That seems to be the safest option to me - thoughts?

Not sure I fully get that proposal, can you elaborate ?

But yeah, that DataDefinition::create() / Data::createDataDefinition() / DataManager->createDataDefinition() stack is precisely the part that puzzles me right now :-)

As I understand it, the Definition objects are created either:

a) For non-dynamic code that knows which class it wants, by static create() methods on the XxDefinition classes (e.g easy syntax in FieldItem::propertyDefinitions())

b) For dynamic code (e.g Rules), by a generic $data_manager->createDataDefinition(string $data_type) factory.
In this case this goes through:
- a static createDataDefinition() method on the XxData class,
- which calls XxDefinition::create() as in a), but wrapping it with extra massaging.

a) and b) are not equivalent then ? That extra massaging layer in XxData::createDataDefinition() happens only in a).

I understand this as:
- code that does a) knows the semantics of the specific XxDefinition class it's using and can do the proper massaging it needs.
- code that does b) only has a $data_type string, and doesn't know the kind of massaging the underlying definition class needs, thus the data class associated to $data_type provides a "default massaging step".

If so, yeah, I guess that would deserve better explanation in the phpdocs for those three places:
- TypedData::createDataDefinition() : it's for generic code that only has a $data_type
- XxData::createDataDefinition() : it's what the above relies on
- XxDefinition::create() : it's the common way for code that can hardcode the class

4. DataReferenceDefinition :
Right, keeping DataReferenceDefinition / DataReferenceInterface / DataReferenceBase is probably best then.

I just committed #2141539: Rename FieldItemListInterface::filterEmptyValues() to filterEmptyItems() (per yched) which broke this one. And since this isn't yet out of architectural discussion, I'm going to remove the "Avoid commit conflicts" tag. Feel free to add it back when this patch is on the home stretch.

Reroll - patch had fuzz, but didn't actually conflict with #2141539: Rename FieldItemListInterface::filterEmptyValues() to filterEmptyItems()

@fago :

let DataDefinition::create($data_type) internally use the factory

Currently, DTManager->createDataDefinition($data_type)
- calls XxData::createDataDefinition($data_type)
- which calls XxDataDefinition::create().

If that last step calls the DTManager factory itself, we have a loop :-)
So I guess that would mean:

DTManager->createDataDefinition($data_type)
- calls XxData::createDataDefinition($data_type)
- which builds the right XxDataDefinition object and returns it

XxDataDefinition::create() is just a shortcut to that same call sequence, with a hardcoded $data_type. The custom code currently in the various XxDataDefinition::create() methods would move up in XxData::createDataDefinition().

Not sure I have a clear opinion right now, tbh :-)

I'd tend to think the notion of "definition" precedes the notion of "data", so having the data class be the one that creates the definition feels a bit weird.

Couldn't we include the 'definition_class' in the DataType annotation, and then have DTManager->createDataDefinition() directly call XxDataDefinition::create(), skipping the step of going through the Data class ?

Looks nice!

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -214,13 +232,12 @@ public function preSaveRevision(EntityStorageControllerInterface $storage_contro
      public function getDefinition() {
        // @todo: This does not make much sense, so remove once TypedDataInterface
        // is removed. See https://drupal.org/node/2002138.
   ...
      /**
   

   Is this method now staying or not? if yes, remove the @todo?

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -466,11 +483,17 @@ public function getIterator() {
      /**
   -   * {@inheritdoc}
   +   * Gets the definition of a contained field.
   +   *
   ...
   +   *   The name of the field.
   +   *
   +   * @return \Drupal\Core\Field\FieldDefinitionInterface|false
   +   *   The definition of the field or FALSE if the field does not exist.
       */
   -  public function getPropertyDefinition($name) {
   +  protected function getFieldDefinition($name) {
   

   Shouldn't this be defined on the interface too?

3. +++ b/core/lib/Drupal/Core/Entity/EntityDefinition.php
   @@ -0,0 +1,99 @@
   +   * @param string $entity_type
   +   *   (optional) The entity type type, or NULL if unknown. Defaults to NULL.
   

   I think we use entity type ID now, since EntityType landed.

4. +++ b/core/lib/Drupal/Core/Entity/EntityDefinition.php
   @@ -0,0 +1,99 @@
   +        // @todo: Add support for handling multiple bundles.
   +        // See https://drupal.org/node/2169813.
   +        $bundles = $this->getBundles();
   

   Not sure how that could work, if the have the same field names but different fields for it?

5. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/Entity.php
   @@ -22,4 +23,11 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function createDataDefinition($data_type) {
   +    return ContentEntityBase::createDataDefinition($data_type);
   +  }
   

   Interesting trick ;)

6. +++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
   @@ -17,7 +17,7 @@
     * Entity field items making use of this base class have to implement
   - * ComplexDataInterface::getPropertyDefinitions().
   + * the static function propertyDefinitions().
   

   function/method? :)

7. +++ b/core/lib/Drupal/Core/Field/FieldItemList.php
   @@ -41,6 +41,15 @@ class FieldItemList extends ItemList implements FieldItemListInterface {
       */
   +  public static function createDataDefinition($data_type) {
   +    // The data type of a field item is in the form of "field_item:$field_type".
   +    $parts = explode(':', $data_type, 2);
   +    return FieldDefinition::create($parts[1]);
   +  }
   

   This is a list, so the comment needs a small tweak.

   And hm, this means that the same data_type but a different definition class is a valid case? Interesting...

8. +++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
   @@ -218,7 +218,7 @@ public function form(array $form, array &$form_state) {
        $original = $comment->getUntranslated();
        foreach (array('cid', 'pid', 'entity_id', 'entity_type', 'field_id', 'uid', 'langcode') as $key) {
   -      $key_name = key($comment->$key->offsetGet(0)->getPropertyDefinitions());
   +      $key_name = key($comment->$key->offsetGet(0)->getDefinition()->getPropertyDefinitions());
          $form[$key] = array('#type' => 'value', '#value' => $original->$key->{$key_name});
   

   I guess this already conflicts with miy ListInterface::get() issue, but this should now be able to rely on something like $comment->getFieldDefinition($key)->getPropertyDefinitions(), no?

9. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Field/FieldType/TaxonomyTermReferenceItem.php
   @@ -89,14 +90,6 @@ public function getSettableOptions(AccountInterface $account = NULL) {
   -  public function getPropertyDefinitions() {
   -    $this->definition['settings']['target_type'] = 'taxonomy_term';
   -    return parent::getPropertyDefinitions();
   -  }
   

   Hm, do have an issue to remove ArrayAccess from FieldDefinition? We should have one... And isn't this now a Field? Why does this even work? So many questions :p

What I've been wondering is if we should open an issue to check what methods we currently get from TypedData, what we want to keep and what should be renamed? Then we could introduce them already and call the others, to keep the impact to calling code minimal and the patch size down for the adapter issue. Not sure it's worth it? For example, I would *love* to rename getValue() on field item and field item list to toArray(), that would allow us to get rid of the very problematic ->getValue() vs. ->value naming conflict.

@fago:

(yched) Couldn't we include the 'definition_class' in the DataType annotation, and then have DTManager->createDataDefinition() directly call XxDataDefinition::create(), skipping the step of going through the Data class ?

(fago) No, we can't as create() takes different arguments. We need to have some custom instantiation logic, so either we have it in the data type class or we need some sort of factory

Right, of course. Then other possibility:
- 'definition_class' in the DataType annotation
- XxDataDefinition holds the two static factories:
::create($whatever, $makes, $sense)
::createFromDataType($data_type)
That last one is the one called from DTManager->createDataDefinition(), and replaces the current XxData::createDataDefinition($data_type) (calls ::create() in the same class)

Not holding strongly to it, but maybe it simplifies the flow a bit - no need to go through the Data class, create() & createFromDataType() work hand in hand in the same class ?

Yay, +1 !

Would this have an impact on the logic around the $list param in DTM->createDataDefinition() ? (hmm, probably not ?)

It's the next area that I find a bit puzzling in the current patch :-)
I *think* I understand the need, but the boolean param is not really clear when you look at calling code:

$dtm->createDataDefinition('integer')
$dtm->createDataDefinition('integer', TRUE)
It's not really obvious that the those two calls produce things that are fairly different conceptually (single integer / list of integers)

I'd think the semantic difference is important enough to deserve two separate factory methods (createDataDefinition() / createListDataDefinition()), rather than one method with a boolean switch ?

Actually, just noticed :

+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
@@ -133,6 +133,43 @@ public function create(DataDefinitionInterface $definition, $value = NULL, $name
+   * @return \Drupal\Core\TypedData\DataDefinitionInterface|\Drupal\Core\TypedData\ListDataDefinitionInterface
+   *   A data or list definition for the given data type.
+   */
+  public function createDataDefinition($data_type, $list = FALSE) {

Not sure why the @return hints at the two interfaces since the latter extends the former, but this reinforces my feeling from #95 that two separate factory methods would be cleaner.

Finished my review at last !
A bit long, so splitting this in two posts.

1. +++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
   @@ -209,7 +246,7 @@ public function getPropertyInstance(TypedDataInterface $object, $property_name,
   +        $definition = $object->getDefinition()->getPropertyDefinition($property_name);
   

   In accordance with the renames that were done earlier, TypedDataInterface::getDefinition() should be getDataDefinition() ?

   FieldItem::getDefinition() (@return FieldItemDataDefinition) is kind of confusing next to
   FieldItem::getFieldDefinition() (@return FieldDefinition)
   (e.g IDE method autocomplete...)

2. +++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
   @@ -365,6 +402,12 @@ public function getConstraints(DataDefinitionInterface $definition) {
   +    // Add any constraints about referenced data.
   +    if ($definition instanceof DataReferenceDefinitionInterface) {
   +      foreach ($definition->getTargetDefinition()->getConstraints() as $name => $options) {
   +        $constraints[] = $validation_manager->create($name, $options);
   +      }
   +    }
   

   Feels a bit strange to have specific logic about DataReferenceDefinitionInterface in there.

   Can't the addition of the constraints on the target be the job of XxxReferenceDefinition->getConstraints() ? That way they would get collected by the existing call to $definition->getConstraints() 20-ish lines above ?
   [edit: if possible at all, could probably be left to a followup]

   + 3 lines above there's a similar Interface check but using is_subclass_of() rather than instanceof - harmonize ?

3. +++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
   @@ -24,6 +24,31 @@
   +  public static function getMainPropertyName();
   

   If we follow the same pattern as we did for the other static methods (schema(), propertyDefinitions()), getMainPropertyName() should be reserved for the real accessor on the Definition objects, while this static method here should be just mainPropertyname() ?

   Also, couldn't we provide a base implementation that just defaults to the first property defined in propertyDefinitions() ?

4. +++ b/core/modules/field/lib/Drupal/field/Entity/Field.php
   @@ -770,10 +771,65 @@ public function getConstraint($constraint_name) {
   +  protected function getFieldItemClass() {
   

   The existing Field::getSchema() method should probably switch to use this method too, then ?

5. +++ b/core/lib/Drupal/Core/Field/FieldDefinitionInterface.php
   @@ -256,6 +256,38 @@ public function isMultiple();
   +  public function getPropertyDefinition($name);
   ...
   +  public function getPropertyDefinitions();
   ...
   +  public function getMainPropertyName();
   

   Minor: code grouping - the existing FieldDefinitionInterface::getPropertyNames() should move to that group as well.

   Also regarding getPropertyNames() : only FieldDefinitionInterface has it - wouldn't it make sense in ComplexDatadefinitionInterface ?

6. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
   @@ -288,6 +295,71 @@ public function getDefaultValue(EntityInterface $entity) {
   +    if (!isset($this->propertyDefinitions)) {
   ...
   +  public function getPropertyDefinition($name) {
   ...
   +      $this->getPropertyDefinitions();
   +    }
   +    if (isset($this->propertyDefinitions[$name])) {
   +      return $this->propertyDefinitions[$name];
   

   A bit leaky - would be cleaner as simply :

   $definitions = $this->getPropertyDefinitions();
   if (isset($definitions[$name]) {... }

   Would keep the logic around "if not set, $this->propertyDefinitions will to be computed and set" contained in the single getPropertyDefinitions(), while it currently spills into getPropertyDefinition($name) as well.

   Same for implementaions in ContentEntityBase, ComplexDataDefinitionBase - & maybe other places ?

   (current code is actually weird in ComplexDataDefinitionBase because the code in getPropertyDefinition($name) assumes getPropertyDefinitions() will populate $this->propertyDefinitions, while the method is abstract / left for subclasses to implement)

7. +++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/Map.php
   @@ -36,21 +43,28 @@ class Map extends TypedData implements \IteratorAggregate, ComplexDataInterface
   +  protected function getPropertyDefinitions() {
   +    return $this->definition->getPropertyDefinitions();
   

   It's the last accessor to "property definitions" left on a Data object. I didn't really get whether this one was targeted for removal at some point ?

8. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/Entity.php
   @@ -22,4 +23,11 @@
    abstract class Entity {
   

   Side note:
   Drupal\Core\Entity\Plugin\DataType\Entity
   Drupal\Core\Entity\Plugin\DataType\EntityReference
   Drupal\Core\Field\Plugin\DataType\FieldItem

   Those really confuse the heck out of me each and every time I bump into them :-) We should really rename them at some point.

9. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
   @@ -691,7 +691,7 @@ protected function mapToStorageRecord(EntityInterface $entity, $table_key = 'bas
   -      foreach (array_keys($field_items[0]->getPropertyDefinitions()) as $property_name) {
   +      foreach (array_keys($field_items[0]->getDefinition()->getPropertyDefinitions()) as $property_name) {
   

   Maybe rather
   $field_items->getFieldDefinition()->getPropertyDefinitions() ?
   Same old, getDefinition() is typed data, we should not have to deal with that here.

   + Similar in CommentFormController

10. +++ b/core/lib/Drupal/Core/Entity/TypedData/EntityDataDefinitionInterface.php
    @@ -0,0 +1,55 @@
    +  /**
    +   * Returns the array of possible entity bundles.
    +   *
    +   * @return array|null
    +   *   The array of possible bundles, or NULL if the bundle is unknown or the
    +   *   entity type does not have bundles.
    +   */
    +  public function getBundles();
    

    So NULL and empty array() have different semantics ? Seems mighty confusing...
    + it's best to keep "functions that return iterable stuff" always returning iterable stuff rather than NULL.

    The method doesn't seem used anywhere except internally, but the existing uses are already a bit obscured by that ambiguity (the is_array() check in getPropertyDefinitions() feels like a code smell)

    Also, we should ensure that getBundles() / setBundles() are idempotent, and notably accept the same arguments with the same semantics (mlaybe that's the case, cannot tell for sure).

11. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
    @@ -72,4 +72,25 @@ public static function baseFieldDefinitions($entity_type);
    +  /**
    +   * Gets the definition of a contained field.
    +   *
    +   * @param string $name
    +   *   The name of the field.
    +   *
    +   * @return \Drupal\Core\Field\FieldDefinitionInterface|false
    +   *   The definition of the field or FALSE if the field does not exist.
    +   */
    +  public function getFieldDefinition($name);
    

    Why FALSE ? Why not NULL like in usual cases of "does not exist" ?

12. +++ b/core/lib/Drupal/Core/TypedData/ListDataDefinition.php
    @@ -34,12 +34,10 @@ public static function create($item_type) {
       public function __construct(array $definition = array(), DataDefinitionInterface $item_definition = NULL) {
    -    parent::__construct($definition);
    -    $this->itemDefinition = isset($item_definition) ? $item_definition : DataDefinition::create('any');
    +    $this->definition = $definition;
    +    $this->itemDefinition = $item_definition;
    

    $definition as array / $itemDefinition as DefinitionInterface is... puzzling :-)

    (probably because Definition::$definition is puzzling: in most places Object::$definition is "the definition of the object", so here it feels like "the definition of the definition", which makes terrifying recursive mental knots :-p)

    Cannot think of a better name for now for the "internal values of the Definition object", though. Also, not introduced here. I'll create a separate issue.
    [edit: opened #2187067: Find a better name for DataDefinition::$definition ]

More nitpicky stuff:

1. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
   @@ -288,6 +295,71 @@ public function getDefaultValue(EntityInterface $entity) {
   +  protected function getFieldItemClass() 
   +    if ($class = $this->itemDefinition->getClass()) {
   

   Striclty speaking, this should use $this->getItemDefinition() ?
   Field::getFieldItemClass() does, and it's the only difference between the two implementations.

   Related: not sure I understand why Field / FieldInstance need their own getItemDefinition() override, while FieldDefinition simply reuses the one from ListDataDefinition ?

2. +++ b/core/lib/Drupal/Core/TypedData/ComplexDataDefinitionBase.php
   @@ -0,0 +1,58 @@
   +  /**
   +   * An array of data definitions.
   +   *
   +   * @var \Drupal\Core\TypedData\DataDefinitionInterface[]
   +   *
   +   * @see ComplexDataDefinitionBase::getPropertyDefinitions()
   +   */
   +  protected $propertyDefinitions;
   

   Minor: the @see seems a bit overkill ? we don't usually add "@see theAssociatedGetter()" mentions on all class properties ?

3. +++ b/core/lib/Drupal/Core/TypedData/ComplexDataDefinitionBase.php
   @@ -0,0 +1,58 @@
   +  public function getPropertyDefinition($name) {
   ...
   +  public function __sleep() {
   ...
   +  public function getMainPropertyName() {
   ...
   +  abstract public function getPropertyDefinitions();
   

   Minor, code order - I'd suggest:
   getPropertyDefinitions()
   getPropertyDefinition($name)
   getMainPropertyname()
   __sleep()
   ?

   More generally, in all classes that implement the methods, putting getPropertyDefinitions() before getPropertyDefinition($name) would make more sense logically IMO.

4. +++ b/core/modules/editor/editor.module
   @@ -526,14 +526,14 @@ function _editor_get_file_uuids_by_field(EntityInterface $entity) {
   -  $properties = $entity->getPropertyDefinitions();
   ...
   +  $fields = $entity->getFieldDefinitions();
   ...
   -  if (empty($properties)) {
   

   s/$fields/$field_definitions ?

5. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldTest.php
   @@ -446,7 +475,7 @@ protected function assertIterator($entity_type) {
   +    $this->assertEqual(array_keys($properties), array_keys($entity->getDefinition()->getPropertyDefinitions()), format_string('%entity_type: All properties returned.', array('%entity_type' => $entity_type)));
   ...
   -    $this->assertEqual(array_keys($properties), array_keys($entity->getPropertyDefinitions()), format_string('%entity_type: All properties returned.', array('%entity_type' => $entity_type)));
   

   wouldn't that be $entity->getFieldDefinitions() ? (still slightly confused by "the definition of an $entity" :-)

6. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTypedDataDefinitionTest.php
   @@ -0,0 +1,137 @@
   +    // To ease access of field item property metadata, field definitions make
   +    // metadata about field item properties on the field level as well.
   

   Missing verb ;-)
   (+ sentence looks a bit awkward)

7. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTypedDataDefinitionTest.php
   @@ -0,0 +1,137 @@
   +    $field_item2 = $this->typedDataManager->createDataDefinition('field_item:integer');
   

   Unfortunate / misleading var name

8. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTypedDataDefinitionTest.php
   @@ -0,0 +1,137 @@
   +    $entity_definition2 = $this->typedDataManager->createDataDefinition('entity:node', FALSE);
   +    $this->assertFalse($entity_definition2 instanceof ListDataDefinitionInterface);
   ...
   +
   

   Minor : missing a comment above as to what this test section is doing (the other tests have "Test using the definition factory...")

9. +++ b/core/modules/system/lib/Drupal/system/Tests/TypedData/TypedDataDefinitionTest.php
   @@ -0,0 +1,110 @@
   +    // Test creating a single list item, which is the same as a list of any
   +    // items.
   

   Huh ? Creating a single item is the same as creating a list ?
   (also, the test creates definitions, not actual lists and items)

I swear, I was asleep half way through the patch review.

1. +++ b/core/lib/Drupal/Core/Config/Schema/Mapping.php
   @@ -108,7 +108,13 @@ public function setPropertyValues($values) {
   +   * @return array|FALSE
   

   I don't know the logic but get usually returns NULL and It should be false.

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -193,14 +185,7 @@ public function onChange($property_name) {
   +  ¶
   

   Whitespace.

3. +++ b/core/lib/Drupal/Core/TypedData/DataDefinitionInterface.php
   @@ -14,11 +14,37 @@
   +   * @return static
   
   +++ b/core/lib/Drupal/Core/TypedData/DataReferenceDefinition.php
   @@ -0,0 +1,72 @@
   +   * @return static
   

   static is not a datatype

4. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTypedDataDefinitionTest.php
   @@ -0,0 +1,136 @@
   +  public static function getInfo() {
   ...
   +  public function setUp() {
   

   Doc block missing.

5. +++ b/core/modules/system/lib/Drupal/system/Tests/TypedData/TypedDataDefinitionTest.php
   @@ -0,0 +1,110 @@
   +  public static function getInfo() {
   ...
   +  public function setUp() {
   

   Doc block missing.

For @return static, that should be used in factory methods like public static function create(), so that's fine.
When its returning itself, it should be @return $this.
So half of the ones in this patch are wrong (but not all!).

See #2158497: [policy, no patch] Use "$this" and "static" in documentation for @return types and https://drupal.org/coding-standards/docs#types

Thanks a lot for the updates @fago.

The split of two methods in TypeDataManager for 'data' & 'list' looks much cleaner.
I'm just surprised that this also turned into two separate createFromDataType() & createFromItemType() methods on the definition classes, which makes a slightly convoluted dance between create(), createFromDataType(), createFromItemType() in ListDataDefinition.
I guess I thought that, since the previous version was able to do both cases with just one single factory method on the DataType classes, similarly both TDM methods could route to the single [DefinitionClass]::createFromDataType().

But maybe that was not doable ? Well, if changeable at all, can be in a followup.

Other than that, final nitpicks:

re #98.1:
I was just pointing that FieldDefinition::getFieldItemClass() does
if ($class = $this->itemDefinition->getClass()) {
while FieldConfig::getFieldItemClass() does
if ($class = $this->getItemDefinition()->getClass()) {
Otherwise the methods are strictly the same. The latter seems more correct ?

(& ack for why FieldConfig can't inherit FieldDefinition::getFieldItemClass() - sure, silly me)

1. +++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
   @@ -226,7 +226,7 @@ public function form(array $form, array &$form_state) {
   -      $key_name = key($comment->$key->first()->getPropertyDefinitions());
   +      $key_name = key($comment->$key->first()->getFieldDefinition()->getPropertyDefinitions());
   

   Could be just $comment->$key->getFieldDefinition() now, no need for the ->first() ?

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/Entity.php
   @@ -6,6 +6,7 @@
   +use Drupal\Core\Entity\ContentEntityBase;
   

   Leftover, not needed anymore.

3. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
   @@ -39,15 +55,25 @@ class FieldDefinition extends ListDefinition implements FieldDefinitionInterface
   +    return FieldDefinition::create($parts[1]);
   

   Should be static::create($parts[1]); ?

4. +++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
   @@ -29,6 +29,13 @@ class DataDefinition implements DataDefinitionInterface, \ArrayAccess {
      public static function create($type) {
   +    return self::createFromDataType($type);
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function createFromDataType($type) {
        $definition['type'] = $type;
        return new static($definition);
   

   In most other cases it's createFromDataType() that wraps a call to create(), which sounds more intuitive - but here it's the opposite.
   (+ we call create() a lot more often than createFromDataType() ?)

5. +++ b/core/lib/Drupal/Core/TypedData/DataDefinitionInterface.php
   @@ -14,11 +14,37 @@
   +   * Creates a new data definition object.
   +   *
   +   * This method is usually called by the manager; i.e.,
   +   * \Drupal\Core\TypedData\TypedDataManager::createDataDefinition().
   

   Since create() is not in the interface (because its parameters vary), it might be good to mention it here ?

   Proposal:
   "This method is typically used by TDM::createDataDefinition() to build a definition object for an arbitrary $data_type. When the definition class is known, it is recommended to directly use the static create() method on that class instead.
   Example: $definition = MapDataDefinition::create();"

   And similar in ListDataDefinitionInterface::createFromItemType() ?

6. +++ b/core/modules/field/lib/Drupal/field/Entity/FieldConfig.php
   @@ -712,6 +713,24 @@ public function __wakeup() {
   +  public static function createFromItemType($item_type) {
   +    // Forward to the field definition class for creating new data definitions
   +    // via the typed manager.
   +    return FieldDefinition::createFromDataType($item_type);
   

   Surprised that it proxies to FieldDefinition::createFromDataType() rather than createFromItemType() ? Maybe worth a comment if that's intended ?
   (same in FieldInstanceConfig)

+++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
@@ -29,15 +29,15 @@ class DataDefinition implements DataDefinitionInterface, \ArrayAccess {
   public static function createFromDataType($type) {
-    $definition['type'] = $type;
-    return new static($definition);
+    return self::$type($type);
   }

self::$type($type) looks weird, might explain the install fail :-)

+ skipped #114.1 ?

Other than that, looks RTBC :-)

Yay. Let's go then!

Er ? Never mind #120, it's about a previous patch. #118 is green.

Minor : apparently there are still occurrences of "@return static" that should be "@return $this".
Also, according to #2158497: [policy, no patch] Use "$this" and "static" in documentation for @return types, no need for a description after "@return static" or "@return $this".

Only phpdoc fixes, so shamelessly leaving at RTBC.

So both this and #2200821: Rename Fieldinterface and FieldInstanceInterface are marked RTBC + Avoid commit conflicts, and they both stomp all over each other, unfortunately. :\ Before when such a thing has happened, yched asked me to commit the rename one despite the horrifically complex one being harder to re-roll, presumably because once it's committed it's done and will no longer conflict. Unfortunately Freenode is getting DDOSed atm, so I don't have a way to ask for a second opinion in real-time, so that's what I've just gone ahead and done. As a result, this one needs a re-roll. :( Sorry about that.

In general, it would be great if we could stop renaming things in entity field API real soon, as this has been going on for months, and it's always heart-breaking to see patches like this have to be re-rolled on account of slightly nicer names of things. :\

This also needs a draft change notice in order to be committable.

Committed 33e431f and pushed to 8.x. Thanks!

Did some minor tidy up during commit. DataReferenceDefinition::create did not match documentation - the argument name in the documentation was more helpful.

diff --git a/core/lib/Drupal/Core/TypedData/DataReferenceDefinition.php b/core/lib/Drupal/Core/TypedData/DataReferenceDefinition.php
index 9c58657..23fb420 100644
--- a/core/lib/Drupal/Core/TypedData/DataReferenceDefinition.php
+++ b/core/lib/Drupal/Core/TypedData/DataReferenceDefinition.php
@@ -30,10 +30,10 @@ class DataReferenceDefinition extends DataDefinition implements DataReferenceDef
    *
    * @return static
    */
-  public static function create($type) {
+  public static function create($target_data_type) {
     // This assumes implementations use a "TYPE_reference" naming pattern.
-    $definition = parent::create($type . '_reference');
-    return $definition->setTargetDefinition(\Drupal::typedDataManager()->createDataDefinition($type));
+    $definition = parent::create($target_data_type . '_reference');
+    return $definition->setTargetDefinition(\Drupal::typedDataManager()->createDataDefinition($target_data_type));
   }
 
   /**
diff --git a/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTypedDataDefinitionTest.php b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTypedDataDefinitionTest.php
index 3e0fe10..23a62e1 100644
--- a/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTypedDataDefinitionTest.php
+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTypedDataDefinitionTest.php
@@ -125,7 +125,7 @@ public function testEntityReferences() {
 
     // Test retrieving metadata about the referenced data.
     $this->assertEqual($reference_definition->getTargetDefinition()->getDataType(), 'entity');
-    $this->assertTrue($reference_definition->getTargetDefinition() instanceof \Drupal\Core\Entity\TypedData\EntityDataDefinitionInterface);
+    $this->assertTrue($reference_definition->getTargetDefinition() instanceof EntityDataDefinitionInterface);
 
     // Test that the definition factory creates the right definition object.
     $reference_definition2 = $this->typedDataManager->createDataDefinition('entity_reference');
diff --git a/core/modules/system/lib/Drupal/system/Tests/TypedData/TypedDataTest.php b/core/modules/system/lib/Drupal/system/Tests/TypedData/TypedDataTest.php
index 34a896c..b8ccc7c 100644
--- a/core/modules/system/lib/Drupal/system/Tests/TypedData/TypedDataTest.php
+++ b/core/modules/system/lib/Drupal/system/Tests/TypedData/TypedDataTest.php
@@ -11,7 +11,6 @@
 use Drupal\Core\TypedData\DataDefinition;
 use Drupal\Core\TypedData\ListDataDefinition;
 use Drupal\Core\TypedData\MapDataDefinition;
-use Drupal\Core\TypedData\Plugin\DataType\Map;
 use Drupal\simpletest\DrupalUnitTestBase;
 use Drupal\Core\Datetime\DrupalDateTime;
 

Yay, published the change records, updated the others as following:

- https://drupal.org/node/2064123/revisions/view/6884385/6954683
- https://drupal.org/node/1806650/revisions/view/6925467/6954713

Since this issue just introduced the file that is being referenced in the fatal error, cross-posting here:

#2205367: [HEAD BROKEN] PHP 5.4

Yay! So glad this is in. Nice work on the change record updates, also.