WIP Added validation on exists database before created for MySQLi…

[oleg1540]

Description
#1759
Added validation on exists database before created for MySQLi and Postgre Forge Class.

Checklist:

• Securely signed commits
• Component(s) with PHPdocs
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[MGatner]

I like the code and concept but I'm not sure that the framework should just gloss over a logic error like creating a database that already exists, returning true as though it made it. What about an additional parameter to Forge->createDatabase() to "ignoreExisting", or something similar?

[oleg1540]

I agree with you. Framework must inform user about this error by default.
Ok, I can add additional parameter checkExisting = false. If it's true, then we check db exists before create and if db exist then ignore error. What do you think?

[MGatner]

Probably best to model it after the same feature on the createTable command:

public function createTable(string $table, bool $if_not_exists = false, array $attributes = [])

(Though by the style guide that should be camel case $ifNotExists)

[MGatner]

The User Guide will also need to be updated.

[oleg1540]

Ok.
User Guide is in user_guide_src folder, right?

[MGatner]

Correct! Particularly you want user_guide_src/source/dbmgmt/forge.rst

[oleg1540]

Thank you!

[MGatner]

This is looking pretty good. We'll need some tests - I think they should be relatively easy to copy the existing ones and add a few cases: tests/system/Database/Live/ForgeTest.php. Also your upstream merge wasn't signed - if you add GPG to the source of that commit it will retroactively add the verification, or you can redo it.

[oleg1540]

Good, I'll add some test cases.
Unfortunately, merge commit wasn't sign. I try to fix it.

[MGatner]

@oleg1540 The additions look good, but now SQLite seems to be failing your tests because it cannot drop the database.

[MGatner]

Great! Thanks a lot. @lonnieezell or @jim-parry I believe this is ready to go, if you want to give it a glance first?

[lonnieezell]

Just one nit-picky change requested. Other than that looks good.

At some point I feel we could probably get the SQLite tests here working by giving it a path, then checking if the file exists...

[oleg1540]

@lonnieezell Maybe dropDatabase() in these tests don't run if DbDriver == 'SQLite3'? So we can check createDatabese function, dropDatabase() function is optional in these tests.

[lonnieezell]

@oleg1540 What error were you hitting when running the tests with dropDatabase() under SQLite3? We can't really trust that createDatabase() tests are working if they previously ran the tests and then the database it created was never cleaned up.

Looking at the logic in those tests it first creates the database (which creates a file). After getting the response from that it drops the database (which should only throw an exception if the file doesn't exist - but we just created it so it should).

Seems like it should work.

[oleg1540]

@lonnieezell At first I didn't write any limitations for these tests. But tests failed on SQLite3 driver with error

CodeIgniter\Database\Exceptions\DatabaseException: Unable to drop the specified database

So I disabled it for SQLite3 driver.

We can't really trust that createDatabase() tests are working if they previously ran the tests and then the database it created was never cleaned up.

I agree with you.

You offer to add if (is_file($dbName)) for SQLite3 driver in these tests, right?

[lonnieezell]

Well - what I was getting at is that it would be nice to get to the bottom of why it couldn't drop the database and go from there. If it's something that we can't easily fix that only happens at Travis then I have no problems skipping the test. But if it's something we can fix we should fix it.

[oleg1540]

@lonnieezell Method createDatabase for SQLite3 always return true, because

// In SQLite, a database is created when you connect to the database.
// We'll return TRUE so that an error isn't generated.

and database would be named as 'database' param from config.
In this way, test database name will be ignored in these test. Therefore, we don't need to delete the database after the test.

[lonnieezell]

I'm going to go ahead and merge this for now. We can revisit the skipped SQLite tests at a later date if desired.