NPM Resource Changed ? Pending

User tests: Successful: Unsuccessful:

avatar wilsonge
wilsonge
25 May 2020

Summary of Changes

Fixes an edge case in the media field

Testing Instructions

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.

Documentation Changes Required

None.

avatar wilsonge wilsonge - open - 25 May 2020
avatar wilsonge wilsonge - change - 25 May 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 25 May 2020
Category Repository NPM Change JavaScript Libraries
avatar wilsonge wilsonge - change - 25 May 2020
Labels Added: NPM Resource Changed ?
avatar brianteeman
brianteeman - comment - 26 May 2020

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


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&57547082c4814bec4eba5ba8437f0f83=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
VM1563:27 Uncaught TypeError: Cannot read property 'message' of undefined
    at <anonymous>:27:31
(anonymous) @ VM1563:27
joomla-field-media.min.js:1 Uncaught (in promise) undefined
avatar Quy
Quy - comment - 26 May 2020

I get the alert now, but still the undefined warning.

29221

avatar wilsonge
wilsonge - comment - 26 May 2020

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 ?

avatar brianteeman
brianteeman - comment - 26 May 2020
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
avatar Quy Quy - test_item - 26 May 2020 - Tested successfully
avatar Quy
Quy - comment - 26 May 2020

I have tested this item successfully on 444123d


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/29221.

avatar wilsonge
wilsonge - comment - 26 May 2020

@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?

avatar brianteeman
brianteeman - comment - 26 May 2020

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

avatar Quy
Quy - comment - 26 May 2020

Tested successfully in Firefox and Chrome Version 83.0.4103.61.

avatar brianteeman
brianteeman - comment - 26 May 2020

image

avatar wilsonge
wilsonge - comment - 26 May 2020

:( 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.

avatar brianteeman
brianteeman - comment - 26 May 2020

Cant see how it will be a browser cache as I never use the other browsers

avatar brianteeman
brianteeman - comment - 26 May 2020

Is this what you wanted
image

avatar wilsonge
wilsonge - comment - 28 May 2020

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?

avatar dgrammatiko
dgrammatiko - comment - 28 May 2020

@wilsonge unfortunately this isn't right. Pressing select without selecting an image is a possible valid case (eg remove an image form the form). I'll have a look on this one this weekend

avatar dgrammatiko
dgrammatiko - comment - 28 May 2020

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)});

      
avatar Quy
Quy - comment - 28 May 2020

To clear the image, click on the x icon.

image

avatar dgrammatiko
dgrammatiko - comment - 28 May 2020

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

avatar wilsonge
wilsonge - comment - 29 May 2020

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)

avatar dgrammatiko
dgrammatiko - comment - 29 May 2020

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...

avatar dgrammatiko
dgrammatiko - comment - 29 May 2020
avatar wilsonge wilsonge - change - 29 May 2020
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2020-05-29 13:24:44
Closed_By wilsonge
avatar wilsonge wilsonge - close - 29 May 2020
avatar wilsonge
wilsonge - comment - 29 May 2020

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

avatar wilsonge
wilsonge - comment - 29 May 2020

Your PR is obviously better. Closing in favour of

Add a Comment

Login with GitHub to post a comment