Mime types support from discovery.xml #106

donquixote · 2025-03-24T12:45:02Z

Exporing if discovery.xml can predict which MIME types are read-only.

brummbar

We don't expect the values inside discovery.xml to change suddenly.
The discovery.xml is anyway invalidated every X hours, so removed formats will be picked up.
Updates in the infrastructure with versions that remove support for files should be followed by a cache rebuild in Drupal.
We will add a way to clear the discovery cache manually from the UI.

brummbar · 2025-03-26T17:34:36Z

src/Discovery/Discovery.php

   */
-  public function getWopiClientURL(string $mimetype = 'text/plain'): ?string {
-    $result = $this->parsedXml->xpath(sprintf('/wopi-discovery/net-zone/app[@name=\'%s\']/action', $mimetype));
+  public function getWopiClientURL(string $mimetype = 'text/plain', ?string $action = NULL): ?string {


Action is mandatory per WOPI protocol

Looking at https://learn.microsoft.com/en-us/openspecs/office_protocols/ms-wopi/7ceea62b-4fb1-436b-af8b-fbf5a721fcba:

The name="..." attribute is required in <action> tag in discovery.xml.

There can be multiple <action> tags with different name and different urlsrc. Therefore calling this without $action and just getting the first match has an underdetermined result, or rather, a result that depends on the order of tags.

But, there can also be multiple <action> tags with same name but different ext and urlsrc, even though ext is optional.

We can make $action a required parameter here, I just want to point out that it being required in the xml is not the same as it being a required parameter in this method.

Also, with the discovery.xml from Collabora, we will have to fall back to 'edit' or 'view-comment' if we don't find a 'view' action.

Also if this is the second parameter, we need to make the first one required too, OR swap them, OR provide a string default, e.g. 'view'.
I will do one commit with default 'view' and then subsequent commits where I swap the params.

Put them both required. I don't see why the first one is not. We return only the first entry matching mime and action.

Ok, I made both required.

src/Discovery/DiscoveryInterface.php

donquixote · 2025-03-28T19:36:07Z

tests/src/Unit/CollaboraDiscoveryTest.php

+      'view_comment' => [
+        'application/pdf',
+      ],
+    ], $supported_action_types);


@brummbar We can remove this part of the test before we merge.
I want to keep it for now to have an overview what we will get with the existing discovery.xml from Collabora.

donquixote · 2025-03-28T19:49:30Z

tests/src/Unit/CollaboraDiscoveryTest.php

+        'calc',
+        'impress',
+        'writer',
+        'writer-global',


These ones are not really MIME types, and they tend to have multiple <action> tags with ext=".." attribute.
In the default discovery.xml from Collabora they all have the same url, but this could be customized.
To make that work we would have to also pass extension name. But if the MIME type is never 'writer' or 'impress' etc for a given file entity, then this part is rather pointless.

Not sure I understand what's this about.

So, the array has the following structure:
$arr[implode(',', $supported_actions)][] = $mime_type;
E.g.
$arr['view_comment'][] = 'application/pdf'; means that for application/pdf we have only view_comment action.

The MIME types are taken from 'name' attributes in the <app name="..."> tags.

But, some of the 'name' attributes in <app name="..."> are not really MIME types. E.g. <app name="writer">. You can expand the xml above to see.

donquixote · 2025-03-28T19:57:57Z

collabora_online.module

      'weight' => 50,
      'url' => CollaboraUrl::previewMedia($media),
    ],
  ];


To make this function complete, we should also check for viewability, not just editability.
However we would have to implement the fallback logic from 'view' to 'edit' and 'view_comment', to get a desirable outcome with Collabora.

collabora_online.module

donquixote · 2025-03-28T20:00:46Z

src/Controller/ViewerController.php

+          // are viewable have an 'edit' or 'view_comment' action but no 'view'
+          // action.
+          ?? $discovery->getWopiClientURL('edit')
+          ?? $discovery->getWopiClientURL('view_comment'));


This fallback logic is needed if we want a desirable behavior with current Collabora.

No, no fallback. View is view, view_comment is different.

Ah I see what you mean, some don't have view at all and only edit. Then we pass the edit flag and it handles whether they can edit or not. This is the info I need to avoid spending time looking around by myself.
I am not sure about view_comment, it allows to add comments and view a pdf, will it prevent to add comments if it's mean to be only view?

So currently for 'application/pdf' we have only 'view_comment' and neither 'view' nor 'edit'.
But Collabora does open pdf for viewing even though that is not explicitly mentioned in the file.

For a yes/no check, we could simply always say yes for "view", to keep the current behavior.
But we also need to obtain the correct url for each operation. Currently we always use text/plain to get the url for whichever operation, which only has 'edit'. So we are currently doing a double fallback from e.g. view + docx to edit + text/plain.

I am not sure about view_comment, it allows to add comments and view a pdf, will it prevent to add comments if it's mean to be only view?

To clarify this is only what we use for lookup in discovery.xml.
That lookup can give us a yes/no and a url. But that url is always the same in the default xml.

We would have to set something in the WOPI response or in the iframe url or the JWT token to tell Collabora whether comments should be allowed. Currently the module does not provide a dedicated comment operation.

To keep this fallback logic out of ViewerController and possibly other places in the future, we could decorate the Discovery class, or we could insert another service in between for MIME lookup.

donquixote force-pushed the mime-types branch 2 times, most recently from b88b5c3 to 0a7d593 Compare March 24, 2025 21:07

brummbar requested changes Mar 26, 2025

View reviewed changes

donquixote force-pushed the mime-types branch from 0a7d593 to a2afec1 Compare March 28, 2025 19:34

donquixote commented Mar 28, 2025

View reviewed changes

collabora_online.module Show resolved Hide resolved

donquixote commented Mar 28, 2025

View reviewed changes

donquixote force-pushed the mime-types branch from f7eb446 to 03e8517 Compare April 2, 2025 11:57

donquixote added 5 commits April 2, 2025 14:00

Add parameter for view/edit in Disdcovery->getWopiClientURL().

5f55c62

Test the realistic discovery.xml.

6d70d7d

Use discovery in CoolUtils::canEdit().

087932c

Collapse CoolUtils::canEdit() into collabora_online_entity_operation().

5bc0cc0

Make both parameters required in getWopiClientURL().

77f69f5

donquixote force-pushed the mime-types branch from 03e8517 to 77f69f5 Compare April 2, 2025 12:01

Mime types support from discovery.xml #106

Are you sure you want to change the base?

Mime types support from discovery.xml #106

Uh oh!

Conversation

donquixote commented Mar 24, 2025

Uh oh!

brummbar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants