User tests: Successful: Unsuccessful:
Fixes an edge case in the media field
Create an article, go to the media field (intro-text, full-text images), open the modal and hit the "select" button without actually selecting an image. Before patch you get a javascript error, after patch a nice happy warning.
None.
Status | New | ⇒ | Pending |
Category | ⇒ | Repository NPM Change JavaScript Libraries |
Labels |
Added:
NPM Resource Changed
?
|
Next time I'll remember to git add
the rest of the code instead of just the first draft. Can you guys try again please
core.min.js?6862a6fb3e4522145ec6145bf18e3adf:1 GET http://localhost/joomla-cms/administrator/index.php?option=com_media&format=json&task=api.files&url=true&path=undefined&ede718a22134338d7aa943ea16353869=1&format=json 500 (Internal Server Error)
a.request @ core.min.js?6862a6fb3e4522145ec6145bf18e3adf:1
(anonymous) @ joomla-field-media.min.js:1
a.getImage @ joomla-field-media.min.js:1
modalClose @ joomla-field-media.min.js:1
onSelected @ joomla-field-media.min.js:1
VM206:27 Uncaught TypeError: Cannot read property 'message' of undefined
at <anonymous>:27:31
(anonymous) @ VM206:27
joomla-field-media.min.js:1 Uncaught (in promise) undefined
I have tested this item
@brianteeman I can't reproduce - and obviously it's worked for @Quy too. can you give any more info (a gif/video or something) and the browser your using.
The URL you're getting is consistent with the original javascript error I expected before the patch was applied (note the path is undefined as you haven't selected an item which then gives a 500 server side). You have recompiled the javascript right for testing after the patch yes?
Retested on three browsers with the same results
Tested again in chrome with debug on and it works then
Tested again in chrome with debug off and the same error as before
Tested successfully in Firefox and Chrome Version 83.0.4103.61.
:( getImage shouldn't be getting called after this PR. can you scroll down to the function modalClose
in that file and check if the amended code exists - some sort of weird browser cache issue is the only thing i can think.
Cant see how it will be a browser cache as I never use the other browsers
Yup - that code doesn't have the patched code in :( - compare with what I've got in this PR. I can't explain what your doing differently - assume you've rebuilt the JS with npm after applying the patch, cleared caches and stuff?
This might work (untested)
Instead of
Joomla.getImage(Joomla.selectedFile, input, this);
Joomla.Modal.getCurrent().close();
use this:
Joomla.getImage(Joomla.selectedFile, input, this)
.then(()=>{Joomla.Modal.getCurrent().close();})
.catch(err => { console.log(err)});
To clear the image, click on the x icon.
Yes you're right, clicking on select without a selected image should just close the window not reset the field. Anyways that wasn't the reported problem here. The problem is a race condition...
These changes work for me
Joomla.getImage = (data, editor, fieldClass) => new Promise((resolve, reject) => {
// change start
if (!data || (typeof data === 'object' && (!data.path || data.path === ''))) {
reject(new Error('Nothing selected'));
return;
}
// change end
const apiBaseUrl = `${Joomla.getOptions('system.paths').rootFull}administrator/index.php?option=com_media&format=json`;
Joomla.request({
url: `${apiBaseUrl}&task=api.files&url=true&path=${data.path}&${Joomla.getOptions('csrf.token')}=1&format=json`,
method: 'GET',
perform: true,
headers: { 'Content-Type': 'application/json' },
onSuccess: (response) => {
const resp = JSON.parse(response);
resolve(Joomla.doIt(resp, editor, fieldClass));
},
// change start
onError: (err) => {
reject(err);
},
// change end
});
});
})(Joomla);
modalClose() {
const input = this.querySelector(this.input);
// change start
Joomla.getImage(Joomla.selectedFile, input, this)
.then(() => { Joomla.Modal.getCurrent().close(); })
.catch(() => { Joomla.Modal.getCurrent().close(); });
// change end
}
A bit of explanation the first function Joomla.getImage
is a promise so we have to do things on resolve and reject, it's not synchronous as the previous code was treating it. I guess my fault in the first place
The problem is Joomla.selectedFile is null. The race condition doesn’t come from this code. But then we call getImage with path undefined. And then the server gives a 500. This isn’t about fixing the race condition. It’s about handling when there’s no selected file (whether that’s because of the race condition you reported or because someone hasn’t selected a file)
The problem is Joomla.selectedFile is null
It goes way deeper. I just jumped into this rabbit hole and there are also needed changes in the Vue app. Will do a PR tomorrow.
Btw there are more unhandled cases...
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2020-05-29 13:24:44 |
Closed_By | ⇒ | wilsonge |
I know it goes deeper - I just couldn't figure out why it was null. But then figured we should be handling that case anyhow better than we were
Your PR is obviously better. Closing in favour of
Before this PR I was unable to replicate the reported issue.
After applying the PR I was still unable to replicate the reported result but I did get the following errors in the console