Skip to content

Fix incorrect if conditions in View #31754

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.

Sign up for GitHub

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
Merged
merged 3 commits into from
Apr 5, 2022
Merged

Fix incorrect if conditions in View #31754

merged 3 commits into from
Apr 5, 2022

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Mar 30, 2022

Signed-off-by: Côme Chilliet come.chilliet@nextcloud.com

@come-nc come-nc added the 3. to review Waiting for reviews label Mar 30, 2022
@come-nc come-nc added this to the Nextcloud 24 milestone Mar 30, 2022
@come-nc come-nc requested a review from icewind1991 March 30, 2022 09:20
@come-nc come-nc self-assigned this Mar 30, 2022
@CarlSchwan
Copy link
Member

Could you add a bit more info in the commit message about what this fix and why the previous code was incorrect?

@come-nc come-nc force-pushed the fix/view-inconsistent-if-conditions branch from adf4bc1 to dcd2a68 Compare March 30, 2022 10:33
@come-nc
Copy link
Contributor Author

come-nc commented Mar 30, 2022

Could you add a bit more info in the commit message about what this fix and why the previous code was incorrect?

Done.

@blizzz blizzz mentioned this pull request Mar 31, 2022
Merged
@PVince81 PVince81 requested review from CarlSchwan, Pytal and artonge April 4, 2022 07:52
CarlSchwan
CarlSchwan approved these changes Apr 4, 2022
View reviewed changes
Copy link
Member

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

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

This probably only make a difference when you can't read a file but still have permission to do other stuff (file drop)

@artonge
Copy link
Contributor

artonge commented Apr 4, 2022

Could we take the opportunity to exit early on this condition instead of wrapping 100 lines into the if: https://github.com/nextcloud/server/blob/dcd2a6841e9f1f111f74c9f6006c3821bfb12e3d/lib/private/Files/View.php#L1446=

@come-nc
Copy link
Contributor Author

come-nc commented Apr 5, 2022

Could we take the opportunity to exit early on this condition instead of wrapping 100 lines into the if: https://github.com/nextcloud/server/blob/dcd2a6841e9f1f111f74c9f6006c3821bfb12e3d/lib/private/Files/View.php#L1446=

Done

CarlSchwan
CarlSchwan reviewed Apr 5, 2022
View reviewed changes
@come-nc
Copy link
Contributor Author

come-nc commented Apr 5, 2022

/rebase

@come-nc
Copy link
Contributor Author

come-nc commented Apr 5, 2022

/backport to stable23

@come-nc
Copy link
Contributor Author

come-nc commented Apr 5, 2022

/backport to stable22

come-nc added 2 commits April 5, 2022 13:58
@come-nc
Fix incorrect if conditions in View
916fbe8 
($something->getPermissions() && Constants::PERMISSION_READ) does not
  make sense as PERMISSION_READ contant is 1 this will always evaluate to
  true.
getPersmissions is returning an int which is a bitwise combination as
  documented in the interface, so it should be used with bit operators.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Exit early if $storage is falsy in View::getDirectoryContent
d36a1a7 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@nextcloud-command nextcloud-command force-pushed the fix/view-inconsistent-if-conditions branch from be6c7ba to d36a1a7 Compare April 5, 2022 13:58
@come-nc
composer run cs:fix
140624d 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc mentioned this pull request Apr 5, 2022
Merged
@come-nc come-nc merged commit fff26ad into master Apr 5, 2022
@come-nc come-nc deleted the fix/view-inconsistent-if-conditions branch April 5, 2022 16:06
@backportbot-nextcloud
Copy link

The backport to stable23 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable22 failed. Please do this backport manually.

@solracsf
Copy link
Member

solracsf commented Apr 5, 2022

/backport to stable23

@solracsf
Copy link
Member

solracsf commented Apr 5, 2022

/backport to stable22

@backportbot-nextcloud
Copy link

The backport to stable22 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable23 failed. Please do this backport manually.

@blizzz blizzz mentioned this pull request Apr 7, 2022
Merged
@come-nc come-nc mentioned this pull request Apr 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CarlSchwan CarlSchwan CarlSchwan approved these changes

@icewind1991 icewind1991 icewind1991 approved these changes

@artonge artonge artonge approved these changes

@Pytal Pytal Awaiting requested review from Pytal

Assignees

@come-nc come-nc

Labels

3. to review Waiting for reviews

Projects

None yet

Milestone

Nextcloud 24

Development

Successfully merging this pull request may close these issues.

5 participants

@come-nc @CarlSchwan @artonge @solracsf @icewind1991