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.

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

Move mimetypes.list.php to config/mimetypemapping.dist.json #17481

Merged
merged 3 commits into from
Jul 13, 2015
Merged

Move mimetypes.list.php to config/mimetypemapping.dist.json #17481

merged 3 commits into from
Jul 13, 2015

Conversation

rullzer
Copy link
Contributor

@rullzer rullzer commented Jul 8, 2015

This allows users to add new mimetypemappings (extention -> mimetype)
themself. And not have to wait until a new release for updated
mimetypes.

Fixes: #15384

CC: @oparoz @Xenopathic @MorrisJobke @PVince81 @DeepDiver1975

This allows users to add new mimetypemappings (extention -> mimetype)
themself. And not have to wait until a new release for updated
mimetypes.

Fixes: #15384
@rullzer rullzer added this to the 8.2-current milestone Jul 8, 2015
@rullzer
Copy link
Contributor Author

rullzer commented Jul 8, 2015

mmmm, we actually should make sure that this file does not get overwritten during an upgrade... else the user loses his additions...
Same actually holds for config/mimetypealiases.json

@oparoz
Copy link
Contributor

oparoz commented Jul 8, 2015

Isn't the config folder left untouched?

I don't remember the instructions. If admins have to move the config, they can also move those json files.

@RobinMcCorkell
Copy link
Member

Perhaps we should support loading multiple files, e.g. mimetypemapping.custom.json, which is packaged as empty and never gets overridden by updates. Any entries in that file override the ones in mimetypemapping.json, which has a note at the top explicitly telling admins not to put stuff there.

Or do it the other way around - make the custom one mimetypemapping.json, with a mimetypemapping.dist.json for the distributed packaged one.

Likewise for the other mimetype file of course.

@RobinMcCorkell
Copy link
Member

But otherwise this PR looks good 👍

@oparoz
Copy link
Contributor

oparoz commented Jul 8, 2015

Good call @Xenopathic. Makes it clear what is what.

@RobinMcCorkell
Copy link
Member

Interesting side note, oC actually loads any config files with the name *.config.php (https://github.com/owncloud/core/blob/master/lib/private/config.php#L183). Not particularly useful for configs, but very handy in this case where we need a default packaged file that gets updated.

@rullzer
Copy link
Contributor Author

rullzer commented Jul 8, 2015

Agreed lets use mimetypemapping.dist.json... let me fix that for this PR. And create a new one for the mimetypealiases.json

@rullzer
Copy link
Contributor Author

rullzer commented Jul 8, 2015

Done :)

@rullzer rullzer changed the title Move mimetypes.list.php to config/mimetypemapping.json Move mimetypes.list.php to config/mimetypemapping.dist.json Jul 8, 2015
"_comment2": "The first index in the mime type array is the assumed correct mimetype",
"_comment3": "and the second (if present] is a secure alternative",

"_comment4": "To add your own mappings create a file mimetypemapping.json",
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be phrased like a warning:

Any changes you make here will be overwritten on an update of ownCloud
Put any custom mappings in a new file mimetypemapping.json in this directory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

This allows users to create their own mapping file to extend our current
mappings. This makes sure that custom mappings are not lost on OC
upgrades.
@RobinMcCorkell
Copy link
Member

Nice! 👍

We need some documentation for this though. And do we want to ship a blank mimetypemapping.json with only comments saying how to use the file? Or do we want to require the user to manually create the file, and be expected to know the syntax etc?

@oparoz
Copy link
Contributor

oparoz commented Jul 8, 2015

@Xenopathic I'd say it's an advanced feature and it shouldn't be too much to ask of an admin to create the file.

@rullzer
Copy link
Contributor Author

rullzer commented Jul 10, 2015

Added todo issue for the docs.

One more review guys :)

@@ -424,8 +424,18 @@ static function rmdirr($dir, $deleteSelf = true) {
*/
static public function getMimetypeDetector() {
if (!self::$mimetypeDetector) {
$dist = file_get_contents(OC::$SERVERROOT . '/config/mimetypemapping.dist.json');
Copy link
Contributor

Choose a reason for hiding this comment

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

@rullzer Can you change this to \OC::$configDir?

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 didn't know about that one. Fixed!

@scrutinizer-notifier
Copy link

A new inspection was created.


//Check if need to load custom mappings
if (file_exists(OC::$configDir . '/mimetypemapping.json')) {
$custom = file_get_contents(OC::$configDir . '/mimetypemapping.json');
Copy link
Member

Choose a reason for hiding this comment

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

Actually, TBH I think the code looks better with those slashes there. PHP ignores multiple slashes (as does any UNIX utility), so we have no bug concerns, and it also means less effort if $configDir at some point loses the trailing slash.

TL;DR: leave the slashes, they're good 😄

@RobinMcCorkell
Copy link
Member

👍

@ghost
Copy link

ghost commented Jul 10, 2015

🚀 Test PASSed.🚀
chuck

@MorrisJobke
Copy link
Contributor

Mime type detection still works 👍

MorrisJobke added a commit that referenced this pull request Jul 13, 2015
Move mimetypes.list.php to config/mimetypemapping.dist.json
@MorrisJobke MorrisJobke merged commit 1006ec5 into owncloud:master Jul 13, 2015
@rullzer rullzer deleted the mimetype-list branch July 13, 2015 06:31
@lock lock bot locked as resolved and limited conversation to collaborators Aug 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move mimetypes.list.php to a text file in /config
5 participants