Skip to content

fix: Forge::dropColumn() seems to always return false.#8931

Closed
ping-yee wants to merge 9 commits intocodeigniter4:developfrom
ping-yee:240414_sqlsrv
Closed

fix: Forge::dropColumn() seems to always return false.#8931
ping-yee wants to merge 9 commits intocodeigniter4:developfrom
ping-yee:240414_sqlsrv

Conversation

@ping-yee
Copy link
Copy Markdown
Contributor

@ping-yee ping-yee commented Jun 1, 2024 •
edited by kenjis
Loading

Description
Fixes #8849
I think return sql execute result directly is a simplest way to fix this issue.

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 reviewed Jun 1, 2024
View reviewed changes
@kenjis kenjis added 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 labels Jun 1, 2024
kenjis
kenjis reviewed Jun 17, 2024
View reviewed changes
kenjis
kenjis reviewed Aug 5, 2024
View reviewed changes
kenjis
kenjis reviewed Aug 5, 2024
View reviewed changes
kenjis
kenjis reviewed Aug 5, 2024
View reviewed changes
kenjis
kenjis reviewed Aug 12, 2024
View reviewed changes
system/Database/SQLite3/Forge.php
Comment on lines +174 to +177
throw new DatabaseException(
'Failed to drop column. Table: "' . $table
. '", Column: "' . $columns . '"'
);
Copy link
Copy Markdown
Member

@kenjis kenjis Aug 12, 2024

Choose a reason for hiding this comment

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

dropColumn() may return false when $this->db->DBDebug is false.
So make this the same, please.

CodeIgniter4/system/Database/Forge.php

Lines 787 to 800 in f60d277

public function dropColumn(string $table, $columnNames)
{
$sql = $this->_alterTable('DROP', $this->db->DBPrefix . $table, $columnNames);
if ($sql === false) {
if ($this->db->DBDebug) {
throw new DatabaseException('This feature is not available for the database you are using.');
}
return false;
}
return $this->db->query($sql);
}

Copy link
Copy Markdown
Member

@kenjis kenjis Aug 21, 2024

Choose a reason for hiding this comment

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

@ping-yee Can you complete this PR?
We are very close to the completion.

Copy link
Copy Markdown
Contributor Author

@ping-yee ping-yee Sep 15, 2024

Choose a reason for hiding this comment

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

@kenjis Sorry for the stagnation, I haven’t had much energy to deal with things other than work a while ago.
I modify this right now.

Copy link
Copy Markdown
Contributor Author

@ping-yee ping-yee Sep 15, 2024

Choose a reason for hiding this comment

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

Done.

Copy link
Copy Markdown
Contributor Author

@ping-yee ping-yee Sep 15, 2024

Choose a reason for hiding this comment

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

However, the rector removed the return false line, implying that if sqlExecuteResult is false, the method would still return false.

@github-actions github-actions bot added the stale Pull requests with conflicts label Dec 17, 2024
@github-actions
Copy link
Copy Markdown

github-actions bot commented Dec 17, 2024

👋 Hi, @ping-yee!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

@paulbalandan paulbalandan mentioned this pull request Dec 30, 2024
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kenjis kenjis kenjis left review comments

Assignees

No one assigned

Labels

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 stale Pull requests with conflicts

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug(SQLite3): Forge::dropColumn() seems to always return false

2 participants

@ping-yee @kenjis