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

Mimetypedetector #15543

Merged
merged 5 commits into from
Jul 28, 2015
Merged

Mimetypedetector #15543

merged 5 commits into from
Jul 28, 2015

Conversation

rullzer
Copy link
Contributor

@rullzer rullzer commented Apr 10, 2015

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 rullzer added this to the 8.2-next milestone Apr 10, 2015
@rullzer
Copy link
Contributor Author

rullzer commented Apr 10, 2015

CC: @DeepDiver1975 @Xenopathic

RobinMcCorkell
RobinMcCorkell reviewed Apr 10, 2015
View reviewed changes
tests/lib/files/type/detection.php

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

@rullzer Still valid? Then please rebase ;)

This was referenced Jul 6, 2015
Closed
Merged
@rullzer
Copy link
Contributor Author

rullzer commented Jul 14, 2015

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
RobinMcCorkell reviewed Jul 14, 2015
View reviewed changes
lib/private/helper.php
@@ -496,9 +428,10 @@ static function getSecureMimeType($mimeType) {
*
* @param string $data
* @return string
* @deprecated Use \OC::$server->getMimeTypeDetector->detectString($data)
Copy link
Member

Choose a reason for hiding this comment

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

getMimeTypeDetector()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks fixed

@rullzer
Copy link
Contributor Author

rullzer commented Jul 14, 2015

@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
Copy link
Contributor Author

rullzer commented Jul 14, 2015

Review time!

RobinMcCorkell
RobinMcCorkell reviewed Jul 14, 2015
View reviewed changes
lib/private/files/type/detection.php

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

Choose a reason for hiding this comment

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

Can we use dependency injection for the URL generator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@rullzer
Copy link
Contributor Author

rullzer commented Jul 15, 2015

@Xenopathic you are right. It looks cleaner now.

@rullzer
Copy link
Contributor Author

rullzer commented Jul 15, 2015

Lets see if Jenkins is still happy

@RobinMcCorkell
Copy link
Member

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
Copy link
Contributor Author

rullzer commented Jul 15, 2015

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

@rullzer
Copy link
Contributor Author

rullzer commented Jul 16, 2015

Ok rebased and squashed... let see

@rullzer
Copy link
Contributor Author

rullzer commented Jul 17, 2015

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

@MorrisJobke MorrisJobke mentioned this pull request Jul 17, 2015
Closed
@RobinMcCorkell
Copy link
Member

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

@rullzer
Copy link
Contributor Author

rullzer commented Jul 18, 2015

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
Copy link
Member

@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
Copy link
Contributor Author

rullzer commented Jul 19, 2015

@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 rullzer mentioned this pull request Jul 20, 2015
Closed
@rullzer
Copy link
Contributor Author

rullzer commented Jul 20, 2015

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

@MorrisJobke
Copy link
Contributor

@rullzer Rebase needed

@rullzer
Copy link
Contributor Author

rullzer commented Jul 21, 2015

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

@MorrisJobke
Copy link
Contributor

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

Makes sense 😃

rullzer
rullzer reviewed Jul 26, 2015
View reviewed changes
tests/lib/files/type/detection.php
public function testMimeTypeIconStaticIcons() {
if (!class_exists('org\\bovigo\\vfs\\vfsStream')) {
$this->markTestSkipped('Pacakge vfsStream not installed');
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@rullzer
Copy link
Contributor Author

rullzer commented Jul 27, 2015

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

Review time!

@rullzer rullzer mentioned this pull request Jul 27, 2015
Closed
@rullzer
Copy link
Contributor Author

rullzer commented Jul 27, 2015

Jenkins is happy! #17889
Lets get this in!

Review time: @MorrisJobke @PVince81 @DeepDiver1975

@Xenopathic your 👍 is still valid I assume?

RobinMcCorkell
RobinMcCorkell reviewed Jul 27, 2015
View reviewed changes
lib/private/helper.php
@@ -496,9 +428,10 @@ static function getSecureMimeType($mimeType) {
*
* @param string $data
* @return string
* @deprecated Use \OC::$server->getMimeTypeDetector()->detectString($data)
Copy link
Member

Choose a reason for hiding this comment

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

Missing deprecation version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

rullzer added 5 commits July 27, 2015 14:58
@rullzer
Added mimetype detector
6db6689 
* Copied unit tests from old functions
@rullzer
Move mimetypedetection to files/type/detection
141a0f0
@rullzer
Loading of mapping/aliases is done in class
9cdd637
@rullzer
Proper deprecate methods
88fb389
@rullzer
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)
@scrutinizer-notifier
Copy link

A new inspection was created.

@ghost
Copy link

ghost commented Jul 27, 2015

🚀 Test PASSed.🚀
chuck

@RobinMcCorkell
Copy link
Member

👍 Let's go! cc @MorrisJobke @PVince81

@MorrisJobke
Copy link
Contributor

Code looks good and it still works 👍

MorrisJobke added a commit that referenced this pull request Jul 28, 2015
@MorrisJobke
Merge pull request #15543 from rullzer/mimetypedetector
c34e63b 
Mimetypedetector
@MorrisJobke MorrisJobke merged commit c34e63b into owncloud:master Jul 28, 2015
@rullzer rullzer deleted the mimetypedetector branch October 1, 2015 11:06
@lock lock bot locked as resolved and limited conversation to collaborators Aug 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
3 - To Review technical debt
Projects
None yet
Milestone
8.2
Development

Successfully merging this pull request may close these issues.
4 participants
@rullzer @MorrisJobke @RobinMcCorkell @scrutinizer-notifier