Problem/Motivation

Having generic metadata introspection is necessary for use-cases like the configuration of required context, but our current implementation has the following problems:
- The metadata introspection methods are generic, and pollute our objects with generic and confusing terminology - e.g. getPropertyDefinitions() on entities
- Getting metadata does right not require run-time objects. That's unfortunate when you do not have a run-time object and e.g. need to instantiate an entity just to have one, but moreover it's not able to properly handle cases where not enough metadata is available to instantiate a run-time object - e.g. when you do not know the bundle of a node, or even not the entity type.

Proposed resolution

The patch makes metadata accessible without the need for run time objects, and consequently frees us from having respective generally named methods like getPropertyDefinitions() our data objects. As we've recently moved to class based data definitions, the patch builds upon this classes and extends them - so those can be used for deriving metadata without the need for run time data objects.

In detail this means:

1) In HEAD, we've got only DataDefinition, ListDefinition and FieldDefinition. The patch extends this with some additional definition classes that implement the also newly added ComplexDataDefinitionInterface and DataReferenceDefinitionInterface. Those interfaces represent the methods needed for deriving metadata, i.e. are the result of moving respective run-time methods like ComplexDataInterface::getPropertyDefinitions() to ComplexDataDefinitionInterface::getPropertyDefinitions().

2) As suggested independently, we want to be able to get property definitions for fields without having actual field items. The patch implements this and lets people define the field item properties in a static method instead (see API changes).
Still, field items implement a respective field item definition that complies with the typed data interfaces and make the property definitions of field items available via the definition class. In addition to that, it's the goal to make FieldDefinition + FieldDefinitionInterface being the primary definition objects for fields - independently of typed data. This patch does not do that yet, but it works towards that by letting the FieldDefinition know about contained properties already + making the FieldItemDefinition implementation just a dumb wrapper around it, just needed for typed-data purposes. Field items should be using the field definition object directly, so they do not have to worry about the typed data implementation. -> todo for #2002138: Use an adapter for supporting typed data on ContentEntities and it's spin-offs.

3)So, as the patch frees us from having ContentEntityInterface::getPropertyDefinitions() it renames that method to go with a more straight forward naming, i.e. getFieldDefinitions(), as seen at the list of API changes.

4) Some definition classes added are MapDefinition, EntityDefinition and DataReferenceDefinition.

  • The MapDefinition class allows one to specify to describe how a map would look like independently from having a map + work with that. Right now, typed data maps are just used as dump containers for any arrays, but given the MapDefinition it's now possible to add meaningful metadata for those as well.
  • EntityDefintion is used for describing entities and retrieving further metadata from them, see 5.
  • DataReferenceDefinition serves for describing the DataReference data types we have, two quite special data types for representing references in a generic typed-data fashion. We only have a "language" and a "entity" references right now. As for the others this class just splits the metadata part of the implementation out and takes it to the definition classes. It lets us to know e.g. $node->author->entity is a regular user entity without having to ask the data instances :-)

5) The patch finally allows us to derive metadata from entities without having actually instantiated entity objects, what is an quite important, necessary feature for use cases like ctools / block&layouts and rules. That's now solved by an EntityDefinition class implementing the respective interfaces, and works e.g. as the following:

<?php
    $user_definition
= $entity_definition->getPropertyDefinition('user_id')
      ->
getPropertyDefinition('entity')
      ->
getTargetDefinition();

    print
$user_definition->getDataType();
   
// entity:user

   
$value_definition = $user_definition->getPropertyDefinition('name')
      ->
getPropertyDefinition('value');
  
   print
$value_definition->getDataType();
  
// string
?>

6) As there are now multiple, different definition classes like EntityDefintion, FieldDefinition and DataDefinition the problem of having to know the right class comes up (and exists already in HEAD). To solve this, the patch adds a factory for definition class that gives you the right definition class for any data type, e.g:

<?php
$node_definition
= $this->typedDataManager->createDataDefinition('entity:node');
?>

That's necessary for generically working modules like Rules, PageManager, etc. to work from the data type string only. If you know you'll work with an entity, or a field up-front - developers can still instantiate the classes directly for improved DX, i.e. keep using:

<?php
FieldDefinition
::create('integer')->getPropertyDefinitions()
?>

Given that, the patch fixes the existing typed data implementation to allow for deriving metadata about e.g. something with data type "entity:node".

Finally, the patch adds test-coverage for the new/moved APIs.

Remaining tasks

none.

User interface changes

None.

API changes

  • Field types need to specify their field item properties in static::propertyDefinitions($field_definition) instead of getPropertyDefinitions() and static::mainPropertyName() instead of getMainPropertyName()
  • ComplexDataInterface::getPropertyDefinition($name) and ComplexDataInterface::getPropertyDefinitions() got removed in favor of the respective methods on the newly added ComplexDataDefinitionInterface. There aren't many usages of the previous methods in core, but converting is as easy as replacing $data->getPropertyDefinitions() with $data->getDataDefinition()->getPropertyDefinitions(). That's mostly the case on field items.
  • getFieldDefinitions() is added to the ContentEntityinterface, which replaces the previously inherited ComplexDataInterface::getPropertyDefinitions() method. Analagously, getFieldDefinition($name) replaces ComplexDataInterface::getPropertyDefinition($name).
  • TypedDataInterface::getDefinition() has been renamed to TypedDataInterface::getDataDefinition() to better distinguish it from plugin definitions and field definitions.

Files: 
CommentFileSizeAuthor
#127 d8_definition_metadata.patch186.85 KBfago
PASSED: [[SimpleTest]]: [MySQL] 64,642 pass(es).
[ View ]
#125 d8_definition_metadata.based-on-rename.patch.txt186.85 KBfago
#125 d8_definition_metadata.combined-with-rename.patch244.16 KBfago
PASSED: [[SimpleTest]]: [MySQL] 64,696 pass(es).
[ View ]
#125 d8_definition_metadata.patch186.54 KBfago
PASSED: [[SimpleTest]]: [MySQL] 64,680 pass(es).
[ View ]
#122 interdiff.txt5.42 KByched
#122 d8_definition_metadata-2002134-122.patch187.43 KByched
PASSED: [[SimpleTest]]: [MySQL] 64,639 pass(es).
[ View ]
#118 d8_definition_metadata.patch187.09 KBfago
PASSED: [[SimpleTest]]: [MySQL] 64,638 pass(es).
[ View ]
#118 d8_definition_metadata.interdiff.txt1.43 KBfago
#115 d8_definition_metadata.interdiff.txt6.45 KBfago
#115 d8_definition_metadata.patch186.98 KBfago
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#112 d8_definition-metadata.interdiff.txt2.6 KBfago
#112 d8_definition-metadata.patch186.84 KBfago
PASSED: [[SimpleTest]]: [MySQL] 64,690 pass(es).
[ View ]
#110 d8_definition-metadata.interdiff.txt670 bytesfago
#110 d8_definition-metadata.patch186.85 KBfago
PASSED: [[SimpleTest]]: [MySQL] 64,690 pass(es).
[ View ]
#108 d8_definition-metadata.patch186.77 KBfago
PASSED: [[SimpleTest]]: [MySQL] 64,676 pass(es).
[ View ]
#106 d8_definition-metadata.patch186.73 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8_definition-metadata_3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#106 d8_definition-metadata.interdiff.txt1.9 KBfago
#102 d8_definition-metadata.interdiff.txt19.5 KBfago
#102 d8_definition-metadata.patch185.81 KBfago
FAILED: [[SimpleTest]]: [MySQL] 63,138 pass(es), 177 fail(s), and 44 exception(s).
[ View ]
#100 d8_definition-metadata.patch180.35 KBfago
FAILED: [[SimpleTest]]: [MySQL] 64,547 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#100 d8_definition-metadata.interdiff.txt44.98 KBfago
#99 d8_definition_metadata.patch164.91 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8_definition_metadata_17.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#89 d8_definition_metadata.interdiff.txt39.41 KBfago
#89 d8_definition_metadata.patch165.33 KBfago
FAILED: [[SimpleTest]]: [MySQL] 63,422 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#86 typed_data_metadata_introspection-2002134-86.patch160.34 KByched
PASSED: [[SimpleTest]]: [MySQL] 63,186 pass(es).
[ View ]
#82 d8_definition-metadata.patch160.35 KBfago
PASSED: [[SimpleTest]]: [MySQL] 63,090 pass(es).
[ View ]
#82 d8_definition-metadata.interdiff.txt4.3 KBfago
#77 d8_definition-metadata.patch159.21 KBfago
PASSED: [[SimpleTest]]: [MySQL] 60,078 pass(es).
[ View ]
#77 d8_definition-metadata.interdiff.txt9.51 KBfago
#73 interdiff.txt5.28 KBamateescu
#73 d8_definition_metadata-73.patch156.58 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 60,101 pass(es).
[ View ]
#71 d8_definition_metadata.interdiff.txt1.79 KBfago
#71 d8_definition_metadata.patch155.29 KBfago
PASSED: [[SimpleTest]]: [MySQL] 60,110 pass(es).
[ View ]
#67 d8_definition_metadata.interdiff.txt1.09 KBfago
#67 d8_definition_metadata.patch152.31 KBfago
FAILED: [[SimpleTest]]: [MySQL] 59,559 pass(es), 211 fail(s), and 6 exception(s).
[ View ]
#65 d8_definition_metadata.interdiff.txt56.69 KBfago
#65 d8_definition_metadata.patch152.67 KBfago
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#63 d8_definition_metadata.patch111 KBfago
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/telephone/lib/Drupal/telephone/Plugin/Field/FieldType/TelephoneItem.php.
[ View ]
#63 d8_definition_metadata.interdiff.txt684 bytesfago
#60 d8_definition_metadata.patch111.24 KBfago
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/entity_reference/lib/Drupal/entity_reference/ConfigurableEntityReferenceItem.php.
[ View ]
#58 d8_definition_metadata.interdiff.txt22.73 KBfago
#58 d8_definition_metadata.patch111.12 KBfago
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/comment/lib/Drupal/comment/Plugin/Field/FieldType/CommentItem.php.
[ View ]
#50 d8_definition_metadata.patch110.89 KBfago
PASSED: [[SimpleTest]]: [MySQL] 59,340 pass(es).
[ View ]
#50 d8_definition_metadata.interdiff.txt11.81 KBfago
#49 d8_definition_metadata.interdiff.txt2.24 KBfago
#49 d8_definition_metadata.patch101.85 KBfago
PASSED: [[SimpleTest]]: [MySQL] 59,985 pass(es).
[ View ]
#47 d8_definition_metadata.patch101.96 KBfago
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#46 d8_definition_metadata.patch101.99 KBfago
PASSED: [[SimpleTest]]: [MySQL] 60,062 pass(es).
[ View ]
#46 d8_definition_metadata.interdiff.txt1.3 KBfago
#45 d8_definition_metadata.interdiff.txt13.32 KBfago
#45 d8_definition_metadata.patch101.69 KBfago
PASSED: [[SimpleTest]]: [MySQL] 60,076 pass(es).
[ View ]
#42 d8_definition_metadata.patch91.88 KBfago
PASSED: [[SimpleTest]]: [MySQL] 59,850 pass(es).
[ View ]
#42 d8_definition_metadata.interdiff.txt11.81 KBfago
#42 d8_definition_metadata.patch91.88 KBfago
PASSED: [[SimpleTest]]: [MySQL] 60,040 pass(es).
[ View ]
#39 d8_definition_metadata.interdiff.txt1.12 KBfago
#39 d8_definition_metadata.patch81.34 KBfago
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#34 d8_definition_metadata-2002134-27.patch81.33 KBfago
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#33 d8_definition_metadata.patch81.32 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8_definition_metadata_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#27 d8_definition_metadata-2002134-27.patch81.33 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#26 d8_definition_metadata.interdiff.txt642 bytesfago
#26 d8_definition_metadata.patch81.32 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8_definition_metadata_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#24 d8_definition_metadata.interdiff.txt5.8 KBfago
#24 d8_definition_metadata.patch82.31 KBfago
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#11 d8_move_metadata.patch79.39 KBfago
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#11 d8_move_metadata.interdiff.patch67.47 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8_move_metadata.interdiff.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#9 2002134-do-not-test.patch55.48 KBamateescu

Comments

fago’s picture

Issue tags:+Entity Field API

adding tag

dixon_’s picture

Assigned:Unassigned» dixon_
dixon_’s picture

Assigned:dixon_» Unassigned

wrong issue

fago’s picture

Issue tags:+Typed sanity

tagging

fago’s picture

Priority:Normal» Major

As this is necessary to do away with the confusing $entity->getPropertyDefinitions() (it's all fields now!), bumping to major.

fago’s picture

Given #2047229: Make use of classes for entity field and data definitions introduced classed data definitions, we might be able to use them for solving this.

So what about:

  • Having a ComplexDataDefinition base class along with an interface, similar to ListDefinition + interface. That class would have a getPropertyDefinitions() method which acts as the primary API for getting child properties based upon definitions only.
  • Complex data types can define their property definitions in a static method on the class, i.e. become ComplexData::getPropertyDefinitions().
  • I don't think we'd actually use definition class called "ComplexDataDefinition", but have different implementations depending on the type of complex data, i.e. field items, entities, or maps. So the place where those property definitions get defined could vary by complex data type - i.e. for maps it could be part of the definition, field items have the static method and for entities it would get the field definitions.

So that would solve #2150555: Change field types to define their property definitions statically and allow us to do away with $entity->getPropertyDefinitions(). Still, you can get them in a generic fashion using $typed_data->getDefinition()->getPropertyDefinitions().

yched’s picture

Sounds like a plan :-)

amateescu’s picture

+1 from me as well.

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

amateescu’s picture

Issue summary:View changes
Status:Active» Needs work
StatusFileSize
new55.48 KB

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

fago’s picture

Assigned:Unassigned» fago

Thanks, as discussed I'll take care of this.

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new67.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8_move_metadata.interdiff.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new79.39 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

ok, here is a first step. It implements the property definitions via the static method, but does not yet remove the method from complex-data-definition interface.

I decided to implement the methods on the Field definition class instead of the Field Item class, so the logic around properties and schema will centered there. Furthermore, that way the field item class stays more dumb and just translates between field definition and typed data definitions. That should make the transition easier when we switch to the typed data adapter approach also.

This also breaks how the Map class works, as it doesn't magically create property definitions for everything you put into it. Imo the new way does make more sense, and I doubt the Map class is already directly used somewhere (except from acting as a base class where that part is always overridden anyway).

So let's see what the bot says & feedback wanted.

The last submitted patch, 11: d8_move_metadata.interdiff.patch, failed testing.

Berdir’s picture

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

fago’s picture

yeah, that seems reasonable but does it need to have metadata or properties generated on the fly? Afaik it's only used for storing an array of values, but does not use properties or metadata for the values.

Status:Needs review» Needs work

The last submitted patch, 11: d8_move_metadata.patch, failed testing.

xjm’s picture

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.

xjm’s picture

Issue tags:+beta target
fago’s picture

Issue summary:View changes

Updated the issue summary

fago’s picture

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

What would the big impact for contrib? Yes, it's a wanted/needed feature for ctools/rules context-configuration stuff - but does that mean it should block beta? I doubt many devs would ran into this and miss their configuration-time metadata API if it's not completed with beta. I mean - I and EclipseGc might, but else I'm not sure.

However, sure it would be nice to have it fixed with the beta, but that doesn't mean it should block it.

Still, we should make sure to settle on the public API to get metadata about field item lists and field items before beta. We've getFieldDefinition(), which is fine - but the question is more of what we'd like to do away with once we solve #2002138: Use an adapter for supporting typed data on ContentEntities, e.g. mark them as deprecated already.

EclipseGc’s picture

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

andypost’s picture

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

yched’s picture

On visual review, patch looks good to me.

amateescu’s picture

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.

fago’s picture

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new82.31 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new5.8 KB

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. But for fields, everyone knows the structure so I think this is how we'd do it for field definitions being decoupled from data definitions.

@MapItem:
The changes just change how dynamic registration of properties, i.e. map values works - but I think that's totally unused. I've added some code that should keep that working for now - however MapItem's implementation seem to ignore the set(),get() part of the API already, so I'm pretty sure it's not used anywhere. I'd suggest cleaning this up + adding tests for it a dedicated issue, but as it's unused that's not of any priority.

I addressed points 1-3 and re-rolled the patch, see attached patch + updated the issue summary - of course there is an API change included as the way field item properties are defined is changed to work with a static. There are just no API changes for API consumers, so I updated the summary to reflect that.

So one issue this moves introduces is that we now have several kind of definition class, what means will need some sort of factory for them. Having to do MapDefinition::create('map') is not only repetitive, but moreover it doesn't work generically when you only have the data type 'map'. So for completing this issue we'll need to allow the definition of custom definition classes at the data type definition and add some sort of factory for them.

Finally, we need to take care of entity definitions, i.e. implement a definition class for them which returns the respective metadata + add tests that ensure entity metadata can be properly dealt with without entity objects or field objects.

As there is quite a bit more todo, I think it's better to move on with an interim step which basically just covers #2150555: Change field types to define their property definitions statically, then complete it in a follow-up (which shouldn't touch so many different classes).

The last submitted patch, 24: d8_definition_metadata.patch, failed testing.

fago’s picture

StatusFileSize
new81.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8_definition_metadata_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new642 bytes

just a quick re-roll + fixed missing class import

effulgentsia’s picture

StatusFileSize
new81.33 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
effulgentsia’s picture

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

The last submitted patch, 26: d8_definition_metadata.patch, failed testing.

The last submitted patch, 26: d8_definition_metadata.patch, failed testing.

fago’s picture

Status:Needs review» Needs work
fago’s picture

Status:Needs work» Needs review
fago’s picture

StatusFileSize
new81.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8_definition_metadata_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
fago’s picture

StatusFileSize
new81.33 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

testbot, please test! ;-)

The last submitted patch, 27: d8_definition_metadata-2002134-27.patch, failed testing.

The last submitted patch, 33: d8_definition_metadata.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 34: d8_definition_metadata-2002134-27.patch, failed testing.

fago’s picture

ad #28: I've been thinking about that as well. #34 already does more than necessary to move the static as it defines the new public API for handling metadata already + implements that for fields. I don't think it can be easily separated as the new way to define it should work along with how typed data metadata gets separated in general.

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, i.e. a follow-up?

fago’s picture

StatusFileSize
new81.34 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new1.12 KB

re-rolled the patch just fixing more typed-data renames.

fago’s picture

Status:Needs work» Needs review

The last submitted patch, 39: d8_definition_metadata.patch, failed testing.

fago’s picture

StatusFileSize
new91.88 KB
PASSED: [[SimpleTest]]: [MySQL] 60,040 pass(es).
[ View ]
new11.81 KB
new91.88 KB
PASSED: [[SimpleTest]]: [MySQL] 59,850 pass(es).
[ View ]

Fixed field definition test and added a dedicated metadata test that has coverage for deriving metadata from list, map and field definitions. The test case shows that we need a factory now for being able to retrieve metadata about an unknown $type.

effulgentsia’s picture

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?

fago’s picture

Yes, that would help - if we find a good way to do it, also see [#8310465-4].

If it's just to keep this patch size manageable/reviewable..

Yeah, my intent was to reduce patch sizes & the amount of necessary re-rolls. However, I think we are not far from completing it + removing the old variant as well, so if that worries you let's do it one step.

fago’s picture

StatusFileSize
new101.69 KB
PASSED: [[SimpleTest]]: [MySQL] 60,076 pass(es).
[ View ]
new13.32 KB

Attached patch implements the definition factory and adds test coverage for it - that way keep being able to generically use of definitions to retrieve metadata about any given data type.

Please review.

fago’s picture

StatusFileSize
new1.3 KB
new101.99 KB
PASSED: [[SimpleTest]]: [MySQL] 60,062 pass(es).
[ View ]

Improved list metadata tests.

fago’s picture

StatusFileSize
new101.96 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

quick re-roll

Status:Needs review» Needs work

The last submitted patch, 47: d8_definition_metadata.patch, failed testing.

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new101.85 KB
PASSED: [[SimpleTest]]: [MySQL] 59,985 pass(es).
[ View ]
new2.24 KB
fago’s picture

StatusFileSize
new11.81 KB
new110.89 KB
PASSED: [[SimpleTest]]: [MySQL] 59,340 pass(es).
[ View ]

Attached patch implements entity definitions accordingly, please review.

What remains for a complete patch is
- moving getting datareference definitions to definitions
- removing old getPropertyDefinition[s]() methods

fago’s picture

oh, I forgot to explain:
Above interdiff removes getFieldDefinitionsByConstraints() which previously was intended to ease getting metadata based on entity reference data definitions. That's obsolete now as the metadata can be easily retrieved from the returned entity definition. (actually only once the datareference definition got updated to use it -> fixing datareference definitions is todo)

effulgentsia’s picture

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.

effulgentsia’s picture

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?

effulgentsia’s picture

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

effulgentsia’s picture

Title:Move TypedData metadata introspection out of the way» Move TypedData metadata introspection from data objects to definition objects

Retitling for clarity/precision

yched’s picture

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

amateescu’s picture

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.

fago’s picture

StatusFileSize
new111.12 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/comment/lib/Drupal/comment/Plugin/Field/FieldType/CommentItem.php.
[ View ]
new22.73 KB

Thanks for the reviews!

#54, 1: k, done.

#54, 2: yeah, was thinking about that as well. It's nice to have everything in one place, but I agree so I've separated stuff into an EntityTypedDataDefinition test.

#54, 3: right - I started with $fields as we use that in baseFieldDefinitions and then continued with it. I disliked either, but I thought I wait until I get feedback on that. Feedbacks there, and variables are re-named :-)

#54, 4,5: good, point - implemented.

#57.1: fixed

#57.2: opened #2169813: Support deriving fields from entity definitions with multiple bundles.

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.

The problem is a field cannot be a list and complex data at the same time, it's primarily a list which contains complex data. If it would implement both interface generic code working just on top of typed-data interfaces won't be able to know whether it should treat it as a list or as complex data. We could bake the assumption into typed data that if its both, it's primarily a list (as it is for fields), but I don't think this would be a good idea as it's more unclear and could easily a bug if some code just checks for ComplexDataInterface first.
We do the same already for the typed data objects, fields do not implement ComplexDataInterface - just FieldItemListInterface has chosen to take some of its methods over + documents them to be forwarded.

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

The idea is that it's primarily field definitions that have all the information - such that all the information is available when working with fields. The item definition though is there to be inline with the typed data API - so once we are doing #2002138: Use an adapter for supporting typed data on ContentEntities we could do that away by implementing an adapter approach for the definitions as well. But if that's confusing as it is right now, it might be better to just implement it on field item definitions (where the API requires it) and leave it out from field definitions for now. Thoughts?

#57.5: Yes, imo it makes sense to support it generally and we already had a todo for doing it (but didn't as previsouly we would have to add to all entity objects).

#57.6: It clarifies the usage of the data type parameter for lists, which is actually a bit different. This might be a bit hacky as the usage of the parameter depens on whether it's a list or not. As alternative we could extend the method to receive two types, the list types + the list item type. Then the first one is basically useless, but we are more conform with *both* interfaces.

#57.7: done

The last submitted patch, 58: d8_definition_metadata.patch, failed testing.

fago’s picture

StatusFileSize
new111.24 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/entity_reference/lib/Drupal/entity_reference/ConfigurableEntityReferenceItem.php.
[ View ]
new3.1 KB

Re-rolled and removed duplicated field-definition use statements Git left for me.

The last submitted patch, 60: d8_definition_metadata.patch, failed testing.

The last submitted patch, 60: d8_definition_metadata.patch, failed testing.

fago’s picture

StatusFileSize
new684 bytes
new111 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/telephone/lib/Drupal/telephone/Plugin/Field/FieldType/TelephoneItem.php.
[ View ]

and one more after merging latest changes.

Status:Needs review» Needs work

The last submitted patch, 63: d8_definition_metadata.patch, failed testing.

fago’s picture

Issue summary:View changes
Status:Needs work» Needs review
Issue tags:-Needs issue summary update+Avoid commit conflicts
StatusFileSize
new152.67 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new56.69 KB

ok, so here is an updated patch which implements the remaining tasks.

It adds a DataReferenceDefinitionInterface and a respective class for defining the entity + language reference, including tests. I've also complemented the already existing entity-field-api introspection test-coverage as integration test that goes through all levels and references.

Then, I've removed the old API as discussed previously, in detail it does the following:

  • ComplexDataInterface::getPropertyDefinition($name) and ComplexDataInterface::getPropertyDefinitions() got removed in favour of the respective methods on the ComplexDataDefinitionInterface. There aren't many usages of that in core, but converting is as easy as replacing $data->getPropertyDefinitions() with $data->getDefinition()->getPropertyDefinitions()
  • For content entities getFieldDefinitions() is added to the interface, which replaces the previously inherited ComplexDataInterface::getPropertyDefinitions(). I've removed ContentEntityInterface::getPropertyDefinition($name) as there was no single usage except internal usage, so I only added an internal ContentEntityInterface::getFieldDefinition($name) to ease migration.
  • Similarly there is now an internal FieldItem::getPropertyDefinitions() method to ease migration.

As this changes some APIs now, I've updated the issue summary accordingly. Also, tagging as "avoid commit conflicts".

The last submitted patch, 65: d8_definition_metadata.patch, failed testing.

fago’s picture

StatusFileSize
new152.31 KB
FAILED: [[SimpleTest]]: [MySQL] 59,559 pass(es), 211 fail(s), and 6 exception(s).
[ View ]
new1.09 KB
yched’s picture

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

The last submitted patch, 67: d8_definition_metadata.patch, failed testing.

fago’s picture

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 ?

hm, I'm not sure I can follow - how do you see that being confusing, with what?

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

It's the typed data definition of the field item object, i.e. what you get when you do $field_item->getDefinition(). Yes, this is confusing but that's not something this patch adds - the patch just changes the class of the returned definition.

Howsoever, please see my thoughts at #2002138-51: Use an adapter for supporting typed data on ContentEntities on improving method names.

Could we make it more specific in the class name that it's about TypedData ?

Do you have any suggestions?

I just continued the naming pattern already laid out by DataDefinition & ListDefinition. I could see us moving them in TypedData sub-namespaces what might help to clarify the class usage, but then it's a bit weird when FieldDefinition stays out of it? It would fit better if we decouple FieldDefinition from being a typed data definition though, i.e. implement a TypedData\FieldItemListDefinition class which translates between the typed data and field definition interfaces?

Regarding naming - the patch continues with the "DataDefinition" name already existing in core, and adds e.g. createDataDefinition() methods. I could see us going with TypedDataDefinitionInterface and createTypedDataDefinition() though. It's longer but probably clarifies the relationship to typed data better. Opinions?

fago’s picture

StatusFileSize
new155.29 KB
PASSED: [[SimpleTest]]: [MySQL] 60,110 pass(es).
[ View ]
new1.79 KB
webchick’s picture

Status:Needs review» Needs work

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

amateescu’s picture

Status:Needs work» Needs review
StatusFileSize
new156.58 KB
PASSED: [[SimpleTest]]: [MySQL] 60,101 pass(es).
[ View ]
new5.28 KB

Rerolled.

yched’s picture

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

yched’s picture

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.

yched’s picture

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 ?

fago’s picture

StatusFileSize
new9.51 KB
new159.21 KB
PASSED: [[SimpleTest]]: [MySQL] 60,078 pass(es).
[ View ]

#74.1 :)
#74.2 - fixed
#74.3:

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

I just left it there to ease migration, so the existing field items using it won't break. I don't think we should have it as public helper or encourage using that as people should be aware it's one the definition, and one intermediate call shouldn't be a big deal either? Also, if we'd have both I fear people might be wondering about the difference?

#74.4: Nope, it's just that the other defined properties continue to have NULL values, thus appear in the list of keys. Previously the test had no defined properties in the map. So the changed behavior of "map" now is, that you can actually define properties of the map up-front - i.e. it becomes usable ;)

#74.6: Not sure, I like the style of always using a separate line for each setter as it makes things easier to read. I think our guidelines would allow for both though. I'd vote for a coding style using new lines always, as you'd do with assignments though. Maybe we should open a separate issue to discuss chained setter coding styles, or is there something like that already?

#74.7: Right, thanks.

74.8: Done.
74.9: Not sure, imho all the nested method calls make the code rather unreadable then. I cleaned it up by removing the $target_type variable instead for now.
74.10: Good catch - cleaning those up is a joy.

75.2: Right, fixed.
75.3: hm, yes - as the setters of MapDefinition make no sense for FieldItemDefinitions. It's a bit strange to have the inheritance of FieldItem objects of Maps, but not on the definition side - but I think it's actually right and ok. A field item can be used like a map, but a field item definition cannot be used like a map definition. Maybe you could say that you cannot use a field item like a map if that won't work for the definition's as well though - so that would mean the "Map" should become a trait which is used by FieldItemBase + Map. Not nothing we can do now though - should we still file an issue for it and postpone it on php5.4 being usable?

75.4: Good point - not necessarily introduced by this issue though. It is already the case in HEAD that when you want to define a list, you have to use ListDefinition and when you want to define a field - use field definition. So you have to know which definition class to use - OR - you use the factory which gets introduced by this patch. So this patch is actually helping to solve it.

I don't think we should do PrimitiveDataDefinition - that does not sound nice to me. Also, it seems to be good to have the simple, stupid DataDefinition class there for everyone not rolling its own (and mostly you shouldn't have to), e.g. the Email data type which is not primitive. It could be PropertyDefinition - but it's not necessarily used as property either.. So imho the general DataDefinition is ok.

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?

Right now it feels I could do both MapDefinition::create() & DataDefinition::create('map'), but not sure what would work / break.

You would not get an instance of MapDefinition, i.e. your definition would not tell you about the contained map properties + the code will fatal when it tries to use it based on ComplexDataDefinitionInterface which is not there.

ad #75.1: This suggestions look good to me. I'm just not sure about DataReferenceDefinition - we've got DataReferenceInterface & DataReferenceBase already.
So would those become just ReferenceInterface + ReferenceBase as well? It's a generic name, but that's ListInterface as well + it's under the typed data namespace anway - so I'd be ok with renaming both.

Apart from that I think the comment should clarify the relation to typed data as well, e.g. for ListDefinition a comment A class for defining lists. could become A typed data definition class for defining lists.?

Before I go ahead and implement this, do others agree with the suggestion #75.1 as well or are there other ideas?

fago’s picture

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

Sry for that, indeed this is missing a good actual summary besides API changes. I'll add one.

Not sure it was reasonable to do all this in one patch ?

Yeah, I'd have split it up into more patches also, but effulgentsia was requesting the deprecation of the previous APIs in the same patch - see #44.

fago’s picture

Issue summary:View changes

added the summary.

fago’s picture

Issue summary:View changes
fago’s picture

Issue summary:View changes
fago’s picture

StatusFileSize
new4.3 KB
new160.35 KB
PASSED: [[SimpleTest]]: [MySQL] 63,090 pass(es).
[ View ]
yched’s picture

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.

fago’s picture

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

There is no extra massaging in XxData::createDataDefinition() - it takes care of creating a new definition given the data type only. However, compared to class::create() there might be some stuff to take care as Class::create() does not take the $data_type as argument - but whatever makes sense for the respective class. e.g. field definitions take the field type, entity definitions the entity type. So, XxData::createDataDefinition() just does the translation between domain-specific and general typed-data types.

So it really just is:
- code that does a) knows the semantics of the specific XxDefinition class and can directly create a definition of the specific definition class, e.g. MapDefinition.
- code that b) only has a $data_type string, and doesn't know the underlying definition class needed, thus the data class associated to $data_type creates the right definition class.

If so, yeah, I guess that would deserve better explanation in the phpdocs for those three places:..

Agreed.

ad 4. DataReferenceDefinition :
ok, let's keep that then.

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?

Either,
a) just document where the factory can be found at DataDefininition::create() or
b) let DataDefinition::create($data_type) internally use the factory - so you'd not necessarily get an instance of DataDefinition. That way DataDefinition::create() would always work right - however, it might be confusing that another class is returned?

webchick’s picture

Status:Needs review» Needs work
Issue tags:-Avoid commit conflicts

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.

yched’s picture

Status:Needs work» Needs review
StatusFileSize
new160.34 KB
PASSED: [[SimpleTest]]: [MySQL] 63,186 pass(es).
[ View ]

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

yched’s picture

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

Berdir’s picture

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’s picture

StatusFileSize
new165.33 KB
FAILED: [[SimpleTest]]: [MySQL] 63,422 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new39.41 KB

ad #87:

>If that last step calls the DTManager factory itself, we have a loop :-)

Well, createDataDefintion() could just go with new DataDefintiion(); Howsoever - I'm unsure whether we should do this either, so I'd propose we just improve documentation for now and get this bastard in. If it turns out to that people tend to use DataDefinition::create() although they shouldn't we can still do the factory-change later on.

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 ?

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. Then, we need it for the data type and the list classes, so we'd need even two factories.
While I agree that the definition precedes the data object, I don't think it's really problematic. It's a static method, so it actually precedes the data object, then it's not uncommon to have static methods like that. E.g. the static schema method on field items is quite similar as it's used to generate schema preceding the field item data. :-)

ad #88:
1. I think it will go with the adapter issue. However, this is yet to be determined and the returned definition makes sense now, so I've removed the todo for now.

2. There was only a single usage, so I thought it's unneeded. But ok, added it.

3. true, although it's mostely not updated yet. Still, changed it to be up2date.

4. It can return the shared FieldStorageDefinitions once we've them ;-)

6. fixed

7. hm, no the comment is actually accurate. The list gets the type of list item passed, which is used to instantiate the right list definition (being the field definition). Not sure what you mean with the different definition classes?

8. yes, already ran into merge conflicts ;-)

9. hm, we need an issue to remove array-access from all data definitions - yes. It's blocked by #1928868: Make typed config valid typed data though.

>And isn't this now a Field? Why does this even work? So many questions :p
What is a field? *confused* Yes fields are entities. (just kidding :)

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.

Yes, as discussed here that's the minimum we have to get done before beta. I think it would be good to have a separate issue for it to avoid derailing the typed-data-adapter issue on it too much. so +1 for doing that separately.

Then, we are still missing feedback of yched and my proposal of #75.1. So as there were no objects I'll assume it's good :-P -> Implemented it.

So if there are no major objections left I think we should move on with this asap to avoid running into further conflicts + move on with other important issues and the typed data adapter work.

Updated patch attached.

Status:Needs review» Needs work

The last submitted patch, 89: d8_definition_metadata.patch, failed testing.

The last submitted patch, 89: d8_definition_metadata.patch, failed testing.

yched’s picture

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

fago’s picture

That seems reasonable, but where to put the 'definition_class' of list classes then? E.g., when you want to create a list of field_item:text items, it needs to instantiate the field definition object. Right now this goes via createDefinition() on the list-class, but we do not have a separate annotation for it?

(I tried doing plugin definition for list classes, but that "list type" stuff turned out to be overally confusing, so it's just a class.)

fago’s picture

ok, thought about this a bit more and I think the solution with the annotation would work. I had some concerns that annotations might be problematic due to the lists and missing inheritance, but while going through our use-cases of it in core it appears to be no issue at all. So, it might be the case that some extending data-type has to take that existing annotations over then, but given that's hardly the case I don't see an issue with it.

So we could do:
- add DataDefinitionInterface::createFromDataType($data_type)
- add a definition_class key to the annotation, defaulting to "DataDefinition"
- add a list_definition_class key to the annotation, defaulting to "ListDataDefinition"

Then, I agree this approach is a good idea to do as it moves the createFromDataType($data_type) instantiantion logic at the class that's instantiated - that's where it belongs.

yched’s picture

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 ?

yched’s picture

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.

yched’s picture

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 ]

yched’s picture

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)

fago’s picture

StatusFileSize
new164.91 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8_definition_metadata_17.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

finally getting back to this one..

First off, re-rolled and fixed merge conflicts.

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new44.98 KB
new180.35 KB
FAILED: [[SimpleTest]]: [MySQL] 64,547 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Running out of time, so posting an interim update. I've not yet worked through all reviews. However this includes the changes discussed in #95.1 and #96, so should give some stuff for review.

#95.1: As discussed, agreed & implemented.
#96: Implemented.
#97.1 Implemented.
#97.2 Yes, but that's the way it currently works for generating constraints. Once we do #2116341: Apply defaults to definition objects this would be a good opportunity to clean it up by moving that logic into the definition objects.

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

I've been favoring instanceof as it allows you to clearly reference the class instead of putting it into a string. Afaik this is not possible without an instance, so others have to remain using is_subclass_of.

Status:Needs review» Needs work

The last submitted patch, 100: d8_definition-metadata.patch, failed testing.

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new185.81 KB
FAILED: [[SimpleTest]]: [MySQL] 63,138 pass(es), 177 fail(s), and 44 exception(s).
[ View ]
new19.5 KB

ok, worked through the rest of the reviews.

#97.3: True, renamed it.

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

hm, we cannot do so easily as it getting properties requires a field definition while passing a field definition to this method does not seem natural. Howsoever, the default was 'value' previously as well, thus I think we should do this in a separate issue and not mix it in here.

#97.4: Done.
#97.5: Done

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

I don't think so. I'd rather remove it - it seems to be not widely used (only 4 usages) and it's easily replaced by array_keys(getPropertyDefinitions())? Howsoever, this is a preexisting situation so it deserves its own issue as well?

#97.6: True, however it's that way to save another function call on something rather heavy used. Not sure whether this micro performance improvement is really worth it, but howsoever it's not something invented here either.

#97.7: Yep, I think it should go as well. I left it there as I feared removing it creates more conflicts, but it turns out it's not that widely used. Thus, done so.

#97.8: Agreed
#97.9: Its changed to go via the field definition already, and I've done so for CommentFormController in the previous patch/interdiff.
#97.10: They are idempotent, except for NULL. However I think NULL is important to differentiate between an emtpy array, as it has the special meaning of "unset". I.e., the bundle is not restricted. I agree though, this needs to be idempotent, so I added support for setting NULL as well.
#97.11: Good question, I guess it follows the old entity_load() FALSE example. However, this stems from getPropertyDefinition($name) - so imo we should change in its own issue as well.
#97.12: True, renamed the parameter $definition to $values - " If given, an array of initial values to set on the definition.".

#98.1 Not sure what you refer to here. However, Field + FieldInstance need to implement their own getItemDefinition() as they cannot inherit from ListDataDefinition. This can change once we move to the adapter approach here.
#98.2: ok, removed.
#98.3: ok, reordered - but did not change the preexisting ordering of getPropertyDefinitions(), getPropertyDefinition($name) everywhere.
#98.4: done.
#98.5: Generally yes, but as the part of this test is about testing the typed-data API of entities based on ComplexDataInterface I've been using that one here.
#98.6: true, improved it.
#98.7: fixed
#98.8: fixed
#98.9: Improved this comment as well.

fago’s picture

Issue summary:View changes

Updated the list of API changes.

Status:Needs review» Needs work

The last submitted patch, 102: d8_definition-metadata.patch, failed testing.

aheimlich’s picture

Issue summary:View changes
fago’s picture

Status:Needs work» Needs review
StatusFileSize
new1.9 KB
new186.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8_definition-metadata_3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Needs work

The last submitted patch, 106: d8_definition-metadata.patch, failed testing.

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new186.77 KB
PASSED: [[SimpleTest]]: [MySQL] 64,676 pass(es).
[ View ]

Merged and fixed conflicts (again).

jibran’s picture

Status:Needs review» Needs work

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.

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new186.85 KB
PASSED: [[SimpleTest]]: [MySQL] 64,690 pass(es).
[ View ]
new670 bytes

@#109: Thanks for the review. Here my reply and an updated patch.

#109.1: Yes, as said - it's not invented by this patch, so a separate issue.
#109.2: Fixed, phpstorm--.
#109.3: That works and is what we are doing currently afaik, e.g. see the existing FieldDefinition class. Or has it changed again?
#109.4,5: Again, this is what we usually do for those test methods. I agree that this is not compliant with out general documentation rules, but afaik it's best practices right now.

tim.plunkett’s picture

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

fago’s picture

StatusFileSize
new186.84 KB
PASSED: [[SimpleTest]]: [MySQL] 64,690 pass(es).
[ View ]
new2.6 KB

Thanks for clarifying tim.plunkett! I fixed the wrong usages.

fago’s picture

yched’s picture

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)

fago’s picture

StatusFileSize
new186.98 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new6.45 KB

...both TDM methods could route to the single [DefinitionClass]::createFromDataType().

Yep, it would be possible as it was previously the case. However, it was only possible by going with "overloaded" meaning of the $data_type parameter, thus it wasn't very clean. By splitting up list and data type creation already in the TDM it was simple and not far too seek to clean this up as well.
I don't think the way this turns out on ListDataDefinition is convoluted though, createFromDataType() and createFromItemType() are implemented separately, while create() points to the latter. Nothing special imho?
On the contrary, previously the mapping of create a single "list" to create "a list of any items" was necessary to be able to handle both cases with a single method. As this is cleaned up now by using separate methods the need for this special handling is gone :)

re #98.1: I see, improved that.

#114:2,3: fixed
#114,4: ok, done so.
#114,5: k, added that
#114.6: ops, no that wasn't intentional. Thanks!

Status:Needs review» Needs work

The last submitted patch, 115: d8_definition_metadata.patch, failed testing.

yched’s picture

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

fago’s picture

Status:Needs work» Needs review
Issue tags:+Avoid commit conflicts
StatusFileSize
new1.43 KB
new187.09 KB
PASSED: [[SimpleTest]]: [MySQL] 64,638 pass(es).
[ View ]

:D - this should be better. Fixed #114.1 also, I think the first() was never necessary as we had forward for getPropertyDefinitions() also. Agreed it's cleaner without the first() though, yep.

Re-adding avoid commit conflicts tag.

yched’s picture

Status:Needs review» Reviewed & tested by the community

Yay. Let's go then!

The last submitted patch, 99: d8_definition_metadata.patch, failed testing.

yched’s picture

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

yched’s picture

StatusFileSize
new187.43 KB
PASSED: [[SimpleTest]]: [MySQL] 64,639 pass(es).
[ View ]
new5.42 KB

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.

webchick’s picture

Status:Reviewed & tested by the community» Needs work

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. :\

webchick’s picture

Issue tags:+Needs change record

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

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new186.54 KB
PASSED: [[SimpleTest]]: [MySQL] 64,680 pass(es).
[ View ]
new244.16 KB
PASSED: [[SimpleTest]]: [MySQL] 64,696 pass(es).
[ View ]
new186.85 KB

@yched: Looks like I forgot to push the interdiff from #112, thanks - added yours in.

@webchick: Looks like you forgot push the other one, but yeah let's commit the other one first.

Here is a reroll of #122, plus a reroll based on #2200821: Rename Fieldinterface and FieldInstanceInterface for later and a combined version incorporating #2200821: Rename Fieldinterface and FieldInstanceInterface (for the bot).

fago’s picture

I've added draft change records for the 8.x-8.x changes:
http://drupal.org/node/2203309
http://drupal.org/node/2203297
http://drupal.org/node/2203303

Apart from that we'll have to update existing change records and docs:
http://drupal.org/node/2064123
http://drupal.org/node/1806650
http://drupal.org/node/2078241
http://drupal.org/node/1794140
http://drupal.org/node/1795854
I'll go over those once this has been committed.

fago’s picture

StatusFileSize
new186.85 KB
PASSED: [[SimpleTest]]: [MySQL] 64,642 pass(es).
[ View ]

#2200821: Rename Fieldinterface and FieldInstanceInterface got pushed, so re-uploading the re-rolled patch.

fago’s picture

Status:Needs review» Reviewed & tested by the community

re-RTBCing myself as this this is only #122 re-rolled and conflicts fixed. (There has been only 1 conflict in the use statements of FieldInstanceConfig).

alexpott’s picture

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;

alexpott’s picture

Status:Reviewed & tested by the community» Fixed
Berdir’s picture

sun’s picture

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

#2205367: [HEAD BROKEN] PHP 5.4

xjm’s picture

Issue tags:-Needs change record

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

Status:Fixed» Closed (fixed)

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