Skip to content
This repository has been archived by the owner on Oct 15, 2020. It is now read-only.

Add - During migration manage hasOne relation type. #16

Merged
merged 20 commits into from
Aug 1, 2019

Conversation

abbadon1334
Copy link
Collaborator

when styleCi will be ok.
i will review my code directly on critical changes, just two, but i think i need to specify it clearly.

* add transcode table for field type => datatype database
* tested on SQLite and MySQL
more space is better than less, this class is very useful during development,
after that will be disabled and database must be optimized with other tools.

i changed because i had a problem storing serialized EXIF in array datatype
switch to codeformatting PSR-1,PSR-2
add hasOne in Test
…" DB

fixed mysql float was mistyped uppercase
@codecov
Copy link

codecov bot commented Jul 22, 2019

Codecov Report

Merging #16 into develop will increase coverage by 5.43%.
The diff coverage is 70%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop      #16      +/-   ##
=============================================
+ Coverage      65.82%   71.25%   +5.43%     
- Complexity       131      135       +4     
=============================================
  Files              6        6              
  Lines            316      327      +11     
=============================================
+ Hits             208      233      +25     
+ Misses           108       94      -14
Impacted Files Coverage Δ Complexity Δ
src/Migration/PgSQL.php 0% <0%> (ø) 10 <10> (ø) ⬇️
src/Migration/MySQL.php 100% <100%> (+9.09%) 3 <3> (ø) ⬇️
src/Migration/SQLite.php 100% <100%> (ø) 1 <1> (ø) ⬇️
src/Migration.php 78.15% <72.22%> (+7.89%) 87 <72> (+4) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3277bef...2abf7eb. Read the comment docs.

Copy link
Collaborator Author

@abbadon1334 abbadon1334 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop support of php < 7.2.0

@@ -15,7 +15,7 @@
}
],
"require": {
"php": ">=7.0.0",
"php": ">=7.2.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop php < 7.2.0

@@ -24,6 +24,7 @@ class MySQL extends \atk4\schema\Migration

/** @var array use this array in extended classes to overwrite or extend values of default mapping */
public $mapToAgile = [
0 => ['string'],
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default field type for Mysql = string

@@ -7,6 +7,10 @@ class SQLite extends \atk4\schema\Migration
/** @var string Expression to create primary key */
public $primary_key_expr = 'integer primary key autoincrement';

public $mapToAgile = [
0 => ['string'],
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default field type for SQLite = string

{
// remove parenthesis
$type = trim(preg_replace('/\(.*/', '', strtolower($type)));

$map = array_merge($this->defaultMapToAgile, $this->mapToAgile);
$map = array_replace($this->defaultMapToAgile, $this->mapToAgile);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this must be array_replace, i think

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes if you use numeric keys then it should be array_replace

// FieldType is stored in the reference field
if ($field->reference instanceof HasOne) {

// @TODO if this can be done better?
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i leave a TODO, probably can done better.
I need to get the field type of the Referenced hasOne.
To find it :

  • i create the referenced model stored in $field->reference->model, using the actual persistence/connection
  • i call $refModel->get($field->reference->their_field) but $their_field is protected
  • i think $their_field must remain protected
  • for this specific case, i think the best option is to use Reflection
  • if $their_field is null use $id_field of the referenced model
  • if try to get the type of this field and if is null i return integer

I use reflection because migration is not a normal "production" and performance are not so important here.

Copy link
Member

@DarkSide666 DarkSide666 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically looks good. I didn't actually test. Was just checking code changes and added few comments.


/** @var Model $refModel */
$refModel = new $refModelClass($m->persistence);
$refModel->getField($their_field);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need this line

$refModel = new $refModelClass($m->persistence);
$refModel->getField($their_field);

$type = $refModel->getField($their_field)->type ?? 'integer';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about also checking $m->getField($refModel->our_field ?: 'id')->type before falling back to integer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i do it before, i should change the name the var to $reference_field in place of $their_filed which is misleading

on line 220 : $their_field = $reference_their_field ?? $field->reference->owner->id_field;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i changed comment and naming to be less missleading

{
// remove parenthesis
$type = trim(preg_replace('/\(.*/', '', strtolower($type)));

$map = array_merge($this->defaultMapToAgile, $this->mapToAgile);
$map = array_replace($this->defaultMapToAgile, $this->mapToAgile);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes if you use numeric keys then it should be array_replace

*
* @return string
*/
public function getModelPHPCode(string $table, string $model, string $id_field = 'id', string $namespace = '\Your\Project\Models') :string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this method is here?
It's of course interesting to have, but I don't see it's relation to atk4\schema.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i removed the method and the related test

@romaninsh romaninsh merged commit 4a189ee into atk4:develop Aug 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants