Mimetypedetector #15543

rullzer · 2015-04-10T17:41:55Z

As described in #4774 (comment)
Moving aways from the mimetype stuff in OC_Helper.

available unit tests are copied and modified.

It should not provide to much trouble as most functions were already wrappers around this.

rullzer · 2015-04-10T19:46:30Z

CC: @DeepDiver1975 @Xenopathic

RobinMcCorkell · 2015-04-10T20:40:30Z

tests/lib/files/type/detection.php

+
+	public function testGetSecureMimeType() {
+		$detection = new Detection();
+		$detection->registerTypeArray(include 'mimetypes.list.php');


mimetypes.list.php is used for custom mimetypes, so probably shouldn't be used here in the tests. Unless you want to try and tackle #15384 in this PR as well?

Ah you are right. I just made the old unit tests work again. I'll have a look later this week to make more proper unit tests.

MorrisJobke · 2015-07-02T22:00:07Z

@rullzer Still valid? Then please rebase ;)

rullzer · 2015-07-14T06:56:48Z

Rebased, moved all the mimetype related stuff to OC/Files/Type/Detection (OCP/Files/IMimetypeDetector).

Test still work. Altough I might extend them.
Old functions call the fancy new mimetypedetector. Just removing them is tricky since some (official) apps depend on them... even tough they are private.

Lets see what jenkins has to say.

RobinMcCorkell · 2015-07-14T12:33:26Z

lib/private/helper.php

@@ -496,9 +428,10 @@ static function getSecureMimeType($mimeType) {
 	 *
 	 * @param string $data
 	 * @return string
+	 * @deprecated Use \OC::$server->getMimeTypeDetector->detectString($data)


getMimeTypeDetector()

Thanks fixed

rullzer · 2015-07-14T20:06:52Z

@DeepDiver1975 this should fix #17421

As you can see I deprecated a lot of functions in OC_Helper. Now I know that we can usually just remove private functions but this was used in a lot of places since there was no OCP alternative. What to do in that case?

rullzer · 2015-07-14T20:07:00Z

Review time!

RobinMcCorkell · 2015-07-14T22:17:23Z

lib/private/files/type/detection.php

+
+		// Is it a dir?
+		if ($mimetype === 'dir') {
+			$this->mimetypeIcons[$mimetype] = \OC::$server->getURLGenerator()->imagePath('core', 'filetypes/folder.png');


Can we use dependency injection for the URL generator?

Yes, much cleaner.
Would also allow unit tests next to the integration tests

rullzer · 2015-07-15T19:04:28Z

@Xenopathic you are right. It looks cleaner now.

rullzer · 2015-07-15T19:04:36Z

Lets see if Jenkins is still happy

RobinMcCorkell · 2015-07-15T20:13:55Z

Looking good. Could you squash some of those commits though? We have 8 commits here, most of them fixes or adjustments of earlier ones.

rullzer · 2015-07-15T20:15:00Z

@Xenopathic yeah will do. Just waiting for Jenkins to be finished.

rullzer · 2015-07-16T05:23:31Z

Ok rebased and squashed... let see

rullzer · 2015-07-17T13:21:05Z

rebased... lets see if docker does its thing now.

RobinMcCorkell · 2015-07-18T19:29:33Z

👍 for the code, just need the whitespace and phpdoc fixes

rullzer · 2015-07-18T19:54:31Z

It seems unit testing the functions is not so trivial in the current setup (you can't set the vars to something you want easily since they read from a hard coded path). Let me think about how to solve that in a clean way.

RobinMcCorkell · 2015-07-18T23:04:30Z

@rullzer How about making the loadMappings() and loadAliases() functions public, and take a path/two paths to the JSON files. These functions then get called during instantiation in the server container, with the correct paths. In unit testing, we can call the functions with whatever paths we like, or not at all (defaults to empty array if not loaded, right?). Unfortunately you lose the deferred loading this way...

Alternatively, pass the config directory path to the constructor? That's how \OC\Config does it.

rullzer · 2015-07-19T05:47:22Z

@Xenopathic I was thinking about the latter one (so passing the config dir to the constructor). That way it is easily testable with a temp dir that is passed. And the full class can actually be properly unit tested.

I'll fix that hopefully this evening.

rullzer · 2015-07-20T10:30:30Z

Unit tests are added. But depend on owncloud-archive/3rdparty#185.

MorrisJobke · 2015-07-20T11:36:18Z

@rullzer Rebase needed

rullzer · 2015-07-21T10:24:07Z

@MorrisJobke will do once the 3rdparty is merged and bumbed on master.

MorrisJobke · 2015-07-21T10:53:30Z

@MorrisJobke will do once the 3rdparty is merged and bumbed on master.

Makes sense 😃

rullzer · 2015-07-26T20:01:18Z

tests/lib/files/type/detection.php

+	public function testMimeTypeIconStaticIcons() {
+		if (!class_exists('org\\bovigo\\vfs\\vfsStream')) {
+			$this->markTestSkipped('Pacakge vfsStream not installed');
+		}


Only run this test if the vfsStream pacakge is avaialble.. since the outcome of owncloud-archive/3rdparty#185 is unclear

rullzer · 2015-07-27T06:32:14Z

Rebased and added conditional to only run the test requiring vfsStream if it is available.

Review time!

rullzer · 2015-07-27T08:26:29Z

Jenkins is happy! #17889
Lets get this in!

Review time: @MorrisJobke @PVince81 @DeepDiver1975

@Xenopathic your 👍 is still valid I assume?

RobinMcCorkell · 2015-07-27T08:34:04Z

lib/private/helper.php

@@ -496,9 +428,10 @@ static function getSecureMimeType($mimeType) {
 	 *
 	 * @param string $data
 	 * @return string
+	 * @deprecated Use \OC::$server->getMimeTypeDetector()->detectString($data)


Missing deprecation version

Will add (right before merge) else jenkins migt die...

* Copied unit tests from old functions

In order to properly test the mimetype function: * constructor takes path to configdir * Added unit tests for mimetype (only if vfsStream is available)

scrutinizer-notifier · 2015-07-27T17:13:30Z

A new inspection was created.

ghost · 2015-07-27T18:41:27Z

🚀 Test PASSed.🚀

RobinMcCorkell · 2015-07-27T19:37:34Z

👍 Let's go! cc @MorrisJobke @PVince81

MorrisJobke · 2015-07-28T11:35:23Z

Code looks good and it still works 👍

Mimetypedetector

rullzer added 3 - To Review technical debt labels Apr 10, 2015

rullzer added this to the 8.2-next milestone Apr 10, 2015

RobinMcCorkell reviewed Apr 10, 2015
View reviewed changes

MorrisJobke added 2 - Developing and removed 3 - To Review labels Jul 2, 2015

This was referenced Jul 6, 2015

Missing public API \OC_Helper::mimetypeIcon #17421

Closed

Add theme support to mimetypeIcon through imagePath integration #15586

Merged

RobinMcCorkell reviewed Jul 14, 2015
View reviewed changes

rullzer added 3 - To Review and removed 2 - Developing labels Jul 14, 2015

RobinMcCorkell reviewed Jul 14, 2015
View reviewed changes

rullzer added 2 - Developing 3 - To Review and removed 3 - To Review 2 - Developing labels Jul 15, 2015

MorrisJobke mentioned this pull request Jul 17, 2015

Jenkins rullzer mimetypedetector #17718

Closed

rullzer mentioned this pull request Jul 20, 2015

Added vfsStream as dev dependency owncloud-archive/3rdparty#185

Closed

rullzer reviewed Jul 26, 2015
View reviewed changes

rullzer mentioned this pull request Jul 27, 2015

[JENKINS] Mimetypedetector jenkins #17889

Closed

RobinMcCorkell reviewed Jul 27, 2015
View reviewed changes

rullzer added 5 commits July 27, 2015 14:58

Added mimetype detector

6db6689

* Copied unit tests from old functions

Move mimetypedetection to files/type/detection

141a0f0

Loading of mapping/aliases is done in class

9cdd637

Proper deprecate methods

88fb389

Pass config dir as parameter to detection class

d276aeb

In order to properly test the mimetype function: * constructor takes path to configdir * Added unit tests for mimetype (only if vfsStream is available)

MorrisJobke added a commit that referenced this pull request Jul 28, 2015

Merge pull request #15543 from rullzer/mimetypedetector

c34e63b

Mimetypedetector

MorrisJobke merged commit c34e63b into owncloud:master Jul 28, 2015

rullzer deleted the mimetypedetector branch October 1, 2015 11:06

lock bot locked as resolved and limited conversation to collaborators Aug 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mimetypedetector #15543

Mimetypedetector #15543

rullzer commented Apr 10, 2015

rullzer commented Apr 10, 2015

RobinMcCorkell Apr 10, 2015

rullzer Apr 13, 2015

MorrisJobke commented Jul 2, 2015

rullzer commented Jul 14, 2015

RobinMcCorkell Jul 14, 2015

rullzer Jul 14, 2015

rullzer commented Jul 14, 2015

rullzer commented Jul 14, 2015

RobinMcCorkell Jul 14, 2015

rullzer Jul 15, 2015

rullzer commented Jul 15, 2015

rullzer commented Jul 15, 2015

RobinMcCorkell commented Jul 15, 2015

rullzer commented Jul 15, 2015

rullzer commented Jul 16, 2015

rullzer commented Jul 17, 2015

RobinMcCorkell commented Jul 18, 2015

rullzer commented Jul 18, 2015

RobinMcCorkell commented Jul 18, 2015

rullzer commented Jul 19, 2015

rullzer commented Jul 20, 2015

MorrisJobke commented Jul 20, 2015

rullzer commented Jul 21, 2015

MorrisJobke commented Jul 21, 2015

rullzer Jul 26, 2015

rullzer commented Jul 27, 2015

rullzer commented Jul 27, 2015

RobinMcCorkell Jul 27, 2015

rullzer Jul 27, 2015

scrutinizer-notifier commented Jul 27, 2015

ghost commented Jul 27, 2015

RobinMcCorkell commented Jul 27, 2015

MorrisJobke commented Jul 28, 2015

Mimetypedetector #15543

Mimetypedetector #15543

Conversation

rullzer commented Apr 10, 2015

rullzer commented Apr 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MorrisJobke commented Jul 2, 2015

rullzer commented Jul 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rullzer commented Jul 14, 2015

rullzer commented Jul 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rullzer commented Jul 15, 2015

rullzer commented Jul 15, 2015

RobinMcCorkell commented Jul 15, 2015

rullzer commented Jul 15, 2015

rullzer commented Jul 16, 2015

rullzer commented Jul 17, 2015

RobinMcCorkell commented Jul 18, 2015

rullzer commented Jul 18, 2015

RobinMcCorkell commented Jul 18, 2015

rullzer commented Jul 19, 2015

rullzer commented Jul 20, 2015

MorrisJobke commented Jul 20, 2015

rullzer commented Jul 21, 2015

MorrisJobke commented Jul 21, 2015

Choose a reason for hiding this comment

rullzer commented Jul 27, 2015

rullzer commented Jul 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scrutinizer-notifier commented Jul 27, 2015

ghost commented Jul 27, 2015

RobinMcCorkell commented Jul 27, 2015

MorrisJobke commented Jul 28, 2015