added accessor for default primary key

[kyle-mccarthy]

Q	A
Is bugfix?	yes
New feature?	no
Breaks BC?	no
Tests pass?	yes

If a model extending the ActiveRecord class does not implement the primaryKey function, the primary key will default to '_id'. However, this property is inaccessible as it is private, resulting in an UnknownPropertyException. The accessibility of the property would need to be changed to protected, or an accessor method would need to be implemented (which I chose).

To reproduce the bug that this PR fixes:

1. Create a new model and extend the ActiveRecord
2. Use the default primary key (_id)/don't override the primaryKey method
3. Attempt to use ActiveDataProvider

[beowulfenator]

Can you please be more specific as to what problem this PR solves?

Right now the model's primary key can be by default accessed as $model->primaryKey as defined by the ActiveRecord::getPrimaryKey() method.

If you're using ActiveDataProvider, it's quite capable of getting the necessary attribute by default (look here).

Why is it necessary to override the primaryKey method?

[kyle-mccarthy]

Sorry let me expand on the issue. The line that you referenced will allow you to get the name of the key but you can't actually get the value since it is set to private. This will cause an UnknownPropertyException.

The line of code that you referenced will correctly get the key's id as '_id'; however, in the following lines of code the exception is thrown when your try to access $model['_id'].

_id will first try to be resolved by the __get method in the BaseActiveRecord, when this happens it will call __get in the parent here. Once this is called the call will travel to the __get in the Component.php . Since the method does get exist in the class and it is not a behavior and exception will be thrown as getting an unknown property.

---

Here is the stack trace to better explain the problem.

yii\base\UnknownPropertyException: Getting unknown property: app\models\elastic\PlaceSearch::_id in /.../vendor/yiisoft/yii2/base/Component.php:143
Stack trace:
#0 /.../vendor/yiisoft/yii2/db/BaseActiveRecord.php(286): yii\base\Component->__get('_id')
#1 /.../vendor/yiisoft/yii2/base/Model.php(984): yii\db\BaseActiveRecord->__get('_id')
#2 /.../vendor/yiisoft/yii2/data/ActiveDataProvider.php(138): yii\base\Model->offsetGet('_id')
#3 /.../vendor/yiisoft/yii2/data/BaseDataProvider.php(82): yii\data\ActiveDataProvider->prepareKeys(Array)
#4 /.../vendor/yiisoft/yii2/data/BaseDataProvider.php(92): yii\data\BaseDataProvider->prepare()
#5 /.../controllers/SearchController.php(15): yii\data\BaseDataProvider->getModels()
#6 [internal function]: app\controllers\SearchController->actionIndex()
#7 /.../vendor/yiisoft/yii2/base/InlineAction.php(55): call_user_func_array(Array, Array)
#8 /.../vendor/yiisoft/yii2/base/Controller.php(154): yii\base\InlineAction->runWithParams(Array)
#9 /.../vendor/yiisoft/yii2/base/Module.php(454): yii\base\Controller->runAction('index', Array)
#10 /.../vendor/yiisoft/yii2/web/Application.php(100): yii\base\Module->runAction('search/index', Array)
#11 /.../vendor/yiisoft/yii2/base/Application.php(375): yii\web\Application->handleRequest(Object(yii\web\Request))
#12 /.../web/index.php(8): yii\base\Application->run()
#13 {main}

[beowulfenator]

Why are you trying to access the _id attribute? Using $model->primaryKey will invoke this public method and return the value of the primary key, whatever it is.

[kyle-mccarthy]

I'm not personally accessing/calling the _id attribute. I am constructing a new ActiveDataProvider and then calling the getModels method. The getModels then calls the prepare method, which in turn calls the prepareKeys method. Then the exception will be thrown up to the prepareKeys method when it tries to access _id here.

[beowulfenator]

Ok, I got it. Rather than updating ActiveRecord to return that attribute (which may or may not be present), we should update the yii\elasticsearch\ActiveDataProvider by overriding the prepareKeys method. Can you do that?

[kyle-mccarthy]

@beowulfenator Okay good idea, I just updated my PR!

[beowulfenator]

Why is it failing the tests?

[kyle-mccarthy]

I am trying to debug this locally; however, the test cases are failing for me on my branch and then also on the master branch in the same two places. Since it is happening on master also, I'm inclined to believe that it's unrelated to my changes.

This is from the master branch...

PHPUnit 5.5.4 by Sebastian Bergmann and contributors.

..............E................................................F. 65 / 85 ( 76%)
....................                                              85 / 85 (100%)

Time: 10.13 seconds, Memory: 26.00MB

There was 1 error:

1) yiiunit\extensions\elasticsearch\ActiveRecordTest::testBooleanAttribute
yii\elasticsearch\Exception: Elasticsearch request failed with code 400.

/.../yii2-elasticsearch/Connection.php:543
/.../yii2-elasticsearch/Connection.php:351
/.../yii2-elasticsearch/Command.php:465
/.../yii2-elasticsearch/tests/data/ar/Customer.php:84
/.../yii2-elasticsearch/tests/ActiveRecordTest.php:420
/usr/local/Cellar/phpunit/5.5.4/libexec/phpunit-5.5.4.phar:569

--

There was 1 failure:

1) yiiunit\extensions\elasticsearch\ActiveRecordTest::testAttributeAccess
Failed asserting that false is true.

/.../yii2-elasticsearch/tests/ActiveRecordTestTrait.php:1220
/usr/local/Cellar/phpunit/5.5.4/libexec/phpunit-5.5.4.phar:569

ERRORS!
Tests: 85, Assertions: 697, Errors: 1, Failures: 1.

[liyadongcn]

public static function primaryKey()
{
return ['id'];
}

Put this function in your data model. When you set the id attribute value, it will map to the _id field of the elasticsearch

[kyle-mccarthy]

@liyadongcn That does not map to the the _id field on each document in elastic. That will only work if you actually define an 'id' in the schema when creating templates for elastic and add it to the attributes.

@beowulfenator I've looked at the issue further and if you add '_id' to the attributes on a model extending yii\elasticsearch\ActiveRecord, it will work. However, I feel that this should still work out of the box with the changes that we talked about. If you think that the solution should be to add '_id' to the list of attributes, I feel as though this should be added to the documentation.

So here are fixes that makes it work:

1. Overload prepareKeys in the ActiveDataProvider to use the primaryKey accessor to get the PK on the model.
2. Add '_id' to the list of attributes on a model.
3. Choose a different attribute to use as the primary key, rather than elastics default '_id'.

[beowulfenator]

@kyle-mccarthy For some strange reason the docs say:

The primaryKey for elasticsearch documents is the _id field by default. This field is not part of the ActiveRecord attributes so you should never add _id to the list of [[attributes()|attributes]].

Let me figure out why the tests are failing and just pull in this PR.

[beowulfenator]

Ok, so the tests are failing because tests for canSetProperty and canGetProperty are up to date, but code that fixes bugs in BaseActiveRecord for those tests will be released with version 2.0.10 of the framework. I'm merging the PR.

[beowulfenator]

Thanks for your update, @kyle-mccarthy!

[kyle-mccarthy]

@beowulfenator Thank you! Also after looking at open issues I believe this resolves #72