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

fix(conversion): missing target file extension #50219

Merged
merged 2 commits into from
Jan 16, 2025

Conversation

elzody
Copy link
Contributor

@elzody elzody commented Jan 16, 2025

Summary

I discovered that, when working on the richdocuments file conversion provider, not all file conversion providers will be properly detected when searching for the correct file extension to assign the converted file. When no destination is provided, the filename will not include an extension (e.g. doc. instead of doc.pdf); however, when a destination is provided, we do not append the extension since it is assumed the destination would include it (e.g. /subfolder/doc.pdf).

The issue lies with the following bit of code, primarily:

private function getRegisteredProviders(): array {
    if (count($this->providers) > 0) {
        return $this->providers;
    }

    // ...continued
}

If this method is called twice, the logic for getting a registration context to peek at the registered file conversion providers is skipped, and the preexisting providers are simply returned. This sounded like a good idea at first, but I ran into an issue where my richdocuments file conversion provider was not being returned in this list on subsequent calls to the method (maybe due to it not being fully registered yet or something?).

Removing the if statement seems to work and resolve the issue, but I think we can avoid having to call this method more than once anyway but changing the logic for getting the correct file extension. We already get the valid conversion provider that can do the conversion, so we just get the right extension from its supported ones instead of re-fetching all the registered providers.

The tests implemented already never failed, and my guess for that is because the testing app's conversion providers are being registered first somehow before other the other apps' providers. /shrug

Checklist

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>
Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>
@elzody elzody added the 3. to review Waiting for reviews label Jan 16, 2025
@elzody elzody added this to the Nextcloud 31 milestone Jan 16, 2025
@elzody elzody self-assigned this Jan 16, 2025
@provokateurin provokateurin merged commit d1b5488 into master Jan 16, 2025
188 of 189 checks passed
@provokateurin provokateurin deleted the fix/file-conversion-missing-extension branch January 16, 2025 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants