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

Jump to bottom

Fix conditional check in MySQL setup #23760

Merged
merged 1 commit into from Apr 20, 2016
Merged

Fix conditional check in MySQL setup #23760

merged 1 commit into from Apr 20, 2016

Conversation

RobinMcCorkell
Copy link
Member

The original check didn't make sense - $result is always an object, and always evaluates to true. I believe the intention of the check was if there is any rows in the result, so $row needs to be checked.

Might affect #23637 cc @enoch85

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @bartv2, @DeepDiver1975 and @MorrisJobke to be potential reviewers

@enoch85
Copy link
Member

enoch85 commented Apr 4, 2016

Thanks for the fix @Xenopathic But in my specific case it didn't work: #23637 (comment)

nickvergessen
nickvergessen reviewed Apr 4, 2016
View reviewed changes
lib/private/setup/mysql.php
@@ -43,7 +43,7 @@ public function setupDatabase($username) {
$query='select count(*) from information_schema.tables where table_schema=? AND table_name = ?';
$result = $connection->executeQuery($query, [$this->dbName, $this->tablePrefix.'users']);
$row = $result->fetch();
if(!$result or $row[0]==0) {
if(!$row or $row[0]==0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (empty($row[0])) { should so the same?

Copy link
Member Author

Choose a reason for hiding this comment

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

I shudder at every usage of empty() now, even if it might be appropriate usage...

@RobinMcCorkell
Copy link
Member Author

Further testing by me and @enoch85 showed that actually the whole conditional was broken... the result comes through as $row['count(*)'].

@enoch85
Copy link
Member

enoch85 commented Apr 4, 2016

Tested and working! 👍

@RobinMcCorkell
Copy link
Member Author

⚠️ Since the original check was completely broken and (always?) evaluated to true, we need to manually test the case where the DB already existed before, and make sure it works properly. In the not-already-existing case it works fine (as tested by @enoch85 )

@enoch85
Copy link
Member

enoch85 commented Apr 4, 2016

This is now in the VM: techandme/owncloud-vm@300cb2a

@enoch85
Copy link
Member

enoch85 commented Apr 4, 2016

Backport to stable 9 please?

@RobinMcCorkell
Copy link
Member Author

OK, I tested it - prior to this PR, running an install a second time on the same (already-existing) database fails during table creation. With this PR it doesn't fail on table creation, but rather hits a problem creating the admin user, since there is already an admin user in the user table. In other words, it seems to be working properly with this PR.

For databases that aren't yet created, this PR works exactly the same as pre-PR, just without the 'Undefined offset' warning.

Please review @DeepDiver1975 @MorrisJobke @bartv2

@DeepDiver1975 DeepDiver1975 added this to the 9.1-current milestone Apr 5, 2016
@enoch85
Copy link
Member

enoch85 commented Apr 5, 2016

@Xenopathic Backport request?

@nickvergessen
Copy link
Contributor

Broken since 114f128
Which changed from mysql_fetch_row to \Doctrine\DBAL\Driver\Statement::fetch()
calling fetch(\PDO::FETCH_NUM) should also fix the problem 😉

@karlitschek for the backport

@karlitschek
Copy link
Contributor

great. please backport 👍

enoch85 pushed a commit that referenced this pull request Apr 5, 2016
[stable9] Fix conditional check in MySQL setup
cff3122 
Backport of #23760

Fix conditional check in MySQL setup.
@enoch85 enoch85 mentioned this pull request Apr 5, 2016
Merged
@enoch85
Copy link
Member

enoch85 commented Apr 5, 2016

Backport to stable 9 is here: #23805

@nickvergessen
Copy link
Contributor

Should also backport to 8.2

enoch85 pushed a commit that referenced this pull request Apr 5, 2016
[stable9] Fix conditional check in MySQL setup
f07323d 
Backport of "Fix conditional check in MySQL setup #23760"
@enoch85 enoch85 mentioned this pull request Apr 5, 2016
Merged
@enoch85
Copy link
Member

enoch85 commented Apr 5, 2016

Should also backport to 8.2

#23807

DeepDiver1975 referenced this pull request Apr 6, 2016
@DeepDiver1975
Merge pull request #23805 from owncloud/backport-of-#23760
fa7f187 
[stable9] Fix conditional check in MySQL setup
@RobinMcCorkell
Copy link
Member Author

Rebased onto master to fix the Ceph CI failures

@RobinMcCorkell
Copy link
Member Author

@DeepDiver1975 The stable9 backport has already been merged, this can be merged as soon as CI is finished

@DeepDiver1975 DeepDiver1975 merged commit fff272b into master Apr 20, 2016
@DeepDiver1975 DeepDiver1975 deleted the fix-setup-mysql branch April 20, 2016 11:46
PVince81 referenced this pull request Apr 22, 2016
Merge pull request #23807 from owncloud/backport-of-#23760-1
e912db0 
[stable8.2] Fix conditional check in MySQL setup
@PVince81 PVince81 mentioned this pull request Jun 28, 2016
Closed
@lock
Copy link

lock bot commented Aug 6, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
4 - To release db:mysql Type:Bug
Projects
None yet
Milestone
9.1
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@RobinMcCorkell @mention-bot @enoch85 @nickvergessen @karlitschek @MorrisJobke @DeepDiver1975