Skip to content

Tidy up typing in OC\Files\View #36836

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
come-nc merged 8 commits into from
Apr 5, 2023
Merged

Tidy up typing in OC\Files\View #36836

come-nc merged 8 commits into from
Apr 5, 2023

Conversation

@come-nc
Copy link
Contributor

@come-nc come-nc commented Feb 23, 2023 •
edited
Loading

Summary

Add return types and fix psalm complaints in View and related classes.

Checklist

@come-nc come-nc added the 2. developing Work in progress label Feb 23, 2023
@come-nc come-nc added this to the Nextcloud 27 milestone Feb 23, 2023
@come-nc come-nc self-assigned this Feb 23, 2023
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 23, 2023
View reviewed changes
@come-nc come-nc mentioned this pull request Feb 28, 2023
Merged
4 tasks
@come-nc come-nc force-pushed the fix/view-type-cleanup branch from 8845976 to 73a0066 Compare March 13, 2023 17:34
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 13, 2023
View reviewed changes
@come-nc
Copy link
Contributor Author

come-nc commented Mar 14, 2023 •
edited
Loading

@come-nc
Copy link
Contributor Author

come-nc commented Mar 27, 2023

/rebase

@come-nc
Copy link
Contributor Author

come-nc commented Mar 30, 2023

@icewind1991 https://github.com/nextcloud/server/blob/master/apps/dav/lib/Connector/Sabre/Directory.php#L139-L141 this is blowing up because it puts null in non-nullable parameters.
Any idea how to refactor this properly?

@come-nc
Copy link
Contributor Author

come-nc commented Mar 30, 2023

@icewind1991 https://github.com/nextcloud/server/blob/master/apps/dav/lib/Connector/Sabre/Directory.php#L139-L141 this is blowing up because it puts null in non-nullable parameters. Any idea how to refactor this properly?

For now I went back to weak typing for internalPath in FileInfo.

come-nc added 7 commits April 3, 2023 10:52
@come-nc
Tidy up typing in OC\Files\View
966a3e6 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
Remove unused method getLocalFolder
4393b96 
It’s not used and not in any OCP interface/class.

Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
Improve typing for fopen/toTmpFile
f974281 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
Fix return type of methods returning false on error
ea05544 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
More type cleanup in View and FileInfo
0b3dad8 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
View needs to return an instance of OC\Files\FileInfo explicitely
5ad0456 
Applications are calling methods from the class which are not from the
 public interface, and which cannot be added easily to public interface
 because Node interface extends FileInfo.

Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
Fix dav application tests
8f26a5d 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc come-nc force-pushed the fix/view-type-cleanup branch from e618668 to 8f26a5d Compare April 3, 2023 08:52
@come-nc come-nc marked this pull request as ready for review April 3, 2023 13:02
@come-nc come-nc requested review from a team, ArtificialOwl, ChristophWurst, blizzz, icewind1991 and provokateurin and removed request for a team April 3, 2023 13:03
ChristophWurst
ChristophWurst reviewed Apr 4, 2023
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

👍 looks ok but I'm not familiar enough with this area of the code to given an approval

icewind1991
icewind1991 approved these changes Apr 4, 2023
View reviewed changes
Copy link
Member

@icewind1991 icewind1991 left a comment

Choose a reason for hiding this comment

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

Good work otherwise 👍

lib/private/Files/View.php
* @param string $path
* @return string
*/
public function getLocalFolder($path) {
Copy link
Member

@icewind1991 icewind1991 Apr 4, 2023

Choose a reason for hiding this comment

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

is deleting this intentional?

Copy link
Contributor Author

@come-nc come-nc Apr 4, 2023

Choose a reason for hiding this comment

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

Yes, the only place it was used was a test.

@icewind1991
Copy link
Member

icewind1991 commented Apr 4, 2023

@icewind1991 https://github.com/nextcloud/server/blob/master/apps/dav/lib/Connector/Sabre/Directory.php#L139-L141 this is blowing up because it puts null in non-nullable parameters. Any idea how to refactor this properly?

probably makes most sense to change those nulls to empty strings. But it probably makes more sense to do a that in a separate PR

@provokateurin provokateurin removed their request for review April 4, 2023 16:13
blizzz
blizzz approved these changes Apr 4, 2023
View reviewed changes
lib/public/Files/FileInfo.php
* Get the owner of the file
*
* @return \OCP\IUser
* @return ?\OCP\IUser
Copy link
Member

@blizzz blizzz Apr 4, 2023

Choose a reason for hiding this comment

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

should be added to critical changes (even if not a de-facto change to raise awareness)

@come-nc come-nc added 3. to review Waiting for reviews pending documentation This pull request needs an associated documentation update and removed 2. developing Work in progress labels Apr 5, 2023
@come-nc come-nc merged commit 8f55039 into master Apr 5, 2023
@come-nc come-nc deleted the fix/view-type-cleanup branch April 5, 2023 08:14
@juliusknorr
Copy link
Member

juliusknorr commented Apr 5, 2023

Causing some test failures on office by stricter typing that might be related to version handling in server. First investigation in nextcloud/richdocuments#2896, to be continued tomorrow.

@DaphneMuller
Copy link

DaphneMuller commented May 30, 2023

Hello!
Thank you for your work on this! I noticed that your PR still has the label 'documentation'. I'm not sure if you already had a look at the documentation for your PR, but at Nextcloud we strive to document changes that affect other app developers or other admins before the release takes place, and I'm pinging you with the hope to get this done.

Are you familiar with our documentation process already?
Changes that affect app developers should be documented here:
https://docs.nextcloud.com/server/27/developer_manual/app_publishing_maintenance/app_upgrade_guide/upgrade_to_27.html

Changes that affect administrators should be documented here:
https://docs.nextcloud.com/server/latest/admin_manual/release_notes/upgrade_to_27.html

While I understand sometimes it's handy to merge fast to get your changes in, it would be great if next time you could try to add the documentation before merging. You can find more information on that here:
https://docs.nextcloud.com/server/27/developer_manual/prologue/compatibility_app_ecosystem.html#documentation-procedures-of-changes-that-affect-app-developers

If all your documentation efforts are done, please remove the label 'documentation' and check the checkbox in your opening note and I'll stop bugging you :)

Many thanks in advance and thanks again for your work!

@come-nc come-nc mentioned this pull request May 30, 2023
Merged
@come-nc come-nc removed the pending documentation This pull request needs an associated documentation update label Jun 1, 2023
@blizzz blizzz mentioned this pull request Jun 2, 2023
Closed
blizzz
blizzz reviewed Jun 2, 2023
View reviewed changes
lib/private/Files/Filesystem.php
* @param string $path
* @return string
*/
public static function getLocalFolder($path) {
Copy link
Member

@blizzz blizzz Jun 2, 2023

Choose a reason for hiding this comment

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

Almost. Used in https://github.com/nextcloud/workflow_script/blob/master/lib/Operation.php#L200

@blizzz blizzz mentioned this pull request Mar 21, 2024
Merged
@artonge artonge mentioned this pull request Jul 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChristophWurst ChristophWurst ChristophWurst left review comments

@blizzz blizzz blizzz approved these changes

@icewind1991 icewind1991 icewind1991 approved these changes

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl is a code owner automatically assigned from nextcloud/server-backend

Assignees

@come-nc come-nc

Labels

3. to review Waiting for reviews

Projects

None yet

Milestone

Nextcloud 27

Development

Successfully merging this pull request may close these issues.

7 participants

@come-nc @icewind1991 @juliusknorr @DaphneMuller @ChristophWurst @blizzz