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

Dynamic share providers #31571

Closed
wants to merge 1 commit into from
Closed

Dynamic share providers #31571

wants to merge 1 commit into from

Conversation

michielbdejong
Copy link
Contributor

(still reviewing what the diff looks like, will mark as 'ready for review' soon)

Fixes #29215

@michielbdejong michielbdejong marked this pull request as draft March 14, 2022 14:09
@szaimen szaimen added the 3. to review Waiting for reviews label Mar 14, 2022
@szaimen szaimen added this to the Nextcloud 24 milestone Mar 14, 2022
@szaimen szaimen removed the 3. to review Waiting for reviews label Mar 14, 2022
@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
Merged
@blizzz blizzz mentioned this pull request Mar 31, 2022
Merged
@blizzz blizzz mentioned this pull request Apr 7, 2022
Merged
@blizzz blizzz mentioned this pull request Apr 13, 2022
Merged
@michielbdejong michielbdejong marked this pull request as ready for review April 19, 2022 10:27
@michielbdejong michielbdejong changed the title [WiP] Dynamic share provider Dynamic share provider Apr 19, 2022
@michielbdejong michielbdejong changed the title Dynamic share provider Dynamic share providers Apr 19, 2022
@michielbdejong michielbdejong force-pushed the dynamic-shareproviders branch 2 times, most recently from 1c94758 to be8e63b Compare April 19, 2022 10:34
@michielbdejong
Copy link
Contributor Author

I'll fix the failing unit tests and also add some more unit tests to cover the newly added code.

@michielbdejong michielbdejong force-pushed the dynamic-shareproviders branch 3 times, most recently from cf37ae8 to d23b15e Compare April 20, 2022 12:41
@blizzz blizzz modified the milestones: Nextcloud 24, Nextcloud 25 Apr 21, 2022
@michielbdejong
Copy link
Contributor Author

Can someone approve running the workflows please?

@michielbdejong
Copy link
Contributor Author

Thanks! Something new to fix. :)

@labkode
Copy link

labkode commented May 16, 2022

@michielbdejong any news on this?

@michielbdejong
Copy link
Contributor Author

@labkode yes sorry, we're working on getting all the actions green now, and after that we will request a review from 2 committers. We hope to be on time with this for Nextcloud 25.

@navid-dada The Lint / php-cs check (pull_request) action is still failing, can you have a look?

The PHPUnit / php8.1-oci (pull_request) action is saying phpunit is not installed, which sounds like it's unrelated to this PR, does someone know?

@ylebre @michielbdejong
Adding dynamic share providers.
d6d8429 
Signed-off-by: Michiel de Jong <michiel@unhosted.org>
@michielbdejong
Copy link
Contributor Author

12 workflows awaiting approval

@michielbdejong
Copy link
Contributor Author

Can someone approve running the workflows again please?

@michielbdejong
Copy link
Contributor Author

What is the next step now? How can we find 2 reviewers for this PR?

@michielbdejong
Copy link
Contributor Author

@blizzz can you help, or point us to the right person, maybe?

@labkode
Copy link

labkode commented Jun 13, 2022

@blizzz @skjnldsv can you please kindly provide a review for this PR?

PVince81
PVince81 reviewed Jul 11, 2022
View reviewed changes
lib/public/Share/IShare.php
/**
* @since ?
*/
public const TYPE_SCIENCEMESH = 1000;
Copy link
Member

Choose a reason for hiding this comment

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

it's not really a dynamic share type if app developers still need to add constants here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, true! We should move this into https://github.com/nextcloud/server/pull/31571/files#diff-dbbe017dd357504abc442a6f1d0305166520ebf80353f42814b3f879a3e241bcR168 probably. And then if two apps are installed that try to claim the same number, it should complain that you cannot have both those apps installed at the same time.

This was referenced Aug 12, 2022
Merged
Merged
This was referenced Aug 24, 2022
Merged
Merged
This was referenced Sep 6, 2022
Merged
Merged
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
Merged
This was referenced Sep 20, 2022
Closed
Merged
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Sep 22, 2022
@michielbdejong
Copy link
Contributor Author

Would it help if, instead of trying to create the dynamic share type registry for you, I change this PR so that it just adds "yet another" else statement for share type ScienceMesh to the lists in various places?

@michielbdejong michielbdejong mentioned this pull request Jan 11, 2023
Closed
@michielbdejong
Copy link
Contributor Author

Replaced by #36228

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PVince81 PVince81 PVince81 left review comments

Assignees
No one assigned
Labels
2. developing Work in progress enhancement
Projects
None yet
Milestone
Nextcloud 26
Development

Successfully merging this pull request may close these issues.

Dynamic share providers / share types in Nextcloud
7 participants
@michielbdejong @labkode @PVince81 @blizzz @skjnldsv @szaimen @ylebre