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

Uses IShare::TYPE_... in Server.php rather than the obsolete Constants #32647

Closed
wants to merge 1 commit into from

Conversation

StCyr
Copy link
Contributor

@StCyr StCyr commented May 29, 2022

It doesn't look like those search plugins are used somewhere else....

hmmm.... Here might be another attention point though:

\OC::$server->getCollaboratorSearch()->registerPlugin($pluginInfo);

Signed-off-by: Cyrille Bollu [email protected]

@StCyr StCyr requested review from PVince81, blizzz and juliusknorr May 29, 2022 17:31
@nickvergessen
Copy link
Member

Here might be another attention point though:

Any app can register a provider:
https://github.com/nextcloud/spreed/blob/master/appinfo/info.xml#L129

@CarlSchwan
Copy link
Member

Here might be another attention point though:

Any app can register a provider: https://github.com/nextcloud/spreed/blob/master/appinfo/info.xml#L129

Yeah so would be good to double check if external apps like talk and circle still work

@StCyr
Copy link
Contributor Author

StCyr commented May 30, 2022

Here might be another attention point though:

Any app can register a provider: https://github.com/nextcloud/spreed/blob/master/appinfo/info.xml#L129

Yeah so would be good to double check if external apps like talk and circle still work

Indeed.

IIUC, plugins registered via an app are processed via server/lib/private/legacy/OC_App.php and this won't work anymore with my change.

Should I just add a test in lib/private/Collaboration/Collaborators/Search.php?

	public function registerPlugin(array $pluginInfo) {
---		$shareType = constant(Share::class . '::' . $pluginInfo['shareType']);
+++	if (str_starts_with($pluginInfo['shareType'], 'SHARE_TYPE') {
+++              $shareType = constant(Share::class . '::' . $pluginInfo['shareType']);
+++	else {
+++              $shareType = $pluginInfo['shareType'];
+++         }
		if ($shareType === null) {
			throw new \InvalidArgumentException('Provided ShareType is invalid');
		}

and create issues in identified apps?

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Blocking merge until apps have been fixed

@@ -106,7 +106,7 @@ public function search($search, array $shareTypes, $lookup, $limit, $offset) {
}

public function registerPlugin(array $pluginInfo) {
$shareType = constant(Share::class . '::' . $pluginInfo['shareType']);
$shareType = $pluginInfo['shareType'];
Copy link
Member

Choose a reason for hiding this comment

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

So your proposal is:

Suggested change
$shareType = $pluginInfo['shareType'];
if (str_starts_with($pluginInfo['shareType'], 'SHARE_TYPE') {
$shareType = constant(Share::class . '::' . $pluginInfo['shareType']);
} else {
$shareType = $pluginInfo['shareType'];
}

Copy link
Member

Choose a reason for hiding this comment

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

How about:

Suggested change
$shareType = $pluginInfo['shareType'];
if (str_starts_with($pluginInfo['shareType'], 'SHARE_TYPE') {
$shareType = constant(IShare::class . '::' . substr($pluginInfo['shareType'], 6));
} else {
$shareType = $pluginInfo['shareType'];
}

This way we still get rid of the old constats?

Copy link
Member

Choose a reason for hiding this comment

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

Please also note that $shareType is a int after constant() but not with your code.
So should use the following on the else branch

		$shareType = (int) $pluginInfo['shareType'];

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

@StCyr Any update on this? Any chance you could open issues for the apps that need fixing? For backward compatiblity we could also go the str_starts_with approach that was commented already.

@szaimen szaimen added the 3. to review Waiting for reviews label Apr 13, 2023
@szaimen szaimen added this to the Nextcloud 27 milestone Apr 13, 2023
This was referenced May 3, 2023
@blizzz blizzz mentioned this pull request May 17, 2023
@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
This was referenced Mar 12, 2024
@skjnldsv
Copy link
Member

As there is no feedback since a while I will close this ticket.
If you will decide to work on this feature again and if it hasn't been fixed or implemented already, feel free to re-open and solve the various conflicts.

Thanks for the interest in Nextcloud and the effort put into this! 🙇

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 15, 2024
@skjnldsv skjnldsv closed this Mar 15, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 29 milestone Mar 15, 2024
@skjnldsv skjnldsv deleted the papercut/use-ishare-in-server branch March 15, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants