Refactor files trashbin app by danialRahimy · Pull Request #39883 · nextcloud/server

danialRahimy · 2023-08-15T12:20:21Z

Summary

I did these items in a single file apps/files_trashbin/lib/Trashbin.php for two reasons:

Pull request hasn't been a lot of changes.
I was not sure about some changes.

Return early
Change the const name for more readability
Add functions' return type
Update doc types

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)

+ if ($owner !== $user) {
+ self::copy_recursive($owner . '/files_versions/' . $ownerPath, $owner . '/files_trashbin/versions/' . static::getTrashFilename(basename($ownerPath), $timestamp), $rootView);
+ }
+ self::move($rootView, $owner . '/files_versions/' . $ownerPath, $user . '/files_trashbin/versions/' . static::getTrashFilename($filename, $timestamp));


- self::move($rootView, $owner . '/files_versions' . $v['path'] . '.v' . $v['version'], $user . '/files_trashbin/versions/' . static::getTrashFilename($filename . '.v' . $v['version'], $timestamp));
+ self::copy($rootView, $owner . '/files_versions' . $v['path'] . '.v' . $v['version'], $owner . '/files_trashbin/versions/' . static::getTrashFilename($v['name'] . '.v' . $v['version'], $timestamp));
 }
+ self::move($rootView, $owner . '/files_versions' . $v['path'] . '.v' . $v['version'], $user . '/files_trashbin/versions/' . static::getTrashFilename($filename . '.v' . $v['version'], $timestamp));


+ }
+
+ if ($view->is_dir('/files_trashbin/versions/' . $file)) {
+ $rootView->rename(Filesystem::normalizePath($user . '/files_trashbin/versions/' . $file), Filesystem::normalizePath($owner . '/files_versions/' . $ownerPath));


+
+ if ($view->is_dir('/files_trashbin/versions/' . $file)) {
+ $rootView->rename(Filesystem::normalizePath($user . '/files_trashbin/versions/' . $file), Filesystem::normalizePath($owner . '/files_versions/' . $ownerPath));
+ } elseif ($versions = self::getVersionsFromTrash($versionedFile, $timestamp, $user)) {


+ } elseif ($versions = self::getVersionsFromTrash($versionedFile, $timestamp, $user)) {
+ foreach ($versions as $v) {
+ if ($timestamp) {
+ $rootView->rename($user . '/files_trashbin/versions/' . static::getTrashFilename($versionedFile . '.v' . $v, $timestamp), $owner . '/files_versions/' . $ownerPath . '.v' . $v);


+ if ($timestamp) {
+ $rootView->rename($user . '/files_trashbin/versions/' . static::getTrashFilename($versionedFile . '.v' . $v, $timestamp), $owner . '/files_versions/' . $ownerPath . '.v' . $v);
+ } else {
+ $rootView->rename($user . '/files_trashbin/versions/' . $versionedFile . '.v' . $v, $owner . '/files_versions/' . $ownerPath . '.v' . $v);


+ } else {
+ $size += $view->filesize('/files_trashbin/versions/' . $filename . '.v' . $v);
+ $view->unlink('/files_trashbin/versions/' . $filename . '.v' . $v);
 }


Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>

…e psalm analysis Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>

…ption on doctype Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>

nfebe · 2023-08-19T10:51:19Z

+ * @return bool|string original location
 */
-	public static function getLocation($user, $filename, $timestamp) {
+	public static function getLocation($user, $filename, $timestamp): bool|string {


Suggested change
publicstaticfunction getLocation($user, $filename, $timestamp): bool|string {
publicstaticfunction getLocation(string$user, string$filename,string$timestamp): bool|string {

nfebe

Since this is just a refactor and no main functional change you can just use build in type hinting for PHP... I left a few examples but not all, most of the functions can use built in type hinting and that would allow you to delete the comment annotations at the top (leaving behind just the exceptions)

Co-authored-by: F. E Noel Nfebe <fenn25.fn@gmail.com> Signed-off-by: danial rahimy <48244647+danialRahimy@users.noreply.github.com>

danialRahimy force-pushed the app_files_trashbin_refactor branch 2 times, most recently from 9d23571 to 24de858Compare August 15, 2023 12:35

github-advanced-securityAI found potential problems Aug 15, 2023
View reviewed changes

szaimen added 3. to review Waiting for reviews technical debt 🧱 🤔🚀 labels Aug 15, 2023

szaimen added this to the Nextcloud 28 milestone Aug 15, 2023

szaimen requested review from a team, blizzz, icewind1991 and nfebe and removed request for a teamAugust 15, 2023 12:40

danialRahimy added 7 commits August 16, 2023 08:54

Files trashbin app: Return early in method retainVersions
558c146
Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>

Files trashbin app: Change the const name for more readability
81c4780
Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>

Files trashbin app: Adding functions' return type
2c298c0
Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>

Files trashbin app: Return early
36338e8
Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>

Files trashbin app: Remove extra spaces between methods
dfceb76
Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>

Files trashbin app: Update doc types
a531255
Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>

Files trashbin app: Add remaining return type.
82dd6fa
Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>

danialRahimy force-pushed the app_files_trashbin_refactor branch from 24de858 to 82dd6faCompare August 16, 2023 05:24

Files trashbin app: Add parameter type to the methods according to th…
8bd6a2f
…e psalm analysis Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>

nickvergessen reviewed Aug 16, 2023
View reviewed changes