Merge PR #4038 branch 'consistent-after-save'

extensions/elasticsearch/ActiveRecord.php

@@ -395,35 +395,31 @@ class ActiveRecord extends BaseActiveRecord
         if ($runValidation && !$this->validate($attributes)) {
             return false;
         }
-        if ($this->beforeSave(true)) {
-            $values = $this->getDirtyAttributes($attributes);
-
-            $response = static::getDb()->createCommand()->insert(
-                static::index(),
-                static::type(),
-                $values,
-                $this->getPrimaryKey(),
-                $options
-            );
-
-//            if (!isset($response['ok'])) {
-//                return false;
-//            }
-            $pk = static::primaryKey()[0];
-            $this->$pk = $response['_id'];
-            if ($pk != '_id') {
-                $values[$pk] = $response['_id'];
-            }
-            $this->_version = $response['_version'];
-            $this->_score = null;
+        if (!$this->beforeSave(true)) {
+            return false;
+        }
+        $values = $this->getDirtyAttributes($attributes);
 
-            $this->afterSave(true);
-            $this->setOldAttributes($values);
+        $response = static::getDb()->createCommand()->insert(
+            static::index(),
+            static::type(),
+            $values,
+            $this->getPrimaryKey(),
+            $options
+        );
 
-            return true;
+        $pk = static::primaryKey()[0];
+        $this->$pk = $response['_id'];
+        if ($pk != '_id') {
+            $values[$pk] = $response['_id'];
         }
+        $this->_version = $response['_version'];
+        $this->_score = null;
+
+        $this->setOldAttributes($values);
+        $this->afterSave(true, $values);
 
-        return false;
+        return true;
     }
 
     /**

extensions/mongodb/ActiveRecord.php

@@ -224,8 +224,8 @@ abstract class ActiveRecord extends BaseActiveRecord
         $this->setAttribute('_id', $newId);
         $values['_id'] = $newId;
 
-        $this->afterSave(true);
         $this->setOldAttributes($values);
+        $this->afterSave(true, $values);
 
         return true;
     }
@@ -241,7 +241,7 @@ abstract class ActiveRecord extends BaseActiveRecord
         }
         $values = $this->getDirtyAttributes($attributes);
         if (empty($values)) {
-            $this->afterSave(false);
+            $this->afterSave(false, $values);
             return 0;
         }
         $condition = $this->getOldPrimaryKey(true);
@@ -260,10 +260,10 @@ abstract class ActiveRecord extends BaseActiveRecord
             throw new StaleObjectException('The object being updated is outdated.');
         }
 
-        $this->afterSave(false);
         foreach ($values as $name => $value) {
             $this->setOldAttribute($name, $this->getAttribute($name));
         }
+        $this->afterSave(false, $values);
 
         return $rows;
     }

extensions/mongodb/file/ActiveRecord.php

@@ -127,8 +127,8 @@ abstract class ActiveRecord extends \yii\mongodb\ActiveRecord
         $this->setAttribute('_id', $newId);
         $values['_id'] = $newId;
 
-        $this->afterSave(true);
         $this->setOldAttributes($values);
+        $this->afterSave(true, $values);
 
         return true;
     }
@@ -144,7 +144,7 @@ abstract class ActiveRecord extends \yii\mongodb\ActiveRecord
         }
         $values = $this->getDirtyAttributes($attributes);
         if (empty($values)) {
-            $this->afterSave(false);
+            $this->afterSave(false, $values);
             return 0;
         }
 
@@ -196,10 +196,10 @@ abstract class ActiveRecord extends \yii\mongodb\ActiveRecord
             }
         }
 
-        $this->afterSave(false);
         foreach ($values as $name => $value) {
             $this->setOldAttribute($name, $this->getAttribute($name));
         }
+        $this->afterSave(false, $values);
 
         return $rows;
     }

extensions/redis/ActiveRecord.php

@@ -99,38 +99,35 @@ class ActiveRecord extends BaseActiveRecord
         if ($runValidation && !$this->validate($attributes)) {
             return false;
         }
-        if ($this->beforeSave(true)) {
-            $db = static::getDb();
-            $values = $this->getDirtyAttributes($attributes);
-            $pk = [];
-            //			if ($values === []) {
-            foreach ($this->primaryKey() as $key) {
-                $pk[$key] = $values[$key] = $this->getAttribute($key);
-                if ($pk[$key] === null) {
-                    $pk[$key] = $values[$key] = $db->executeCommand('INCR', [static::keyPrefix() . ':s:' . $key]);
-                    $this->setAttribute($key, $values[$key]);
-                }
-            }
-            //			}
-            // save pk in a findall pool
-            $db->executeCommand('RPUSH', [static::keyPrefix(), static::buildKey($pk)]);
-
-            $key = static::keyPrefix() . ':a:' . static::buildKey($pk);
-            // save attributes
-            $args = [$key];
-            foreach ($values as $attribute => $value) {
-                $args[] = $attribute;
-                $args[] = $value;
+        if (!$this->beforeSave(true)) {
+            return false;
+        }
+        $db = static::getDb();
+        $values = $this->getDirtyAttributes($attributes);
+        $pk = [];
+        foreach ($this->primaryKey() as $key) {
+            $pk[$key] = $values[$key] = $this->getAttribute($key);
+            if ($pk[$key] === null) {
+                $pk[$key] = $values[$key] = $db->executeCommand('INCR', [static::keyPrefix() . ':s:' . $key]);
+                $this->setAttribute($key, $values[$key]);
             }
-            $db->executeCommand('HMSET', $args);
-
-            $this->afterSave(true);
-            $this->setOldAttributes($values);
+        }
+        // save pk in a findall pool
+        $db->executeCommand('RPUSH', [static::keyPrefix(), static::buildKey($pk)]);
 
-            return true;
+        $key = static::keyPrefix() . ':a:' . static::buildKey($pk);
+        // save attributes
+        $args = [$key];
+        foreach ($values as $attribute => $value) {
+            $args[] = $attribute;
+            $args[] = $value;
         }
+        $db->executeCommand('HMSET', $args);
+
+        $this->setOldAttributes($values);
+        $this->afterSave(true, $values);
 
-        return false;
+        return true;
     }
 
     /**

extensions/sphinx/ActiveRecord.php

@@ -395,8 +395,8 @@ abstract class ActiveRecord extends BaseActiveRecord
             return false;
         }
 
-        $this->afterSave(true);
         $this->setOldAttributes($values);
+        $this->afterSave(true, $values);
 
         return true;
     }
@@ -488,7 +488,7 @@ abstract class ActiveRecord extends BaseActiveRecord
         }
         $values = $this->getDirtyAttributes($attributes);
         if (empty($values)) {
-            $this->afterSave(false);
+            $this->afterSave(false, $values);
             return 0;
         }
 
@@ -530,10 +530,10 @@ abstract class ActiveRecord extends BaseActiveRecord
             }
         }
 
-        $this->afterSave(false);
         foreach ($values as $name => $value) {
             $this->setOldAttribute($name, $this->getAttribute($name));
         }
+        $this->afterSave(false, $values);
 
         return $rows;
     }

framework/CHANGELOG.md

@@ -22,6 +22,7 @@ Yii Framework 2 Change Log
 - Bug #3194: Date formatter works only for timestamps in the year range 1970 to 2038 (kartik-v)
 - Bug #3204: `yii\di\Container` did not handle the `$config` parameter well in case when it does not have a default value (qiangxue)
 - Bug #3216: Fixed the bug that `yii.activeForm.destroy()` did not remove `submit` event handlers (qiangxue)
+- Bug #3233: Ensure consistent behavior in ActiveRecord::afterSave() (cebe, qiangxue)
 - Bug #3236: Return value for DateTime->format('U') casted to double to allow correct date formatting (pgaultier)
 - Bug #3268: Fixed the bug that the schema name in a table name was not respected by `yii\db\mysql\Schema` (terazoid, qiangxue)
 - Bug #3311: Fixed the bug that `yii\di\Container::has()` did not return correct value (mgrechanik, qiangxue)

framework/UPGRADE.md

@@ -57,3 +57,12 @@ Upgrade from Yii 2.0 Beta
 * If your model code is using the `file` validation rule, you should rename its `types` option to `extensions`.
 
 * `MailEvent` class has been moved to the `yii\mail` namespace. You have to adjust all references that may exist in your code.
+
+* The behavior and signature of `ActiveRecord::afterSave()` has changed. `ActiveRecord::$isNewRecord` will now always be
+  false in afterSave and also dirty attributes are not available. This change has been made to have a more consistent and
+  expected behavior. The changed attributes are now available in the new parameter of afterSave() `$changedAttributes`.
+
+* `ActiveRecord::updateAttributes()` has been changed to not trigger events and not respect optimistic locking anymore to
+  differentiate it more from calling `update(false)` and to ensure it can be used in `afterSave()` without triggering infinite
+  loops.
+

framework/base/ModelEvent.php

@@ -8,9 +8,7 @@
 namespace yii\base;
 
 /**
- * ModelEvent class.
- *
- * ModelEvent represents the parameter needed by model events.
+ * ModelEvent represents the parameter needed by [[Model]] events.
  *
  * @author Qiang Xue <qiang.xue@gmail.com>
  * @since 2.0

framework/classes.php

@@ -89,6 +89,7 @@ return [
   'yii\db\ActiveRecord' => YII_PATH . '/db/ActiveRecord.php',
   'yii\db\ActiveRecordInterface' => YII_PATH . '/db/ActiveRecordInterface.php',
   'yii\db\ActiveRelationTrait' => YII_PATH . '/db/ActiveRelationTrait.php',
+  'yii\db\AfterSaveEvent' => YII_PATH . '/db/AfterSaveEvent.php',
   'yii\db\BaseActiveRecord' => YII_PATH . '/db/BaseActiveRecord.php',
   'yii\db\BatchQueryResult' => YII_PATH . '/db/BatchQueryResult.php',
   'yii\db\ColumnSchema' => YII_PATH . '/db/ColumnSchema.php',

framework/db/ActiveRecord.php

@@ -431,8 +431,8 @@ class ActiveRecord extends BaseActiveRecord
             }
         }
 
-        $this->afterSave(true);
         $this->setOldAttributes($values);
+        $this->afterSave(true, $values);
 
         return true;
     }

framework/db/AfterSaveEvent.php

+<?php
+/**
+ * @link http://www.yiiframework.com/
+ * @copyright Copyright (c) 2008 Yii Software LLC
+ * @license http://www.yiiframework.com/license/
+ */
+
+namespace yii\db;
+
+use yii\base\Event;
+
+/**
+ * AfterSaveEvent represents the information available in [[ActiveRecord::EVENT_AFTER_INSERT]] and [[ActiveRecord::EVENT_AFTER_UPDATE]].
+ *
+ * @author Carsten Brandt <mail@cebe.cc>
+ * @since 2.0
+ */
+class AfterSaveEvent extends Event
+{
+    /**
+     * @var array The attribute values that had changed and were saved.
+     */
+    public $changedAttributes;
+}

framework/db/BaseActiveRecord.php

@@ -644,16 +644,16 @@ abstract class BaseActiveRecord extends Model implements ActiveRecordInterface
      * Updates the specified attributes.
      *
      * This method is a shortcut to [[update()]] when data validation is not needed
-     * and only a list of attributes need to be updated.
+     * and only a small set attributes need to be updated.
      *
      * You may specify the attributes to be updated as name list or name-value pairs.
      * If the latter, the corresponding attribute values will be modified accordingly.
      * The method will then save the specified attributes into database.
      *
-     * Note that this method will NOT perform data validation.
+     * Note that this method will **not** perform data validation and will **not** trigger events.
      *
      * @param array $attributes the attributes (names or name-value pairs) to be updated
-     * @return integer|boolean the number of rows affected, or false if [[beforeSave()]] stops the updating process.
+     * @return integer the number of rows affected.
      */
     public function updateAttributes($attributes)
     {
@@ -666,7 +666,19 @@ abstract class BaseActiveRecord extends Model implements ActiveRecordInterface
                 $attrs[] = $name;
             }
         }
-        return $this->updateInternal($attrs);
+
+        $values = $this->getDirtyAttributes($attrs);
+        if (empty($values)) {
+            return 0;
+        }
+
+        $rows = $this->updateAll($values, $this->getOldPrimaryKey(true));
+
+        foreach ($values as $name => $value) {
+            $this->_oldAttributes[$name] = $this->_attributes[$name];
+        }
+
+        return $rows;
     }
 
     /**
@@ -680,7 +692,7 @@ abstract class BaseActiveRecord extends Model implements ActiveRecordInterface
         }
         $values = $this->getDirtyAttributes($attributes);
         if (empty($values)) {
-            $this->afterSave(false);
+            $this->afterSave(false, $values);
             return 0;
         }
         $condition = $this->getOldPrimaryKey(true);
@@ -699,10 +711,10 @@ abstract class BaseActiveRecord extends Model implements ActiveRecordInterface
             throw new StaleObjectException('The object being updated is outdated.');
         }
 
-        $this->afterSave(false);
         foreach ($values as $name => $value) {
             $this->_oldAttributes[$name] = $this->_attributes[$name];
         }
+        $this->afterSave(false, $values);
 
         return $rows;
     }
@@ -855,15 +867,18 @@ abstract class BaseActiveRecord extends Model implements ActiveRecordInterface
     /**
      * This method is called at the end of inserting or updating a record.
      * The default implementation will trigger an [[EVENT_AFTER_INSERT]] event when `$insert` is true,
-     * or an [[EVENT_AFTER_UPDATE]] event if `$insert` is false.
+     * or an [[EVENT_AFTER_UPDATE]] event if `$insert` is false. The event class used is [[AfterSaveEvent]].
      * When overriding this method, make sure you call the parent implementation so that
      * the event is triggered.
      * @param boolean $insert whether this method called while inserting a record.
      * If false, it means the method is called while updating a record.
+     * @param array $changedAttributes The attribute values that had changed and were saved.
      */
-    public function afterSave($insert)
+    public function afterSave($insert, $changedAttributes)
     {
-        $this->trigger($insert ? self::EVENT_AFTER_INSERT : self::EVENT_AFTER_UPDATE);
+        $this->trigger($insert ? self::EVENT_AFTER_INSERT : self::EVENT_AFTER_UPDATE, new AfterSaveEvent([
+            'changedAttributes' => $changedAttributes
+        ]));
     }
 
     /**

tests/unit/data/ar/Customer.php

@@ -58,11 +58,11 @@ class Customer extends ActiveRecord
         })->orderBy('id');
     }
 
-    public function afterSave($insert)
+    public function afterSave($insert, $changedAttributes)
     {
         ActiveRecordTest::$afterSaveInsert = $insert;
         ActiveRecordTest::$afterSaveNewRecord = $this->isNewRecord;
-        parent::afterSave($insert);
+        parent::afterSave($insert, $changedAttributes);
     }
 
     /**

tests/unit/data/ar/elasticsearch/Customer.php

@@ -40,11 +40,11 @@ class Customer extends ActiveRecord
         return $this->hasMany(OrderWithNullFK::className(), ['customer_id' => 'id'])->orderBy('created_at');
     }
 
-    public function afterSave($insert)
+    public function afterSave($insert, $changedAttributes)
     {
         ActiveRecordTest::$afterSaveInsert = $insert;
         ActiveRecordTest::$afterSaveNewRecord = $this->isNewRecord;
-        parent::afterSave($insert);
+        parent::afterSave($insert, $changedAttributes);
     }
 
     /**

tests/unit/data/ar/redis/Customer.php

@@ -38,11 +38,11 @@ class Customer extends ActiveRecord
     /**
      * @inheritdoc
      */
-    public function afterSave($insert)
+    public function afterSave($insert, $changedAttributes)
     {
         ActiveRecordTest::$afterSaveInsert = $insert;
         ActiveRecordTest::$afterSaveNewRecord = $this->isNewRecord;
-        parent::afterSave($insert);
+        parent::afterSave($insert, $changedAttributes);
     }
 
     /**

tests/unit/framework/ar/ActiveRecordTestTrait.php

@@ -816,7 +816,7 @@ trait ActiveRecordTestTrait
         $this->afterSave();
 
         $this->assertNotNull($customer->id);
-        $this->assertTrue(static::$afterSaveNewRecord);
+        $this->assertFalse(static::$afterSaveNewRecord);
         $this->assertTrue(static::$afterSaveInsert);
         $this->assertFalse($customer->isNewRecord);
     }
@@ -867,7 +867,7 @@ trait ActiveRecordTestTrait
         /* @var $customerClass \yii\db\ActiveRecordInterface */
         $customerClass = $this->getCustomerClass();
         /* @var $this TestCase|ActiveRecordTestTrait */
-        // save
+        /* @var $customer Customer */
         $customer = $customerClass::findOne(2);
         $this->assertTrue($customer instanceof $customerClass);
         $this->assertEquals('user2', $customer->name);
@@ -879,8 +879,8 @@ trait ActiveRecordTestTrait
         $this->afterSave();
         $this->assertEquals('user2x', $customer->name);
         $this->assertFalse($customer->isNewRecord);
-        $this->assertFalse(static::$afterSaveNewRecord);
-        $this->assertFalse(static::$afterSaveInsert);
+        $this->assertNull(static::$afterSaveNewRecord);
+        $this->assertNull(static::$afterSaveInsert);
         $customer2 = $customerClass::findOne(2);
         $this->assertEquals('user2x', $customer2->name);