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

Don't load app for dav requests #135

Merged
merged 3 commits into from
Aug 24, 2020

Conversation

kesselb
Copy link
Contributor

@kesselb kesselb commented Feb 23, 2020

After #124 it's not longer necessary to add a mapping kdbx => application/x-kdbx to mimetypemapping.json. Instead the keeweb app is loaded for every request and adds the mapping.

Loading the app for every request is not ideal. Recently @sndrr pointed me to dicomviewer app and how they solved that problem. The app automatically appends the mapping for the mime types to mimetypemapping.json and the admins don't have to deal with json. This PR will add the same for keeweb app. Kudos to @ayselafsar for the idea 🚀.

cc @maurerle @SimJoSt mind to help me testing? ;)

@SimJoSt
Copy link

SimJoSt commented Feb 26, 2020

@kesselb so when installing this version of the app, it should automatically add the mimetype to the config files, correct?
I see there is an unregister method. When does that one trigger?

What should happen when there is already an entry for the mimetype in the config?


if (file_exists($mappingFile)) {
$existingMapping = json_decode(file_get_contents($mappingFile), true);
if (json_last_error() === JSON_ERROR_NONE) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
if (json_last_error() === JSON_ERROR_NONE) {
if (json_last_error() === JSON_ERROR_NONE && is_array($existingMapping)) {

otherwise array_merge logs a warning and returns null.

@kesselb
Copy link
Contributor Author

kesselb commented Feb 26, 2020

Thanks @SimJoSt 👍

installing this version of the app, it should automatically add the mimetype to the config files, correct?

Yes. The mapping will be added to $configDirectory/mimetypemapping.json.

I see there is an unregister method. When does that one trigger?

If the app is disabled (either via the app manager or on upgrade if the app is not compatible).

What should happen when there is already an entry for the mimetype in the config?

<?php

$old = [
    'dng' => ["image/x-dcraw"],
    'fb2' => ["application/x-fictionbook+xml", "text/plain"],
    'ical' => ["text/calendar"],
    'kdbx' => ['application/x-kdbx'],
];

$new = [
    'kdbx' => ['application/x-kdbx']
];

var_dump(array_merge($old, $new));

The expected result is to see the entry for kdbx once. That should array_merge already do.

@kesselb kesselb force-pushed the enh/dont-load-app-for-dav branch from 7d0bb98 to 3a5ca0c Compare February 26, 2020 22:36
@maurerle
Copy link
Contributor

I just tested this in a development environment with NC18 in docker and can confirm that the registration and unregistration is working properly.

@maurerle
Copy link
Contributor

I don't quite like the way this is done in general. Every app which can open a file with a given mimetype has its own implementation for the (un)registration of a MimeType.
This is a lot of duplication and slightly different code for the same functionality. It does not feel right that an App rewrites the global mimetypemapping file on its own and can possibly break it.

I would like it far better if the core server provided a maintained functionality with a recommended usage to (un)register and mimetypes by just providing the needed mapping.

@arnowelzel
Copy link
Collaborator

I don't quite like the way this is done in general. Every app which can open a file with a given mimetype has its own implementation for the (un)registration of a MimeType.
This is a lot of duplication and slightly different code for the same functionality. It does not feel right that an App rewrites the global mimetypemapping file on its own and can possibly break it.

I would like it far better if the core server provided a maintained functionality with a recommended usage to (un)register and mimetypes by just providing the needed mapping.

Unfortunately this is still not solved. Also see nextcloud/server#9192

@arnowelzel
Copy link
Collaborator

@jhass To me this looks fine. I'd like to include this for the NC 19 update as well, what do you think?

@jhass
Copy link
Owner

jhass commented Aug 24, 2020

Your call, this is exactly the kind of thingies I'm a bit tired off to be honest :P

@arnowelzel arnowelzel merged commit 93b3969 into jhass:master Aug 24, 2020
@kesselb kesselb deleted the enh/dont-load-app-for-dav branch August 24, 2020 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants