Typed data does not handle set() and onChange() consistently

A bit above my head ;-), but looks like it makes sense.

Just wondering: some param / var names are $notify, others $notify_parent - and the distinction is really welcome to help make sense of the code flow :-)
I does seem like some (most ?) $notify vars are indeed about "notify self" rather that "notify parent", but I'm not sure that's the case for all.
Mapping & FieldItemBase::set($notify) seems to be about notifying the parent ?

So I'm wondering if the other $notify vars would need double checking too ?

Other than that - is "@return self" a thing now ?

TextItemBase.php:
public function onChange($property_name, $notify_parent = TRUE) {
  // ...
  parent::onChange($property_name, $notify_parent);
}

This gets a bit hair pulling - so the $notify_parent param is about notifying the parent's parent ?

Looks good - minor remarks :

1. +++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
   @@ -190,23 +191,26 @@ public function __isset($name) {
   +    // Explicitly unset the property in $this->values if a non-defined
   +    // property is unset, such that its key is removed from $this->values.
   +    if (!$this->definition->getPropertyDefinition($name)) {
   +      unset($this->values[$name]);
   +    }
   +    else {
   +      $this->set($name, NULL);
   +    }
   

   I think in other similar places we tend to order the cases the other way :

   if (official field/property) {
     do this
   }
   else {
     do that
   }
   

2. +++ b/core/modules/system/src/Tests/Entity/EntityFieldTest.php
   @@ -148,6 +148,34 @@ protected function doTestReadWrite($entity_type) {
   +    $entity->user_id->first()->get('entity')->setValue($new_user2);
   ...
   +    $entity->user_id->first()->get('target_id')->setValue($new_user2->id());
   ...
   +    $entity->name->first()->get('value')->setValue(NULL);
   +    $entity->user_id->first()->get('target_id')->setValue(NULL);
   

   Right, first() auto-creating the item is something I'd really want to get rid of in #2164601: Stop auto-creating FieldItems on mere reading of $entity->field[N], but this is not a blocker here :-)

3. +++ b/core/modules/system/src/Tests/Entity/EntityFieldTest.php
   @@ -148,6 +148,34 @@ protected function doTestReadWrite($entity_type) {
   +    $entity2 = entity_create($entity_type, array(
   +      'user_id' => array('target_id' => $new_user2->id()),
   +    ));
   +    // Access the property object, and set a value.
   +    $entity2->user_id->first()->get('target_id')->setValue($new_user2->id());
   

   Not sure I get this - this assigns a target_id in cerate(), and the assigns the same target_id with setValue() ? why ?

:-)

Partial review :

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -375,9 +375,9 @@ protected function getTranslatedField($name, $langcode) {
      public function set($name, $value, $notify = TRUE) {
   -    // If default language or an entity key changes we need to react to that.
   -    $notify = $name == 'langcode' || in_array($name, $this->getEntityType()->getKeys());
   -    $this->get($name)->setValue($value, $notify);
   +    $this->get($name)->setValue($value, FALSE);
   +    // Directly notify ourselves, so we can take changes into account.
   +    $this->onChange($name);
   

   We do nothing with the $notify param, and never notify our parents ?
   Shouldn't this be $this->onChange($name, $notify) ?

   + For clarity, I'd suggest the following commenting :

   // Assign the value on the child, making sure this does not notify ourselves or our parents yet.
   $this->get($name)->setValue($value, FALSE);
   // Then notify ourselves, bubbling up to our parents if specified by the $notify parameter.
   $this->onChange($name, $notify);
   

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -140,14 +140,10 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
   +    // Treat the values as property value of the entity property, if no array
   +    // is given as this handles entity ids and objects.
        if (isset($values) && !is_array($values)) {
   ...
   +      $this->set('entity', $values, $notify);
        }
   

   Comment is a bit unclear - suggestion : "If either a scalar or an object was passed as the value for the item, assign it to the 'entity' property since that works for both cases " ?

3. Maybe you have already explained this to me live, but :
   $map->set($property, $value) notifies $map->onChange() for the property that was changed,
   but yet $map->setValue($values_for_properties) doesn't notify $map->onChange() for the various properties that were changed ?

   What's the reasoning ? setValue() operates on the scope of all properties that are being written, and can thus do whatever reaction is needed without needing separate onChange() reactions on each individual property ?

+++ b/core/modules/text/src/Plugin/Field/FieldType/TextItemBase.php
@@ -59,12 +59,7 @@ public function isEmpty() {
+  public function onChange($property_name, $notify = TRUE) {

That onChange() implementation calls $this->set($property, $value, FALSE) - shouldn't it call $this->writePropertyValue() instead ?

ContentEntityBase - "there is no parent to notify anyway" : doh, of course. Self-slap time.

1. +++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
   @@ -139,32 +123,32 @@ public function setValue($values, $notify = TRUE) {
   +   * Writes the value of a property without handling changes.
   +   *
   +   * @param string $property_name
   +   *   The name of the property to be written.
   +   * @param $value
   +   *   The value to set.
       */
   -  public function set($property_name, $value, $notify = TRUE) {
   +  protected function writePropertyValue($property_name, $value) {
   

   The phpdoc is already in Map, it should be @inheritdoc here.

   What we should document here, on the other hand, is how FieldItemBase's logic for $this->values & $this->properties is different from the one in Map - which is the reason why we override writePropertyValue().

   Also, minor : code order is weird - could we put this override below the override of setValue(), instead of between __get() and __set() ?

2. +++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/Map.php
   @@ -134,19 +134,31 @@ public function get($property_name) {
   +   * Writes the value of a property without handling changes.
   +   *
   +   * @param string $property_name
   +   *   The name of the property to be written.
   +   * @param $value
   +   *   The value to set.
   +   */
   +  protected function writePropertyValue($property_name, $value) {
   

   And here we could document that this is the method that should be used by onChange() implementations instead of set(), because set() calls onChange().

3. +++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/Map.php
   @@ -212,11 +224,16 @@ public function __clone() {
   +   *   (optional) Whether to forward the notification to the parent. Defaults to
   +   *   TRUE. By passing FALSE overriding implementations can re-use the logic
   +   *   of parent classes without triggering the parent notification.
   

   "By passing FALSE overriding implementations can re-use the logic of parent classes without triggering the parent notification."
   A bit cryptic :-)
   - would be easier to parse with a comma after "By pasing FALSE, ..."
   - "overriding implementations can ..." : implementations of what ?

4. +++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/Map.php
   @@ -212,11 +224,16 @@ public function __clone() {
   -    if (isset($this->parent)) {
   +    if (isset($this->parent) && $notify) {
   

   Nitpick : logically, would make a bit more sense as
   if ($notify && isset($this->parent)) ?

   "If we're asked to notify and there is actually something to notify..."

Thanks @fago.

+++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/Map.php
@@ -228,12 +231,12 @@ public function __clone() {
+   *   TRUE. By passing FALSE, overrides of this method can re-use the logic
+   *   of parent classes without triggering notification.

Sorry for being thick, I can't honestly say I really understand it better.

Not a blocker for RTBC, but maybe we can discuss live see if we can clarify it a bit more ? :-)

This is blocking a critical issue.

Given

However, the previous return value was not implemented at all, so no can be making use of that API right now.

(from issue summary) I do not think we need a CR here. This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 9f04e89 and pushed to 8.0.x. Thanks!