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

Harden count user logic on upgrade #6002

Closed
wants to merge 1 commit into from
Closed

Conversation

MorrisJobke
Copy link
Member

In addition to #6001 and #5695

@MorrisJobke MorrisJobke added the 3. to review Waiting for reviews label Aug 7, 2017
@MorrisJobke MorrisJobke added this to the Nextcloud 13 milestone Aug 7, 2017
lib/base.php Outdated
$totalUsers = array_sum($stats);
$tooBig = ($totalUsers > 50);
} catch (\Doctrine\DBAL\Exception\TableNotFoundException $e) {
\OC::$server->getLogger()->logException($e, ['level' => \OCP\Util::INFO, 'app' => 'core');
Copy link
Member

Choose a reason for hiding this comment

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

💣 💣

ParseError: syntax error, unexpected ')', expecting ']' in /drone/src/github.com//pull/6002/lib/base.php:350

@codecov
Copy link

codecov bot commented Aug 7, 2017

Codecov Report

Merging #6002 into master will increase coverage by 3.58%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master   #6002      +/-   ##
===========================================
+ Coverage     53.11%   56.7%   +3.58%     
===========================================
  Files          1404      61    -1343     
  Lines         88166    7520   -80646     
  Branches       1327    1327              
===========================================
- Hits          46832    4264   -42568     
+ Misses        41334    3256   -38078
Impacted Files Coverage Δ Complexity Δ
apps/dav/lib/CardDAV/Xml/Groups.php
apps/encryption/lib/Session.php
lib/private/SubAdmin.php
lib/private/DB/OracleConnection.php
...curity/Exceptions/StrictCookieMissingException.php
apps/testing/appinfo/routes.php
apps/files_versions/lib/Hooks.php
lib/private/Session/Internal.php
lib/private/Activity/Manager.php
lib/private/Preview/PDF.php
... and 1332 more

@nickvergessen
Copy link
Member

I don't get what this is for. countUsers only touches the database backends, they should work?
Accounts table is not touched by this...

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

... but sure, anything is better than an exception

@MorrisJobke
Copy link
Member Author

I don't get what this is for. countUsers only touches the database backends, they should work?
Accounts table is not touched by this...

I just want to avoid, that upstream somehow renames this table and then our backend will not be able to count anymore. But we can also not run this code and fix it once it happens. It could also be the case, that a column is renamed or so.

@MorrisJobke
Copy link
Member Author

Makes no sense at all -> closing

@MorrisJobke MorrisJobke closed this Aug 8, 2017
@MorrisJobke MorrisJobke deleted the fix-count-user branch August 8, 2017 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants