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

Revert interface changes from "store unencrypted size in the unencrypted_size column" #32943

Merged
merged 5 commits into from
Aug 17, 2022

Conversation

icewind1991
Copy link
Member

This reverts #31966 and uses the changes from #32708 instead which removes the significant interface changes at the cost of a "less nice" solution.

Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@CarlSchwan
Copy link
Member

now this will break all the app I changed a few days ago. For files_lock the solution was nextcloud/files_lock#77

@icewind1991
Copy link
Member Author

The casts to string will still work, it will just be a noop

lib/private/Files/Cache/Propagator.php Outdated Show resolved Hide resolved
@PVince81
Copy link
Member

do we still need this or do we keep this change for 25 at least ?

@danxuliu
Copy link
Member

Please check #33484 for a regression introduced in this pull request (second item in the list, storage stats size after deleting the last file in a folder is wrong).

@icewind1991 icewind1991 force-pushed the unencrypted-size-revert-interface-changes branch from 9e104da to b3d6728 Compare August 16, 2022 08:52
@icewind1991
Copy link
Member Author

@danxuliu thanks for making the test, regression should be fixed now

@icewind1991 icewind1991 force-pushed the unencrypted-size-revert-interface-changes branch 2 times, most recently from e09a3c6 to 2813e5e Compare August 16, 2022 09:57
@icewind1991 icewind1991 force-pushed the unencrypted-size-revert-interface-changes branch from 2813e5e to a4cbb16 Compare August 16, 2022 10:12
@icewind1991 icewind1991 force-pushed the unencrypted-size-revert-interface-changes branch from a4cbb16 to de63f63 Compare August 16, 2022 11:54
@danxuliu
Copy link
Member

@danxuliu thanks for making the test, regression should be fixed now

I have run the integration tests again rebased on the latest commit and that issue is indeed fixed. Thanks!

@icewind1991
Copy link
Member Author

do we still need this or do we keep this change for 25 at least ?

Imo it still makes sense to merge this to prevent needless churn for apps

@PVince81
Copy link
Member

@icewind1991 I'm worried that apps might have already adjusted by now, especially if the interface change was already released recently or was it not ?

@icewind1991
Copy link
Member Author

24 doesn't have the interface change.

For apps that added casts like nextcloud/files_lock#77, it will still work with this PR applied, only if apps extended the querybuilder and overwrote existing method should they need to adjust for this PR

@PVince81 PVince81 merged commit 312b719 into master Aug 17, 2022
@PVince81 PVince81 deleted the unencrypted-size-revert-interface-changes branch August 17, 2022 09:36
@skjnldsv
Copy link
Member

This breaks master,

{
  "reqId": "r9OmEUeVynCcFsLI8Lvu",
  "level": 3,
  "time": "2022-08-17T13:37:17+00:00",
  "remoteAddr": "172.18.0.1",
  "user": "admin",
  "app": "PHP",
  "method": "GET",
  "url": "/ocs/v2.php/apps/notifications/api/v2/notifications",
  "message": "Declaration of OC\\DB\\QueryBuilder\\ExpressionBuilder\\ExpressionBuilder::comparison($x, string $operator, $y, $type = null): string must be compatible with OCP\\DB\\QueryBuilder\\IExpressionBuilder::comparison($x, string $operator, $y, $type = null): OCP\\DB\\QueryBuilder\\IQueryFunction at /var/www/nextcloud/lib/private/DB/QueryBuilder/ExpressionBuilder/ExpressionBuilder.php#119",
  "userAgent": "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/94.0.4606.81 Safari/537.36",
  "version": "25.0.0.5",
  "data": {
    "app": "PHP"
  }
}

@skjnldsv
Copy link
Member

Hum, weird, they look identical! 🤔

@skjnldsv
Copy link
Member

#31966 (comment)

@skjnldsv skjnldsv mentioned this pull request Aug 18, 2022
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.

6 participants