CodeIgniter4
CodeIgniter4 copied to clipboard
codeigniter4
Improve Connection::_foreignKeyData()
This PR improves the method for retrieving foreign keys. It aligns all DBMS to use the same naming convention. It adds some more information such as delete, update, and match rules. The returned dataset uses a new structure which uses one row per foreign key with an array of fields - useful for composite foreign keys.
This is the first part of https://github.com/codeigniter4/CodeIgniter4/pull/6430
Checklist:
- [x] Securely signed commits
- [x] Component(s) with PHPDoc blocks, only if necessary or adds value
- [x] Unit testing, with >80% coverage
- [x] User guide updated
- [x] Conforms to style guide
The returned dataset uses a new structure which uses one row per foreign key with an array of fields
Can you add the structure in the user guide?
@sclubricants Thank you for this PR. This is easy to understand!
The new PHPStan reports new errors. We've fixed. Please rebase to get rid of the errors. See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch
There are a few GitHub Action check failures. Also please check.
There are two PHPUnit / PHP 7.4 - MySQLi (pull_request) tests. Not sure why. Not sure why one of them is failing.
ErrorException: Undefined property: stdClass::$constraint_name
Connection::_foreignKeyData()
$sql = '
SELECT
tc.constraint_name,
....
tc.table_name = ' . $this->escape($table);
if (($query = $this->query($sql)) === false) {
throw new DatabaseException(lang('Database.failGetForeignKeyData'));
}
$query = $query->getResultObject();
$indexes = [];
foreach ($query as $row) {
$indexes[$row->constraint_name]['constraint_name'] = $row->constraint_name;
The only way I can see this failing is if MySql isn't returning the fields in lower case. I had this problem with Oracle in upper case.
There are two PHPUnit / PHP 7.4 - MySQLi (pull_request) tests. Not sure why. Not sure why one of them is failing.
The last one is MySQL 8.0.
https://github.com/codeigniter4/CodeIgniter4/pull/6468/commits/6bfd0bec2c1e71e3c18f8fcd9ad8fc7dad85a577
Fix MySql _foreignKeyData() SQL upper case
Please write why you changed to uppercase in the commit message.
We can easily know that this commit changed the column names to uppercase, but it is difficult to know why this change was needed from the commit.
https://github.com/codeigniter4/CodeIgniter4/pull/6468/commits/1b03ee7a7fcdf8c21ea8d9e425cb71dfc5c58a88
Using a prefix causes problems when using dropForeignKey().
What's the problems?
What's the problems?
When you call dropForeignKey() the table parameter gets prefixed by table prefix. I mentioned it elsewhere here..
Please write why you changed to uppercase in the commit message.
Good question.. basically cause it worked. Its something I've never seen with MySql. MySql has always returned the same letter case as the sql is typed in my experience but for some reason the server you are running that test on returns upper case no matter what.
Perhaps there might be a configuration for this?
basically cause it worked.
It seems MySQL 8 changed the behavior.
what are the changes in mysql 8 result rowset case? - Stack Overflow https://stackoverflow.com/questions/54538448/what-are-the-changes-in-mysql-8-result-rowset-case
When you call dropForeignKey() the table parameter gets prefixed by table prefix.
Oh, we can't drop any key that does not begin with the DB prefix!
https://github.com/codeigniter4/CodeIgniter4/blob/a8072a51bb8c1ea05998d9a045ce61d2c78e32bc/system/Database/Forge.php#L466-L470
The DB prefix was added in: https://github.com/codeigniter4/CodeIgniter4/pull/609/commits/fdb853810199df2adaadc3f5059e136b2ee0d75c#diff-a07677ecb7c1a5d29e0d0a8e72f00b5b77b577508602ada4fb8c5c1e0c7c5916
Oh, we can't drop any key that does not begin with the DB prefix!
We definitely want to have the prefix. Someone might have a different connection on same database with duplicates of tables but with different prefixes. This way we avoid name collision.
We definitely want to have the prefix. Someone might have a different connection on same database with duplicates of tables but with different prefixes. This way we avoid name collision.
Do we need the prefix for foreign key name?
FYI, if we allow setting the name manually this won't apply to SQLite. SQLite does not really support naming the constraint. You can name it when you create table but you cannot retrieve the name with the foreign keys. My guess this is because you can't drop or alter the foreign keys so it doesn't need a name. Allowing people to specify a name should through an error. They will need to use the generated name that we assign to it.
FYI, if we allow setting the name manually this won't apply to SQLite. SQLite does not really support naming the constraint.
Yes, I'm aware of this. Since we had never any controls over constraint names in SQLite I don't recognize it as a problem. Adding a friendly note in the user guide about SQLite should be enough.
I was wondering if throwing an error when we define a name in SQLite would be a good choice... simply from a practical perspective. SQLite is frequently used during tests in smaller projects and throwing such an error may be frustrating for some use cases (tests on SQLite3 and production on MySQL). But probably throwing an error is the right thing to do here.
@sclubricants Please rebase to resolve conflicts. It seems ready! Thank you for your cooperation.
