Skip to content

Fix undefined index and consequential damages in versions code - #23295

Merged
MorrisJobke merged 2 commits into
masterfrom
fix/versions-no-space-no-versions-undefined-index
Oct 13, 2020
Merged

Fix undefined index and consequential damages in versions code#23295
MorrisJobke merged 2 commits into
masterfrom
fix/versions-no-space-no-versions-undefined-index

Conversation

@ChristophWurst

@ChristophWurstChristophWurst commented Oct 8, 2020

Copy link
Copy Markdown
Member

If the user has no space and there are no versions, there won't be an
all index in the versions entry. Hence this triggers a warning and
becomes null, afterwards count, foreach and friends will happily
throw even more warnings and errors because they don't want to play with
null. Thus adding a fallback to an empty array.

@ChristophWurstChristophWurst added this to the Nextcloud 21 milestone Oct 8, 2020
@ChristophWurstChristophWurst self-assigned this Oct 8, 2020
@ChristophWurst

ChristophWurst commented Oct 8, 2020

Copy link
Copy Markdown
MemberAuthor

/backport to stable20

@ChristophWurst

ChristophWurst commented Oct 8, 2020

Copy link
Copy Markdown
MemberAuthor

/backport to stable19

@kesselb

kesselb commented Oct 8, 2020

Copy link
Copy Markdown
Contributor

Make sense. I also think there are some reports around for this.

@ChristophWurstChristophWurst mentioned this pull request Oct 9, 2020
Merged
nickvergessen
nickvergessen requested changes Oct 9, 2020
View reviewed changes
Comment threadapps/files_versions/lib/Storage.php Outdated
Comment threadapps/files_versions/lib/Storage.php
@MorrisJobke

MorrisJobke commented Oct 9, 2020

Copy link
Copy Markdown
Member

@ChristophWurst I added a commit that added the changes the @nickvergessen proposed, because I think that is the better approach to deal with this.

nickvergessen
nickvergessen reviewed Oct 12, 2020
View reviewed changes
Comment threadapps/files_versions/lib/Storage.php Outdated
@nickvergessen

nickvergessen commented Oct 12, 2020

Copy link
Copy Markdown
Member

DB tests are not happy. Seems to be the "random fail" but it failed on all DBs here, so not sure its unrelated?

@ChristophWurst

ChristophWurst commented Oct 12, 2020

Copy link
Copy Markdown
MemberAuthor

DB tests are not happy. Seems to be the "random fail" but it failed on all DBs here, so not sure its unrelated?

Unfortunately also happening with other PRs right now

ChristophWurstand others added 2 commits October 12, 2020 22:17
@ChristophWurst@MorrisJobke
Fix undefined index and consequential damages in versions code
02fa325
If the user has no space and there are no versions, there won't be an
`all` index in the versions entry. Hence this triggers a warning and
becomes `null`, afterwards `count`, `foreach` and friends will happily
throw even more warnings and errors because they don't want to play with
`null`. Thus adding a fallback to an empty array.
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@MorrisJobke
Return all keys in getAllVersions() instead
6ce05bc
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke
MorrisJobkeforce-pushed the fix/versions-no-space-no-versions-undefined-index branch from 28cd924 to 6ce05bcCompareOctober 12, 2020 20:17
@MorrisJobke

MorrisJobke commented Oct 12, 2020

Copy link
Copy Markdown
Member

DB tests are not happy. Seems to be the "random fail" but it failed on all DBs here, so not sure its unrelated?

Fixed with #23391

@MorrisJobkeMorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 12, 2020
@faily-bot

faily-botBot commented Oct 12, 2020

Copy link
Copy Markdown

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 34039: failure

sqlite

Show full log
There were 2 warnings:
1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
--
There was 1 failure:
1) OCA\Files_Versions\Tests\VersioningTest::testRestoreMovedShare
File content has not changed
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'version 2'
+'version 1'
/drone/src/apps/files_versions/tests/VersioningTest.php:729

mysql8.0-php7.4

@MorrisJobke
MorrisJobke merged commit f8ed08c into masterOct 13, 2020
@MorrisJobke
MorrisJobke deleted the fix/versions-no-space-no-versions-undefined-index branch October 13, 2020 15:35
This was referenced Oct 13, 2020
Merged
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rullzerrullzerrullzer approved these changes
@nickvergessennickvergessennickvergessen approved these changes
@MorrisJobkeMorrisJobkeAwaiting requested review from MorrisJobke
@icewind1991icewind1991Awaiting requested review from icewind1991

Assignees

@ChristophWurstChristophWurst

Labels

4. to releaseReady to be released and/or waiting for tests to finishbugfeature: versions

Projects

None yet

Milestone

Nextcloud 21

Development

Successfully merging this pull request may close these issues.

5 participants

@ChristophWurst@kesselb@MorrisJobke@nickvergessen@rullzer