Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add $cached param to BaseConnection::tableExists() #6364

Merged
merged 6 commits into from
Aug 15, 2022

Conversation

sclubricants
Copy link
Member

@sclubricants sclubricants commented Aug 9, 2022

Follow-up #6249
This PR fixes #6351

In Forge::createTable()

        // If table exists lets stop here
        if ($ifNotExists === true && $this->db->tableExists($table, false)) {
            $this->reset();

            return true;
        }

This PR forces to check if table exists rather than relying on cache.

We add $tableName param to Connection::_listTables(). This allows us to lookup only one table rather than return a full list of tables.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added bug Verified issues on the current code behavior or pull requests that will fix them breaking change Pull requests that may break existing functionalities database Issues or pull requests that affect the database layer labels Aug 10, 2022
@sclubricants
Copy link
Member Author

Any idea what is wrong with Mock?

1) CodeIgniter\Database\Forge\CreateTableTest::testCreateTableWithExists
Error: Class "Mock_MockQuery_c8b84d32" not found

/home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/Mock/MockConnection.php:58
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/BaseConnection.php:1423
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/Forge.php:501
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Database/Forge/CreateTableTest.php:39
    public function query(string $sql, $binds = null, bool $setEscapeFlags = true, string $queryClass = '')
    {
        $queryClass = str_replace('Connection', 'Query', static::class);

        $query = new $queryClass($this);

@kenjis
Copy link
Member

kenjis commented Aug 10, 2022

  1. CodeIgniter\Database\Forge\CreateTableTest::testCreateTableWithExists
    Error: Class "Mock_MockQuery_c8b84d32" not found

Because it uses PHPUnit mock.
The Mock_MockQuery_c8b84d32 does not exists.

Don't worry. The test is no longer of much use.

@sclubricants
Copy link
Member Author

I'll remove it and create a live test

@kenjis
Copy link
Member

kenjis commented Aug 10, 2022

Well not sure what to do about it. Do I need to create a mock class?

No, you can't. The part c8b84d32 of the classname Mock_MockQuery_c8b84d32 will be changed randomly.

I'll remove it and create a live test

Yes, createTable() now depends on the SQL, so the test with mock does not make sense much.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

This is good work! Please address the comments.

@sclubricants
Copy link
Member Author

sclubricants commented Aug 10, 2022

I added code that detects when cache is out of sync. If out of sync then we call resetDataCache().

However only if dataCache['table_names'] has been built already.

system/Database/MySQLi/Connection.php Outdated Show resolved Hide resolved
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for the null changes. @kenjis anything else from you?

@sclubricants sclubricants mentioned this pull request Aug 13, 2022
5 tasks
Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

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

Thank you.

@kenjis kenjis merged commit 42e5b69 into codeigniter4:develop Aug 15, 2022
@sclubricants sclubricants deleted the DbTableExists branch August 19, 2022 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them database Issues or pull requests that affect the database layer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: createTable() method in Forge class checks for table existence using cache and causes problems
3 participants