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.

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

refactor(apps): Use PHP 5.5 features #48362

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

refactor(apps): Use PHP 5.5 features #48362

wants to merge 1 commit into from

Conversation

provokateurin
Copy link
Member

Summary

Setting rector PHP level to 5.3 and 5.4 did not lead to any changes so we can start at 5.5 right away.

Checklist

@provokateurin
refactor(apps): Use PHP 5.5 features
b9115da 
Signed-off-by: provokateurin <kate@provokateurin.de>
@provokateurin provokateurin added technical debt ♻️ refactor Refactor code (not a bug fix, not a feature just refactoring) labels Sep 26, 2024
@provokateurin provokateurin added this to the Nextcloud 31 milestone Sep 26, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 26, 2024
View reviewed changes
apps/admin_audit/lib/AppInfo/Application.php
@@ -232,6 +232,6 @@
private function trashbinHooks(IAuditLogger $logger): void {
$trashActions = new Trashbin($logger);
Util::connectHook('\OCP\Trashbin', 'preDelete', $trashActions, 'delete');
Util::connectHook('\OCA\Files_Trashbin\Trashbin', 'post_restore', $trashActions, 'restore');
Util::connectHook(\OCA\Files_Trashbin\Trashbin::class, 'post_restore', $trashActions, 'restore');

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OCP\Util::connectHook has been marked as deprecated
apps/files/lib/AppInfo/Application.php
@@ -147,6 +147,6 @@
}

private function registerHooks(): void {
Util::connectHook('\OCP\Config', 'js', '\OCA\Files\App', 'extendJsConfig');
Util::connectHook('\OCP\Config', 'js', \OCA\Files\App::class, 'extendJsConfig');

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OCP\Util::connectHook has been marked as deprecated
apps/files_external/lib/Command/Scan.php
@@ -70,13 +70,13 @@
/** @var Scanner $scanner */
$scanner = $storage->getScanner();

$scanner->listen('\OC\Files\Cache\Scanner', 'scanFile', function (string $path) use ($output): void {
$scanner->listen(\OC\Files\Cache\Scanner::class, 'scanFile', function (string $path) use ($output): void {

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OC\Hooks\EmitterTrait::listen has been marked as deprecated
apps/files_external/lib/Command/Scan.php
$output->writeln("\tFile\t<info>$path</info>", OutputInterface::VERBOSITY_VERBOSE);
++$this->filesCounter;
$this->abortIfInterrupted();
});

$scanner->listen('\OC\Files\Cache\Scanner', 'scanFolder', function (string $path) use ($output): void {
$scanner->listen(\OC\Files\Cache\Scanner::class, 'scanFolder', function (string $path) use ($output): void {

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OC\Hooks\EmitterTrait::listen has been marked as deprecated
apps/files_sharing/lib/Controller/DeletedShareAPIController.php
@@ -226,7 +226,7 @@
throw new QueryException();
}

return $this->serverContainer->get('\OCA\Talk\Share\Helper\DeletedShareAPIController');
return $this->serverContainer->get(\OCA\Talk\Share\Helper\DeletedShareAPIController::class);

Check failure

Code scanning / Psalm

UndefinedClass Error

Class, interface or enum named OCA\Talk\Share\Helper\DeletedShareAPIController does not exist
apps/files_sharing/lib/Helper.php
@@ -12,10 +12,10 @@

class Helper {
public static function registerHooks() {
\OCP\Util::connectHook('OC_Filesystem', 'post_rename', '\OCA\Files_Sharing\Updater', 'renameHook');
\OCP\Util::connectHook('OC_Filesystem', 'post_delete', '\OCA\Files_Sharing\Hooks', 'unshareChildren');
\OCP\Util::connectHook('OC_Filesystem', 'post_rename', \OCA\Files_Sharing\Updater::class, 'renameHook');

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OCP\Util::connectHook has been marked as deprecated
apps/files_sharing/lib/Helper.php
\OCP\Util::connectHook('OC_Filesystem', 'post_rename', '\OCA\Files_Sharing\Updater', 'renameHook');
\OCP\Util::connectHook('OC_Filesystem', 'post_delete', '\OCA\Files_Sharing\Hooks', 'unshareChildren');
\OCP\Util::connectHook('OC_Filesystem', 'post_rename', \OCA\Files_Sharing\Updater::class, 'renameHook');
\OCP\Util::connectHook('OC_Filesystem', 'post_delete', \OCA\Files_Sharing\Hooks::class, 'unshareChildren');

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OCP\Util::connectHook has been marked as deprecated
apps/files_sharing/lib/Helper.php

\OCP\Util::connectHook('OC_User', 'post_deleteUser', '\OCA\Files_Sharing\Hooks', 'deleteUser');
\OCP\Util::connectHook('OC_User', 'post_deleteUser', \OCA\Files_Sharing\Hooks::class, 'deleteUser');

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OCP\Util::connectHook has been marked as deprecated
apps/files_trashbin/lib/Command/RestoreAllFiles.php
@@ -256,7 +256,7 @@
* That's why PSALM doesn't know the class GroupTrashItem.
* @psalm-suppress RedundantCondition
*/
if ($scope === self::SCOPE_GROUPFOLDERS && $trashItemClass !== 'OCA\GroupFolders\Trash\GroupTrashItem') {
if ($scope === self::SCOPE_GROUPFOLDERS && $trashItemClass !== \OCA\GroupFolders\Trash\GroupTrashItem::class) {

Check failure

Code scanning / Psalm

UndefinedClass Error

Class, interface or enum named OCA\GroupFolders\Trash\GroupTrashItem does not exist
apps/files_trashbin/lib/Trashbin.php
@@ -505,7 +505,7 @@
$view->chroot('/' . $user . '/files');
$view->touch('/' . $location . '/' . $uniqueFilename, $mtime);
$view->chroot($fakeRoot);
\OCP\Util::emitHook('\OCA\Files_Trashbin\Trashbin', 'post_restore', ['filePath' => $targetPath, 'trashPath' => $sourcePath]);
\OCP\Util::emitHook(\OCA\Files_Trashbin\Trashbin::class, 'post_restore', ['filePath' => $targetPath, 'trashPath' => $sourcePath]);

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OCP\Util::emitHook has been marked as deprecated
nickvergessen
nickvergessen requested changes Sep 26, 2024
View reviewed changes
apps/admin_audit/lib/AppInfo/Application.php
@@ -232,6 +232,6 @@ private function versionsHooks(IAuditLogger $logger): void {
private function trashbinHooks(IAuditLogger $logger): void {
$trashActions = new Trashbin($logger);
Util::connectHook('\OCP\Trashbin', 'preDelete', $trashActions, 'delete');
Util::connectHook('\OCA\Files_Trashbin\Trashbin', 'post_restore', $trashActions, 'restore');
Util::connectHook(\OCA\Files_Trashbin\Trashbin::class, 'post_restore', $trashActions, 'restore');
Copy link
Member

Choose a reason for hiding this comment

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

This was created by Christoph the other day.
It is a breaking change:

var_dump('\OCA\Files_Trashbin\Trashbin' === \OCA\Files_Trashbin\Trashbin::class);
// bool(false)

We can only do this, if we modify the hooker itself and basically trim leading slashes on connect and emit

Copy link
Member Author

Choose a reason for hiding this comment

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

So the comparison fails, but shouldn't the ::class result in the exact same string content?
Maybe I'm missing something here.

Copy link
Contributor

Choose a reason for hiding this comment

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

\OCA\Files_Trashbin\Trashbin::class is 'OCA\Files_Trashbin\Trashbin' without the first \

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, in the preview IntelliJ displayed it to me with the leading backslash 🤷‍♀️

@provokateurin provokateurin added the 3. to review Waiting for reviews label Sep 26, 2024
@provokateurin
Copy link
Member Author

Another problem as Psalm reported it: Class strings for classes from apps outside of server don't work. Using stubs for that should be fine, right? And there shouldn't be any problem with loading that code even if the class is not available?

@come-nc
Copy link
Contributor

come-nc commented Sep 26, 2024

Another problem as Psalm reported it: Class strings for classes from apps outside of server don't work. Using stubs for that should be fine, right? And there shouldn't be any problem with loading that code even if the class is not available?

Yeah psalm is annoying with that. It’s perfectly valid to use \My\Fully\Qualified\Name::class even if the class does not exists, it still works.

@come-nc
Copy link
Contributor

come-nc commented Sep 26, 2024

The only problematic place with starting / should be connectHook, which we should get rid of anyway.
So maybe we first migrate away from hooks in apps before applying this?

@provokateurin
Copy link
Member Author

So maybe we first migrate away from hooks in apps before applying this?

Sounds good 👍

@provokateurin provokateurin marked this pull request as draft September 26, 2024 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen requested changes

@come-nc come-nc Awaiting requested review from come-nc

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst ChristophWurst is a code owner

@miaulalala miaulalala Awaiting requested review from miaulalala miaulalala is a code owner

@tcitworld tcitworld Awaiting requested review from tcitworld tcitworld is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
3. to review Waiting for reviews ♻️ refactor Refactor code (not a bug fix, not a feature just refactoring) technical debt
Projects
None yet
Milestone
Nextcloud 31
Development

Successfully merging this pull request may close these issues.
3 participants
@provokateurin @come-nc @nickvergessen