add test for . in path with no extension, fix smb #26143

butonic · 2016-09-19T10:01:44Z

Description

No longer try to append the original file extension to a temp file used when uploading to an SMB storage.
Implement recursive mkdir
Add unit test.

Related Issue

#25994 (comment)

Motivation and Context

removes unnecessary code which might lead to problems

How Has This Been Tested?

new unit test

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

mention-bot · 2016-09-19T10:01:45Z

@butonic, thanks for your PR! By analyzing the annotation information on this pull request, we identified @icewind1991, @blizzz, @PVince81 and @Xenopathic to be potential reviewers

butonic · 2016-09-19T10:01:59Z

cc @VicDeo

butonic · 2016-09-19T10:02:19Z

backport to 9.1 needed

PVince81 · 2016-09-19T10:04:12Z

@butonic don't forget to mention the other SMB master @jvillafanez

PVince81 · 2016-09-19T10:06:02Z

might lead to problems

Was about to ask about severity, seems this didn't cause any issues.

Still 👍 for removing this unnecessary logic

jvillafanez · 2016-09-20T09:38:00Z

apps/files_external/lib/Lib/Storage/SMB.php

@@ -517,9 +512,18 @@ public function filetype($path) {
 public function mkdir($path) {
 $this->log('enter: '.__FUNCTION__."($path)");
 $result = false;
- $path = $this->buildPath($path);
+ $dirs = explode('/', $path);


How do the rest of the implementations work? Are the implementations expected to create the directories recursively?

The API says it should be recursive: https://github.com/owncloud/core/blob/master/lib/public/Files/Storage/IStorage.php#L66

I tried using a recursive mkdir ... that at least also breaks webdav ... looking at the other implementations nearly none does it recursively.

will send a separate PR for that. for now I'll use 5 mkdirs ... jeez

#7324 oh boy

jvillafanez · 2016-09-20T10:15:37Z

We'll have to check if the mimetypes are properly sent to the clients. I think this was set to help mimetype detection.

butonic · 2016-09-20T13:27:29Z

@jvillafanez hm:

14:00:36 There were 2 failures:
14:00:36 
14:00:36 1) OCA\Files_Sharing\Tests\CacheTest::testSearchByMime
14:00:36 Failed asserting that 4 matches expected 3.
14:00:36 
14:00:36 /var/lib/jenkins/workspace/owncloud-core/core/PR-26143/apps/files_sharing/tests/CacheTest.php:444
14:00:36 /var/lib/jenkins/workspace/owncloud-core/core/PR-26143/apps/files_sharing/tests/CacheTest.php:231
14:00:36 
14:00:36 2) OCA\Files_Sharing\Tests\CacheTest::testGetFolderContentsWhenSubSubdirShared
14:00:36 Failed asserting that two strings are equal.
14:00:36 --- Expected
14:00:36 +++ Actual
14:00:36 @@ @@
14:00:36 -'application/xml'
14:00:36 +'text/plain'
14:00:36 
14:00:36 /var/lib/jenkins/workspace/owncloud-core/core/PR-26143/apps/files_sharing/tests/CacheTest.php:465
14:00:36 /var/lib/jenkins/workspace/owncloud-core/core/PR-26143/apps/files_sharing/tests/CacheTest.php:449
14:00:36 /var/lib/jenkins/workspace/owncloud-core/core/PR-26143/apps/files_sharing/tests/CacheTest.php:426

investigating ...

jvillafanez · 2016-09-20T15:02:47Z

apps/files_external/lib/Lib/Storage/SMB.php

@@ -413,6 +408,7 @@ public function fopen($path, $mode) {
 if (!$this->isCreatable(dirname($path))) {
 break;
 }
+ $ext = pathinfo($path, PATHINFO_EXTENSION);


My trust level in these kind of functions is usually low. I've checked http://php.net/manual/en/function.pathinfo.php and I'm not sure if this is what we really want here...

pathinfo actually implements the extension extraction correctly. In contrast to the php mess used before.

we may need additional handling because we have different mimetypes for .tar.bz2/.bz2 and .tar.gz/.gz, see

core/resources/config/mimetypemapping.dist.json

Line 152 in fffcb79

"tar.bz2": ["application/x-bzip2"],

If the basename of the path starts with a dot, the following characters are interpreted as extension, and the filename is empty

This is what worries me, in addition to the .tar.bz2 problem.

I'll add a test for '/path/to a/hidden/.file', '/path/to a/hidden/.file.php', '/path/to a/hidden/.file.gz' and '/path/to a/hidden/.file.tar.gz' to https://github.com/owncloud/core/pull/26143/files#diff-5513e31c79d843ab51b3af2a528af6ddR138

@jvillafanez pathinfo treats .files correctly:
pathinfo('.hidden')

Array ( [dirname] => . [basename] => .hidden [extension] => hidden [filename] => )

pathinfo('.hidden.gz')

Array ( [dirname] => . [basename] => .hidden.gz [extension] => gz [filename] => .hidden )

perfectly fine IMO.

Am I the only one who think that for the ".hidden" file the filename should be ".hidden" and the extension should be empty? If I'm the only one, then let's move on.

butonic · 2016-09-20T21:45:23Z

@PVince81 @VicDeo @jvillafanez jenkins is green.

additional handling for .tar.bz2/.bz2 and .tar.gz/.gz: add test for . in path with no extension, fix smb #26143 (comment)
~~ignore . at beginning of basename for hidden files~~ tests show that pathinfo treats .hidden and .hidden.txt correctly
submit separate PR to test and implement recursive mkdir
backport to stable9.1
backport to stable9.0

jvillafanez · 2016-09-22T09:36:40Z

lib/public/Files.php

+ * @return array|string|null
+ * @since 9.2.0
+ */
+ static function pathinfo( $path, $options = PATHINFO_DIRNAME | PATHINFO_BASENAME | PATHINFO_EXTENSION | PATHINFO_FILENAME ) {


Is it possible to simplify this code? Scrutinizer reports a complexity value of 17 for this function, which is pretty high: https://scrutinizer-ci.com/g/owncloud/core/inspections/a866f5ac-174c-4d40-af68-2a73fc3059f1/code-structure/class/OCP%5CFiles

At least we have tests for this function, but this is a black box for me...

Could you simplify this? Maybe:

if ($path endswith 'tar.gz') { $result = handleTarGzPaths($path); } else if ($path endswith 'tar.bz2') { $result = handleTarBz2Paths($path) } else { $result = pathinfo($path); }

Whether those "handle" function are public or private is just to be decided, but at least the complexity of this function is reduced and it should also be easier to extend with other custom extensions such as ".tar.gzip", ".jra.gz" or ".hbm.xml" which we might want to add at some point.

Maybe we should consider to introduce a "ExtensionHandling" class or similar for these kind of functions.

I absolutly agree on moving this to a class on its own - we don't want static APIs in OCP and ocp files shall die.

butonic · 2016-10-14T12:10:06Z

@jvillafanez @PVince81 @PhilippSchaffrath jenkins is green after a rebase again

PVince81 · 2016-12-06T17:57:35Z

@jvillafanez @DeepDiver1975 rereview ? Not sure if your points were addressed though.

jvillafanez · 2016-12-07T14:22:30Z

Except for my comment, the rest of the code is fine.

PVince81 · 2017-04-06T14:59:36Z

finish this or defer ?

jvillafanez · 2017-04-07T10:47:48Z

I'm not sure if we have tested with different locales (http://php.net/manual/es/function.pathinfo.php#119468). If not I'd rather defer this. Code should be fine otherwise.

mmattel · 2017-07-13T11:02:14Z

Any update on this?

jvillafanez · 2017-07-17T09:28:25Z

I'd wait for @butonic status report on this.

PVince81 · 2018-02-15T16:04:14Z

any update ?

PVince81 · 2019-02-08T14:50:23Z

@jvillafanez @butonic please evaluate if investing time in this provides enough value

I'd vote for closing unless there are strong arguments for keeping this

butonic · 2019-02-08T15:03:06Z

we should probably move the test cases to an acceptance test that we can also use with phoenix and OCIS

jvillafanez · 2019-02-11T08:45:19Z

If we want to keep this I'd prefer to make a new PR based on this one. This way we don't need to deal with the conflicts.
There are also a couple of changes I'd do:

Create a IPathOperator class (name to be agreed on) to handle path operations.
Create the implementation. The base would be the OCP\Files::pathinfo, and might include some other functions (thinking about the Filesystem::normalizePath).

In any case, fixing all those conflicts will be a pain, so I don't think it's a good idea.

PVince81 · 2019-02-11T14:57:29Z

raised #34442 for acceptance test cases

PVince81 · 2019-02-11T14:59:58Z

I can't find the original comment/reason for this PR as the link in OP doesn't work.

If this was important, please re-raise as new ticket.

butonic added Type:Bug 3 - To Review app:files_external labels Sep 19, 2016

butonic added this to the 9.2 milestone Sep 19, 2016

butonic mentioned this pull request Sep 19, 2016

[stable9.1] add test for . in path with no extension, fix smb #26146

Closed

jvillafanez reviewed Sep 20, 2016

View reviewed changes

butonic force-pushed the extension-fixes branch from cdd21dd to 5d85a8b Compare September 20, 2016 10:52

butonic force-pushed the extension-fixes branch from 5d85a8b to 1e7af6d Compare September 20, 2016 13:58

jvillafanez reviewed Sep 20, 2016

View reviewed changes

butonic added 2 - Developing and removed 3 - To Review labels Sep 20, 2016

butonic force-pushed the extension-fixes branch 3 times, most recently from 2e021f8 to d59b6af Compare September 21, 2016 21:36

jvillafanez reviewed Sep 22, 2016

View reviewed changes

DeepDiver1975 force-pushed the extension-fixes branch from e9968f7 to 7d11107 Compare September 29, 2016 07:33

butonic added 4 commits October 14, 2016 12:02

add test for . in path with no extension, fix smb

134be85

don't try to append file extension

577636e

fix broken extension, make pathinfo handle .tar.(gz|bz2)

1950640

test dot files

7599680

butonic force-pushed the extension-fixes branch from 7d11107 to 7599680 Compare October 14, 2016 10:11

butonic added the 3 - To Review label Oct 14, 2016

butonic removed the 2 - Developing label Oct 14, 2016

PVince81 modified the milestones: triage, 10.0 May 17, 2017

ownclouders added the status/STALE label Dec 7, 2017

ownclouders removed the status/STALE label Feb 15, 2018

ownclouders added the status/STALE label Feb 23, 2018

PVince81 modified the milestones: triage, maybe some day May 22, 2018

PVince81 assigned jvillafanez Feb 8, 2019

PVince81 modified the milestones: maybe some day, backlog Feb 8, 2019

ownclouders removed the status/STALE label Feb 8, 2019

PVince81 mentioned this pull request Feb 11, 2019

Acceptance tests for dots in path #34442

Closed

PVince81 closed this Feb 11, 2019

PVince81 deleted the extension-fixes branch February 11, 2019 15:00

lock bot locked as resolved and limited conversation to collaborators Feb 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add test for . in path with no extension, fix smb #26143

add test for . in path with no extension, fix smb #26143

butonic commented Sep 19, 2016 •

edited

Loading

mention-bot commented Sep 19, 2016

butonic commented Sep 19, 2016

butonic commented Sep 19, 2016

PVince81 commented Sep 19, 2016

PVince81 commented Sep 19, 2016

jvillafanez Sep 20, 2016

butonic Sep 20, 2016

butonic Sep 20, 2016

jvillafanez commented Sep 20, 2016

butonic commented Sep 20, 2016

jvillafanez Sep 20, 2016

butonic Sep 20, 2016

butonic Sep 20, 2016

jvillafanez Sep 21, 2016

butonic Sep 22, 2016

butonic Sep 22, 2016 •

edited

Loading

jvillafanez Sep 22, 2016

butonic commented Sep 20, 2016 •

edited

Loading

jvillafanez Sep 22, 2016

jvillafanez Oct 19, 2016

DeepDiver1975 Nov 21, 2016

butonic commented Oct 14, 2016

PVince81 commented Dec 6, 2016

jvillafanez commented Dec 7, 2016

PVince81 commented Apr 6, 2017

jvillafanez commented Apr 7, 2017

mmattel commented Jul 13, 2017

jvillafanez commented Jul 17, 2017

PVince81 commented Feb 15, 2018

PVince81 commented Feb 8, 2019

butonic commented Feb 8, 2019

jvillafanez commented Feb 11, 2019

PVince81 commented Feb 11, 2019

PVince81 commented Feb 11, 2019

add test for . in path with no extension, fix smb #26143

add test for . in path with no extension, fix smb #26143

Conversation

butonic commented Sep 19, 2016 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

mention-bot commented Sep 19, 2016

butonic commented Sep 19, 2016

butonic commented Sep 19, 2016

PVince81 commented Sep 19, 2016

PVince81 commented Sep 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvillafanez commented Sep 20, 2016

butonic commented Sep 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

butonic Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

butonic commented Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

butonic commented Oct 14, 2016

PVince81 commented Dec 6, 2016

jvillafanez commented Dec 7, 2016

PVince81 commented Apr 6, 2017

jvillafanez commented Apr 7, 2017

mmattel commented Jul 13, 2017

jvillafanez commented Jul 17, 2017

PVince81 commented Feb 15, 2018

PVince81 commented Feb 8, 2019

butonic commented Feb 8, 2019

jvillafanez commented Feb 11, 2019

PVince81 commented Feb 11, 2019

PVince81 commented Feb 11, 2019

butonic commented Sep 19, 2016 •

edited

Loading

butonic Sep 22, 2016 •

edited

Loading

butonic commented Sep 20, 2016 •

edited

Loading