Of course displaying Invalid ID fallback representation is fine for now.

However with the latest ckeditor widget implementation, the system will call "atom/ajax-widget-expand/%scald_atom", which will return 404 page not found and give a js error popup.
I think this is an issue, as we should have a fallback in this case.

I also had a problem once where the user had several of these atoms on a single wysiwyg instance, and then of course apart from a lot of displayed js error popups, it was impossible to delete the atoms, cause even by selection delete from text area, some portion of the atom still remained, and when i saved the node the downCast died with a JS error and the save did not happen.

CommentFileSizeAuthor
#3 fix-js-error-on-deleted-atom-ckeditor-widget-2479281-3.patch2.21 KBnagy.balint
#2 fix-js-error-on-deleted-atom-ckeditor-widget-2479281-2.patch1.96 KBnagy.balint

Comments

nagy.balint’s picture

nagy.balint commented

I found the problem:
In:

  $items['atom/ajax-widget-expand/%scald_atom'] = array(
    'page callback' => 'mee_ajax_widget_expand',
    'page arguments' => array(2),
    'access callback' => TRUE,
    'delivery callback' => 'ajax_deliver',
    'theme callback' => 'ajax_base_page_theme',
  );

The scald_atom_load will be called for this url which returns FALSE if the %scald_atom is not a valid id, and that will cause a page not found.
We basically need a different loading function that produces a fallback atom object when the id is not valid, and then the callback would work.

nagy.balint’s picture

nagy.balint commented
Status: Active » Needs review
StatusFileSize
new fix-js-error-on-deleted-atom-ckeditor-widget-2479281-2.patch1.96 KB

Tried a few approaches, then ended up at the attached patch.

This will fix the immediate issue where upon opening the edit page the site keeps dropping js error popups by providing a fallback.

But Im open for suggestions if anyone has a better idea. @gifad ?

nagy.balint’s picture

nagy.balint commented
StatusFileSize
new fix-js-error-on-deleted-atom-ckeditor-widget-2479281-3.patch2.21 KB

One improvement in scald load functions, is that actually if the sid is not numeric we shouldn't even try to load an atom.

  • nagy.balint committed 9d10040 on 7.x-1.x 
    Issue #2479281 by nagy.balint: Ckeditor widget problems after deleting...
nagy.balint’s picture

nagy.balint commented
Status: Needs review » Fixed

This seems to be a good workaround for now.

At any rate we can change it in the future if necessary, but it solves the main issue of having these ajax errors popping up "endlessly" when a node has atoms in it embedded that are deleted permanently.

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented

A note about the concept of fallback. Should we return the fallback image for any case?
Mostly about this condition:

if (!is_numeric($sid)) {
  return FALSE;
}

It might be meaningful to return the fallback atom always when a valid atom cannot be loaded for any reason.

nagy.balint’s picture

nagy.balint commented

I was thinking about that as well.

But the sid has to be numeric, and the only way it wont be numeric if someone is manually tampering with the sid in plain text mode, or if someone is trying to do an XSS attack.

So I have decided back then to return FALSE in those cases.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.