ProfessionalWiki / SimpleBatchUpload

Allows for basic, no-frills uploading of multiple files
https://www.mediawiki.org/wiki/Extension:SimpleBatchUpload
Other
22 stars 17 forks source link

Fix JS query selector to match the previous id-attribute change #44

Closed Joeytje50 closed 3 years ago

Joeytje50 commented 3 years ago

The previous change, while necessary to make the HTML valid, did break batchuploading with a single instance, due to the id-attribute being used as a selector to find the textarea. This change will simply look for the name attribute within the same upload box, in order to match the correct textarea.

Also updated the release notes with a short description of both this change and the previous one.

It turns out that the current live version (1.7.0) actually has a major bug relating to having multiple instance of #batchupload on the page. Aside from just being invalid HTML with the id not being unique, this also causes the second batch upload to use the first batch upload's value. So a page with the following:

{{#batchupload:Foo}}
{{#batchupload:Bar}}

would cause files dropped on the "Bar" batchupload box to insert {{Foo}} onto the page. This is due to $("#myID") matching the first element with id="myID" to be matched, no matter what. The second element with that same id="myID" will simply be ignored by that query. This means that this change is not actually just a "let's comply with HTML standards" commit, but more of a "let's fix an actual breaking bug" commit.

The way I found out about the real effects the v1.7.0 bug have, was sadly the fact that on a wiki I edit on, this has actually caused hundreds of files to get the incorrect license added to them. I would recommend testing the unified changes of #43 and this PR, and if this does indeed work the way you expect it (which it should, going by my own testing), please release this fix ASAP, if at all possible.

Joeytje50 commented 3 years ago

As I've said in my previous comment, I highly recommend releasing this change ASAP, so that other wikis will not have the same issue that causes the wrong license to be used when multiple batchupload forms are used on a single page. This is a massive pain to correct when the wrong license is applied.

Please make this change the new release and make it available. If you're going to wait for a bit more to include with the next release then this major bug will be in your software the entire time until the next release.

JeroenDeDauw commented 3 years ago

done

wiki-hazmat commented 3 years ago

I think this change and #43 completely break the extension with Cannot read property 'replace' of undefined.

This is because $(container).find('[name="wfUploadDescription"]').val(); is always undefined, because the element with name=wfUploadDescription is not a child of the container variable, which is the span.fileupload-container.

Something like $(container).parent().find('[name="wfUploadDescription"]').val(); would work, but there is probably a better way to fix the original issue than going further this direction

JeroenDeDauw commented 3 years ago

Uh, seriously? @Joeytje50 did you not test your changes at all?

JeroenDeDauw commented 3 years ago

@wiki-hazmat is it working in 1.8.1 now?