FriendsOfFlarum / upload

The file upload extension with insane intelligence for your Flarum forum.
https://discuss.flarum.org/d/4154
MIT License
177 stars 96 forks source link

fix regression for template extensions ( A beta 16 breaking change) #281

Closed eddiewebb closed 3 years ago

eddiewebb commented 3 years ago

Bug from change

Bug Report

Current Behavior Prior to beta 16 my extension (gpx preview) added a custom template to upload, worked great.

changes as part of beta 16 introduced hard-coded switch statement inclusive only of your 4 templates. And now my plugin is broke.

Steps to Reproduce

  1. create extension with a "tag" not in the predefined switch
  2. it defaults to a file URL

Expected Behavior It looks up tag based on the list of templates it has, and then asks the tag'd template for it's appropriate BBcode.

Screenshots

export default function fileToBBcode(file) {
    switch (file.tag()) {
        // File
        case 'file':
            return `[upl-file uuid=${file.uuid()} size=${file.humanSize()}]${file.baseName()}[/upl-file]`;

        // Image template
        case 'image':
            return `[upl-image uuid=${file.uuid()} size=${file.humanSize()} url=${file.url()}]${file.baseName()}[/upl-image]`;

        // Image preview
        case 'image-preview':
            return `[upl-image-preview url=${file.url()}]`;

        // 'just-url' or unknown
        default:
            return file.url();
    }
}

Environment

Possible solution(s) A hard coded switch like this is generally a sign that some logic should be delegated to the child/component classes. We already have a list of all registered templates, and a lookup function.

We should shift interface of templates to (i think logically) include the BBCode (oh wait, they do!) they generate/ parse as well. This could be done in PHP where the collection of templates lived, and a "bbcode" attribute returned from the API on upload success for the frontend to use when placing the (also server generated) url into editor.

Additional Context I hacked the File class and overwrote with myown, fixed upload but display worked, so I think there weee other changes in a similar assumption of upload-only templates.

imorland commented 3 years ago

Hi @eddiewebb

I'm so sorry this crept in to the beta 16 update. I did spot it in the PR originally, but seems it never got addressed before merging.

I've started to work on a possible solution to this here #286 - it's still a work-in-progress, but I'd be greatful for your input, especially as your own extension is extending fof/upload templates :)