Refactor db code #1143

kirtangajjar · 2018-08-02T09:15:49Z

For why we needed refactor, see - #1141.

This PR refactors methods of EE_DB class methods for better support of multiple tables and also migrates from Sqlite3 to PDO. Along with it, it also handles - #1153 (Refactor db schema) and #1161 (Add model class) since they are related and have to some extent overlapping work.

Related PRs -
EasyEngine/site-command#96
EasyEngine/site-type-wp#16
EasyEngine/cron-command#13

Closes #1153, Closes #1161, Closes #1141

Signed-off-by: Kirtan Gajjar <[email protected]>

Fix function name in where Signed-off-by: Kirtan Gajjar <[email protected]>

Fix EE_Site_Command Signed-off-by: Kirtan Gajjar <[email protected]>

Signed-off-by: Kirtan Gajjar <[email protected]>

Correct __get method Signed-off-by: Kirtan Gajjar <[email protected]>

Signed-off-by: Kirtan Gajjar <[email protected]>

rahulsprajapati · 2018-08-13T12:14:15Z

php/EE/Model/Base.php

+	 *
+	 * @throws \Exception
+	 *
+	 * @return bool Model deleted` successfully


"deleted`" typo.

rahulsprajapati · 2018-08-13T12:19:44Z

php/EE/Model/Base.php

+			EE::db()
+				->table( static::$table )
+				->where( $column, $value )
+				->all()


check first if you're getting anything here or not then use the many_array_to_model function to parse.

Added Exception instead.

rahulsprajapati · 2018-08-13T12:20:59Z

php/EE/Model/Base.php

+/**
+ * Base EE Model class.
+ */
+abstract class Base extends \ArrayObject


Why this need to be extended from ArrayObject?

Because

It would allow us to access any object property from array notation

It allows us to use array_* functions on array of models.
i.e.

$sites = Sites::all(); $site_paths = array_column( $sites, 'site_fs_path' );

I guess you can use ArrayObject https://github.com/EasyEngine/easyengine/pull/1143/files#diff-9a84c75c5309379993ff2ed9362b8772R309 instead of that.

return new static( $arr );

Since you're not going to need other functions of Base.

Actually the idea is that you'll be able to use both the methods you've defined on Model as well as methods of ArrayObject.

i.e. Its possible to do

$sites = Site::all(); $sites[0]->add_default_cron(); //some method defined on site model $sites[0]->delete();

Also if we return an ArrayObject we won't be able to update models easily as now like

$site = site::find('abc.com'); $site->is_ssl = true; $site->save(); //If ArrayObject is returned we won't be able to do this.

Hence It's best to use return Model the model and have the model extend ArrayObject

Verified. There's no need to extend ArrayObject hence removing it.

rahulsprajapati · 2018-08-13T12:22:14Z

php/EE/Model/Base.php

+ * Base EE Model class.
+ */
+abstract class Base extends \ArrayObject
+{


Move this bracket to above line[L10], like as we are using it other places.

rahulsprajapati · 2018-08-13T12:30:30Z

php/EE/Runner.php


 		// Getting the type of the respective site-name from database.
-		$type = EE::db()::get_site_command( $site_name );
+		$type = Site::find( $site_name )->site_type;


this find method have false in return. So please check if you're getting proper response then try to use the site_type from it.

There' a function called above - auto_site_name. It ensures site is present in database. If it isn't it will throw error. Hence Site::find() will never return false.

rahulsprajapati · 2018-08-14T06:25:26Z

php/EE/Model/Site.php

+	 *
+	 * @return string type of site.
+	 */
+	public static function get_site_command( $site_name ) {


if you want to get the site type from this function as @return comment. I think this function name should be get_site_type only. or maybe update @return comment to: type/command of site

Removed the function as it isn't necessary now

rahulsprajapati · 2018-08-14T06:35:19Z

php/class-ee-db.php

 	 *
-	 * @param array $data in key value pair.
+	 * @return array Record
+	 * @throws Exception


Please move this @throws before @return in this file or if there are other places in this PR for consistency in some place you have added @throws before @return and in some place after that. Please add it before @return.

ref: https://github.com/EasyEngine/easyengine/pull/1143/files#diff-cfaa88ab8e5a4e9f3e67c13e5d049636R66

rahulsprajapati · 2018-08-14T06:48:50Z

php/class-ee-db.php

 	 */
-	public static function get_site_command( $site_name ) {
+	private function get_where_fragment( $condition ) {
+		$column   = $condition[0];


Please add one condition before using $condition[0] here and return if below condition is not satisfied.

if ( ! is_array( $condition ) || empty( $condition ) || count( $condition ) > 3 ) {
// return or exeption.....
}

Added below

why below, what's the point checking empty after that ???
you know that if you try to access index 0 value if the variable is empty it will give you warning/error right? That's why I specifically asked you to add it before this line.

Ohh... Okay. I've moved the condition to the top and added array type hint in parameter(and removed is_array from if condition ).

rahulsprajapati · 2018-08-14T06:50:08Z

php/class-ee-site.php

 */
-abstract class EE_Site_Command {
+abstract class EE_Site_Command
+{


why this bracket is again in new line? Please check it all places and correct it.

rahulsprajapati · 2018-08-14T06:56:59Z

php/class-ee-site.php


+			$this->site['type'] = $site->site_type;
+			$this->site['root'] = $site->site_fs_path;
+			$this->le = null !== $site->site_ssl;


please add a bracket here:
$this->le = ( null !== $site->site_ssl );

At first look it seems we have assign this.... $this->le = null

rahulsprajapati · 2018-08-14T07:06:11Z

php/class-ee-db.php

+
+		$set_clause = implode( $set_keys, ' = ?, ' ) . ' = ?';
+
+		$query         = "UPDATE $this->tables SET $set_clause{$this->where['query_string']}";


I think here you should also add the modified_time for site table. or you're doing it in all places where you update any data in site table?

class-ee-db.php is a generic class and will be used by all tables. So instead of adding modified_date here, I will add it in Site model's save() method. (That's what we're using everywhere to update a site)

Yes, that should be the case.

Signed-off-by: Kirtan Gajjar <[email protected]>

rahulsprajapati · 2018-08-16T04:56:36Z

php/EE/Model/Base.php

+	 *
+	 * @throws \Exception
+	 *
+	 * @param string|int $index Name of property to check


why @param after @throws ?

Signed-off-by: Kirtan Gajjar <[email protected]>

Also corrected array usage Signed-off-by: Kirtan Gajjar <[email protected]>

Initial db refactoring code

cbd23a3

kirtangajjar changed the title ~~WIP: Refactoring db code~~ WIP: Refactor db code Aug 2, 2018

kirtangajjar self-assigned this Aug 6, 2018

kirtangajjar added 8 commits August 7, 2018 22:48

Finalize EE_DB refactoring

a0fe01a

Signed-off-by: Kirtan Gajjar <[email protected]>

Fixed formatting

b07bd16

Signed-off-by: Kirtan Gajjar <[email protected]>

Fix ee site command

9d0a33d

Fix function name in where Signed-off-by: Kirtan Gajjar <[email protected]>

Fix EE_DB Commands

9ba053a

Fix EE_Site_Command Signed-off-by: Kirtan Gajjar <[email protected]>

Merge branch 'develop-v4' into refactor-db

27279ed

Refactor DB

141a0b4

Signed-off-by: Kirtan Gajjar <[email protected]>

Update site tables

2de3ba1

Signed-off-by: Kirtan Gajjar <[email protected]>

Merge remote-tracking branch 'origin/develop-v4' into refactor-db

5f52633

mrrobot47 mentioned this pull request Aug 9, 2018

Add initial mailhog-command EasyEngine/mailhog-command#1

Merged

kirtangajjar added 11 commits August 10, 2018 05:02

Add Models of EE

ab6ee87

Signed-off-by: Kirtan Gajjar <[email protected]>

Made required changes in DB

ceaa99f

Signed-off-by: Kirtan Gajjar <[email protected]>

Apply refactor changes in EE_Site_Command

402e95b

Signed-off-by: Kirtan Gajjar <[email protected]>

Apply refactoring changes in Migrations

49d4bf6

Signed-off-by: Kirtan Gajjar <[email protected]>

Update base table. Add more moethods

d91c8a7

Signed-off-by: Kirtan Gajjar <[email protected]>

Update models

8e51971

Signed-off-by: Kirtan Gajjar <[email protected]>

Update DB and Site Command

5a154b8

Signed-off-by: Kirtan Gajjar <[email protected]>

Correct insert method

7ff3626

Signed-off-by: Kirtan Gajjar <[email protected]>

Add parameter to find_* method

8ce01cd

Correct __get method Signed-off-by: Kirtan Gajjar <[email protected]>

Correct wildcard logic

ec9c278

Signed-off-by: Kirtan Gajjar <[email protected]>

Rename app_site_type to app_sub_type

393a0e1

Signed-off-by: Kirtan Gajjar <[email protected]>

kirtangajjar changed the title ~~WIP: Refactor db code~~ Refactor db code Aug 13, 2018

rahulsprajapati self-requested a review August 13, 2018 11:48

rahulsprajapati suggested changes Aug 14, 2018

View reviewed changes

rahulsprajapati reviewed Aug 14, 2018

View reviewed changes

rahulsprajapati mentioned this pull request Aug 14, 2018

Refactor to accommodate new DB changes EasyEngine/site-type-wp#16

Merged

Made changes according to PR

1d5e4e4

Signed-off-by: Kirtan Gajjar <[email protected]>

rahulsprajapati reviewed Aug 16, 2018

View reviewed changes

kirtangajjar added 5 commits August 16, 2018 12:53

Correct docblock

bc83e91

Signed-off-by: Kirtan Gajjar <[email protected]>

Add array type hint. Return early on args validation.

df06474

Signed-off-by: Kirtan Gajjar <[email protected]>

Remove inheritance from ArrayObject in Base class

c3c5dda

Signed-off-by: Kirtan Gajjar <[email protected]>

Fix phpcs errors

14b24b9

Signed-off-by: Kirtan Gajjar <[email protected]>

Fix phpcs errors

5d8a65c

Signed-off-by: Kirtan Gajjar <[email protected]>

rahulsprajapati previously approved these changes Aug 16, 2018

View reviewed changes

Remove array notation usage

ce73d27

Signed-off-by: Kirtan Gajjar <[email protected]>

kirtangajjar dismissed rahulsprajapati’s stale review via ce73d27 August 17, 2018 06:37

Correct primary key usage

762f57c

Also corrected array usage Signed-off-by: Kirtan Gajjar <[email protected]>

rahulsprajapati approved these changes Aug 20, 2018

View reviewed changes

Merge branch 'develop-v4' into refactor-db

3ff7bc1

rahulsprajapati approved these changes Aug 20, 2018

View reviewed changes

rahulsprajapati merged commit 75595ee into EasyEngine:develop-v4 Aug 20, 2018

This was referenced Oct 4, 2018

Change column name use from is_enabled to site_enabled EasyEngine/site-command#154

Merged

Errors due to incomplete search-replace EasyEngine/site-command#153

Closed

kirtangajjar deleted the refactor-db branch November 23, 2018 09:35


		$set_clause = implode( $set_keys, ' = ?, ' ) . ' = ?';

		$query = "UPDATE $this->tables SET $set_clause{$this->where['query_string']}";

Refactor db code #1143

Refactor db code #1143

Uh oh!

Conversation

kirtangajjar commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kirtangajjar Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kirtangajjar commented Aug 2, 2018 •

edited

Loading

kirtangajjar Aug 16, 2018 •

edited

Loading