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

Fix comparisons in apps #4790

Merged
merged 11 commits into from
Aug 2, 2017
Merged

Fix comparisons in apps #4790

merged 11 commits into from
Aug 2, 2017

Conversation

nickvergessen
Copy link
Member

Found via #4767

@@ -147,7 +147,7 @@ public function put($data) {
// compare expected and actual size
if (isset($_SERVER['CONTENT_LENGTH']) && $_SERVER['REQUEST_METHOD'] === 'PUT') {
$expected = $_SERVER['CONTENT_LENGTH'];
if ($count != $expected) {
if ($count !== $expected) {
Copy link
Member Author

Choose a reason for hiding this comment

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

int + strings are compared, need to fix

@@ -410,7 +410,7 @@ private function createFileChunked($data) {
if (isset ($_SERVER['REQUEST_METHOD']) && $_SERVER['REQUEST_METHOD'] === 'PUT') {
if (isset($_SERVER['CONTENT_LENGTH'])) {
$expected = $_SERVER['CONTENT_LENGTH'];
if ($bytesWritten != $expected) {
if ($bytesWritten !== $expected) {
Copy link
Member Author

Choose a reason for hiding this comment

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

int + strings are compared, need to fix

@MorrisJobke
Copy link
Member

I would say this is not the best point in time to do stuff like that. 🙈

@MorrisJobke MorrisJobke added the 2. developing Work in progress label May 10, 2017
@nickvergessen
Copy link
Member Author

Yeah I just wanted to have one check passed, so we can run the general tests (info.xml, language and database), but maybe it's easier to add an option to only run those...

@nickvergessen nickvergessen removed the 3. to review Waiting for reviews label May 11, 2017
@MorrisJobke MorrisJobke force-pushed the fix-comparisons-in-apps branch from ef202f3 to 145e3f5 Compare June 19, 2017 21:05
@codecov
Copy link

codecov bot commented Jun 19, 2017

Codecov Report

Merging #4790 into master will decrease coverage by 2.91%.
The diff coverage is 46.96%.

@@             Coverage Diff              @@
##             master    #4790      +/-   ##
============================================
- Coverage     57.06%   54.14%   -2.92%     
- Complexity    21017    22345    +1328     
============================================
  Files          1245     1380     +135     
  Lines         71040    85551   +14511     
  Branches          0     1329    +1329     
============================================
+ Hits          40537    46323    +5786     
- Misses        30503    39228    +8725
Impacted Files Coverage Δ Complexity Δ
apps/files_external/templates/settings.php 0% <ø> (ø) 0 <0> (?)
apps/files_sharing/templates/public.php 0% <ø> (ø) 0 <0> (ø) ⬇️
apps/files_external/lib/Lib/Storage/FTP.php 0% <0%> (ø) 35 <0> (?)
apps/files_sharing/lib/Helper.php 20.35% <0%> (ø) 38 <0> (ø) ⬇️
apps/testing/locking/provisioning.php 0% <0%> (ø) 31 <0> (?)
apps/files_external/ajax/oauth2.php 0% <0%> (ø) 0 <0> (?)
apps/files_external/lib/Lib/Storage/Dropbox.php 0% <0%> (ø) 87 <0> (?)
apps/files_external/lib/Lib/Storage/SFTP.php 0% <0%> (ø) 89 <0> (?)
apps/files_sharing/lib/ShareBackend/Folder.php 54.54% <0%> (ø) 13 <0> (ø) ⬇️
apps/dav/lib/CardDAV/CardDavBackend.php 85.74% <0%> (ø) 88 <0> (ø) ⬇️
... and 564 more

@MorrisJobke
Copy link
Member

Rebased on current master. @nickvergessen Could you please fix the comments you have?

@nickvergessen nickvergessen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 20, 2017
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Unit tests fail

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jun 21, 2017
@@ -161,7 +161,7 @@ private static function getVersionsSize($user) {
* store a new version of a file.
*/
public static function store($filename) {
if(\OCP\Config::getSystemValue('files_versions', Storage::DEFAULTENABLED)=='true') {
if(\OCP\Config::getSystemValue('files_versions', Storage::DEFAULTENABLED)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Comparing bool to string worked before, however this config is not documented anywhere.
@schiessle any idea what other values are and why it exists?

@nickvergessen nickvergessen force-pushed the fix-comparisons-in-apps branch from de1ba6e to 622d80d Compare July 25, 2017 12:25
@nickvergessen nickvergessen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 25, 2017
@nickvergessen
Copy link
Member Author

Rebased and fixed

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍 Code looks good

@rullzer
Copy link
Member

rullzer commented Aug 1, 2017

Needs a rebase...

@MorrisJobke
Copy link
Member

Needs a rebase...

Let me try.

@MorrisJobke MorrisJobke force-pushed the fix-comparisons-in-apps branch from 622d80d to a209342 Compare August 1, 2017 11:59
@MorrisJobke
Copy link
Member

Rebased.

@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 1, 2017
@nickvergessen
Copy link
Member Author

Phan fail is unrelated?

@MorrisJobke
Copy link
Member

Phan fail is unrelated?

Correct - this is fixed in master.

@MorrisJobke MorrisJobke merged commit 6290ba8 into master Aug 2, 2017
@MorrisJobke MorrisJobke deleted the fix-comparisons-in-apps branch August 2, 2017 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants