Skip to content

Conversation

@juliusknorr
Copy link
Member

To reproduce, mount an SMB storage with a path like Path\To\Subdirectory and try to run the files_external:notify command. The self-test will fail as the root and change path in

} elseif (substr($fullPath, 0, strlen($this->root)) === $this->root) {
won't match due to the wrong path separator being used.

The change object itself already uses UNIX path separators so this makes sure we have a normalized root as well.

I've recognized when testing that using Path/To/Subdirectory as subdirectory in the external storage config solves the issue as well, however since entering backslash separated paths there is supported i feel this patch solves causing confusion there for admins who might be used to the Windows path separators.

@juliusknorr juliusknorr added bug 3. to review Waiting for reviews labels Jan 14, 2021
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.

Might be worth it to just change \ into / in the smb configuration.

@juliusknorr
Copy link
Member Author

Might be worth it to just change \ into / in the smb configuration.

Yep that fixed it as well, mainly suggested the change because i can see people who are more used to Windows path separators that might run into this more often ;)

@rullzer
Copy link
Member

rullzer commented Jan 27, 2021

So go or no go?

@PVince81
Copy link
Member

@icewind1991 merge or block ?

@PVince81 PVince81 added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Mar 19, 2021
@PVince81 PVince81 merged commit e6fd9b2 into master Mar 19, 2021
@PVince81 PVince81 deleted the bugfix/smb-notify-subpath branch March 19, 2021 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release Ready to be released and/or waiting for tests to finish bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants