Skip to content

Conversation

@skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Apr 24, 2025

@skjnldsv skjnldsv force-pushed the feat/prevent-duplicate-mounts branch 4 times, most recently from 495c451 to 335747c Compare April 24, 2025 14:06
@skjnldsv skjnldsv force-pushed the feat/prevent-duplicate-mounts branch from 335747c to b26826c Compare April 24, 2025 14:12
@skjnldsv skjnldsv marked this pull request as ready for review April 24, 2025 14:12
@skjnldsv skjnldsv self-assigned this Apr 24, 2025
@skjnldsv skjnldsv added bug 3. to review Items that need to be reviewed labels Apr 24, 2025
@icewind1991
Copy link
Member

Do we actually want to do this? there are legitimate use cases for it and blocking it now might break existing workflows

@skjnldsv skjnldsv force-pushed the feat/prevent-duplicate-mounts branch from aa7960d to 2a06a69 Compare May 16, 2025 07:47
@skjnldsv

This comment was marked as resolved.

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
@skjnldsv skjnldsv force-pushed the feat/prevent-duplicate-mounts branch from 2a06a69 to a41aaa5 Compare May 16, 2025 07:55
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.

I'm predicting the need to add a config flag for this eventually, but for the majority of users this should be an improvement.

Comment on lines +180 to +184
$storageId = $this->getRootFolderStorageId();
if ($storageId === null) {
throw new OCSNotFoundException('Groupfolder not found');
}

Copy link
Member

Choose a reason for hiding this comment

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

why is this here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because psalm isn't smart enough to understand the check above 🤦

Copy link
Member

@provokateurin provokateurin May 20, 2025

Choose a reason for hiding this comment

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

Then you should rather suppress it than bending your code for psalm to be happy (unless your code is actually bad).

Copy link
Member Author

Choose a reason for hiding this comment

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

With something like @var ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah or just @psalm-supress

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if really cleaner, but here it is: #3802

@skjnldsv skjnldsv merged commit 2b3d925 into master May 20, 2025
50 checks passed
@skjnldsv skjnldsv deleted the feat/prevent-duplicate-mounts branch May 20, 2025 13:02
@skjnldsv skjnldsv mentioned this pull request Aug 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Items that need to be reviewed bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants