Feature/nex 136/configure spanner #636
Conversation
…b.com/oat-sa/generis into feature/autoincrement-less-statements
…ith the same method than the other ones.
…nto feature/NEX-136/configure-spanner # Conflicts: # manifest.php # scripts/update/Updater.php
…/NEX-136/configure-spanner
…ments' into feature/NEX-136/configure-spanner
…/generis into feature/NEX-136/configure-spanner
|
Code Climate has analyzed commit ee36b3b and detected 3 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
Please do not consider this comment for now. |
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| /** | ||
| * @inheritdoc | ||
| */ | ||
| public function prepareStatement($modelId, $subject, $predicate, $object, $lang, $author) |
There was a problem hiding this comment.
Method prepareStatement has 6 arguments (exceeds 4 allowed). Consider refactoring.
| * @param string $author | ||
| * @return array | ||
| */ | ||
| abstract public function prepareStatement($modelId, $subject, $predicate, $object, $lang, $author); |
There was a problem hiding this comment.
Method prepareStatement has 6 arguments (exceeds 4 allowed). Consider refactoring.
julien-sebire
changed the base branch from
develop
to
feature/NEX-336/newsql-schema
| 'predicate' => $predicate, | ||
| 'object' => $object, | ||
| 'l_language' => $lang, | ||
| 'author' => is_null($author) ? '' : $author, |
There was a problem hiding this comment.
if possible please use $author ??
There was a problem hiding this comment.
PHP 7.1 rulez! (at last)... ;-)
| { | ||
| $models = array_map( | ||
| function ($a) { | ||
| return "'" . $a . "'"; |
There was a problem hiding this comment.
should we use cast (string) instead of the concatenation
There was a problem hiding this comment.
Where adding the quotes, in fact. $a is supposed to be a string already.
| public function createModelsTable(Schema $schema) | ||
| { | ||
| $table = $schema->createTable('models'); | ||
| $table->addColumn('modelid', 'string', ['length' => 23, 'notnull' => true]); |
There was a problem hiding this comment.
please don't forget about the length of 36 for the uuid4
There was a problem hiding this comment.
Yes, right! Thanks for reminder.
| public function createStatementsTable(Schema $schema) | ||
| { | ||
| $table = $schema->createTable('statements'); | ||
| $table->addColumn('id', 'string', ['length' => 23, 'notnull' => true]); |
There was a problem hiding this comment.
same here 36 chr
core/kernel/api/RdsModelFactory.php
Outdated
| { | ||
| $this->dbWrapper->insert('models', ['modeluri' => $namespace]); | ||
| $result = $this->dbWrapper->query('select modelid from models where modeluri = ?', [$namespace]); | ||
| return $result->fetch()['modelid']; |
There was a problem hiding this comment.
if the fetch will not return an array that will contain the modelid index this will create a notice
There was a problem hiding this comment.
Correct. Fixed by adding a test for the previous insert, also in NewSqlRdsModelFactory.
| //TODO bad way, need to find better | ||
| $modelId = $this->getModelId($namespace); | ||
| if ($modelId === false) { | ||
| common_Logger::d('modelId not found, need to add namespace ' . $namespace); |
There was a problem hiding this comment.
can we use the LoggerAwareTrait and the function from inside that instead of this singleton old logger
| ) | ||
| ); | ||
|
|
||
| if (intval($result->fetchColumn()) > 0) { |
There was a problem hiding this comment.
casting to int will optimize your condition
| public function getIteratorQuery($modelIds) | ||
| { | ||
| return 'SELECT * FROM statements ' | ||
| . (is_null($modelIds) ? '' : 'WHERE ' . $this->buildModelSqlCondition($modelIds) . ' ') |
There was a problem hiding this comment.
i don't think you need the is_null, also i think a sprintf will be more readable
| private static $instance = null; | ||
| public static function singleton() | ||
| { | ||
| $returnValue = null; |
There was a problem hiding this comment.
normally
if (self::$instance == null)
{
self::$instance = new Singleton();
}
return self::$instance;
but your implementation is different, can you please comment on the value of this form
There was a problem hiding this comment.
This is not my implementation. I just moved it from the bottom of the same file. But I'll remove this change, I don't want to be anywhere in seeming like writing a singleton method! :D
helpers/UuidPrimaryKeyTrait.php
Outdated
| */ | ||
| public function getUniquePrimaryKey() | ||
| { | ||
| return strrev(uniqid('', true)); |
There was a problem hiding this comment.
we will modify this with uuid, no ?
…m/oat-sa/generis into feature/NEX-136/configure-spanner
| 'predicate' => $predicate, | ||
| 'object' => $object, | ||
| 'l_language' => $lang, | ||
| 'author' => is_null($author) ? '' : $author, |
There was a problem hiding this comment.
PHP 7.1 rulez! (at last)... ;-)
| { | ||
| $models = array_map( | ||
| function ($a) { | ||
| return "'" . $a . "'"; |
There was a problem hiding this comment.
Where adding the quotes, in fact. $a is supposed to be a string already.
| public function createModelsTable(Schema $schema) | ||
| { | ||
| $table = $schema->createTable('models'); | ||
| $table->addColumn('modelid', 'string', ['length' => 23, 'notnull' => true]); |
There was a problem hiding this comment.
Yes, right! Thanks for reminder.
| public function createStatementsTable(Schema $schema) | ||
| { | ||
| $table = $schema->createTable('statements'); | ||
| $table->addColumn('id', 'string', ['length' => 23, 'notnull' => true]); |
core/kernel/api/RdsModelFactory.php
Outdated
| { | ||
| $this->dbWrapper->insert('models', ['modeluri' => $namespace]); | ||
| $result = $this->dbWrapper->query('select modelid from models where modeluri = ?', [$namespace]); | ||
| return $result->fetch()['modelid']; |
There was a problem hiding this comment.
Correct. Fixed by adding a test for the previous insert, also in NewSqlRdsModelFactory.
| //TODO bad way, need to find better | ||
| $modelId = $this->getModelId($namespace); | ||
| if ($modelId === false) { | ||
| common_Logger::d('modelId not found, need to add namespace ' . $namespace); |
| ) | ||
| ); | ||
|
|
||
| if (intval($result->fetchColumn()) > 0) { |
| public function getIteratorQuery($modelIds) | ||
| { | ||
| return 'SELECT * FROM statements ' | ||
| . (is_null($modelIds) ? '' : 'WHERE ' . $this->buildModelSqlCondition($modelIds) . ' ') |
| private static $instance = null; | ||
| public static function singleton() | ||
| { | ||
| $returnValue = null; |
There was a problem hiding this comment.
This is not my implementation. I just moved it from the bottom of the same file. But I'll remove this change, I don't want to be anywhere in seeming like writing a singleton method! :D
helpers/UuidPrimaryKeyTrait.php
Outdated
| */ | ||
| public function getUniquePrimaryKey() | ||
| { | ||
| return strrev(uniqid('', true)); |
| \core_kernel_persistence_smoothsql_SmoothModel::OPTION_READABLE_MODELS => array('1'), | ||
| \core_kernel_persistence_smoothsql_SmoothModel::OPTION_WRITEABLE_MODELS => array('1'), | ||
| \core_kernel_persistence_smoothsql_SmoothModel::OPTION_NEW_TRIPLE_MODEL => '1', | ||
| \core_kernel_persistence_smoothsql_SmoothModel::OPTION_READABLE_MODELS => [$writableModelId], |
There was a problem hiding this comment.
This requires PR oat-sa/lib-generis-search#21 and release of lib-generis-search.
This adds NewSql schema support.