Notice: Undefined index: values in scald_image_scald_add_atom_count() (line 74 of {...}/scald/modules/providers/scald_image/scald_image.module).
and others, due to a misconditioned call to scald_atom_add_form_add_submit().

Comments

gifad created an issue. See original summary.

gifad’s picture

Status: Active » Needs review
StatusFileSize
new477 bytes

Attached patch is just a workaround - the modal state identification could probably be detected more efficiently...

nagy.balint’s picture

StatusFileSize
new2.57 KB

I think that, that function has to run, as it is the one that creates the new Atom entity and such.

And we cannot check whether form state values is empty or not (outside of the hook), because the idea behind the hook in that function was that the provider decides how many entity it needs.

So in fact the issue is in the image provider and any provider that implements this hook, to not do something that can throw a notice.

So attached a patch that fixes this for the base providers.

nagy.balint’s picture

Although it seems i cant reproduce the problem here, as that code only runs if
if (empty($form_state['scald'])) {
And form_state scald is not empty for me if i edit an atom.

So for me this situation never appears.
Any additional steps to reproduce?

nagy.balint’s picture

Because in the
function scald_atom_edit_page($js, $atom) {
It will use
ctools_object_cache_set('scald_atom', 'edit:' . $atom->sid, $scald);
Which is then read

  $form_state = array(
    'ajax' => $js,
    'scald' => ctools_object_cache_get('scald_atom', $cache_id),
  );

So in fact if you are editing atoms it cannot be empty.

Unless its possible to somehow avoid using that edit_page function and not have this cache.

gifad’s picture

This call of scald_atom_add_form_add_submit() is here for the case of atom creation, when the provider does not specify a specific add_form hook - so the add step is 'automatically' validated;
In the case of the bug, we are just editing an existing atom, the edit has successfully completed, and scald_atom_add_page() is called to finalize the ctools wizard; the real bug is that this call is made with the $atom_id argument set to NULL (instead of the actual atom id), and that confuses scald_atom_add_page() : the $cache_id is set to 'add', at line 57, so the cache is not retrieved, so the $form_state['scald'] is NULL, etc...

Anyway, it does not make sense to submit a form without values, hence my simple patch...

--- cross posting , this is reply to #3

gifad’s picture

steps to reproduce ?

fresh install of scald suite

  • create an atom, validate;
  • create an article
    • in the library widget, click "Edit" of the atom
    • in the ctools modal form, just click "Finish"
  • Visit admin/reports/dblog if you did not see the notices in the library widget, AFTER the "Atom {title}, of type Image has been updated." message
nagy.balint’s picture

I think that "scald_atom_add_page" should not be called when clicking the finish on the edit form, that might be the real bug.

But still checking.

gifad’s picture

"scald_atom_add_page" should actually be called at this stage, but just for executing :

    if ($output === FALSE || !empty($form_state['complete'])) {
      // Dismiss the modal.
      $commands[] = array('command' => 'dnd_refresh');
      $commands[] = ctools_modal_command_dismiss();
    }

(around line 145)

nagy.balint’s picture

I see, then the bug is that it does too much in that step, maybe a code restructure is needed.

nagy.balint’s picture

When adding a new atom:
If the first step is options and not add, then form_state values will be empty.
Yet that function should run, because it will create the single atom entity or whatever the provider defines.

So then i think patch #2 breaks that.

Patch #3 would remove the notice, but it requires all providers to pay attention to it, and add issets.

Alternatively if we can detect that its just about to close and add that to the if, it could be a good solution as well.

nagy.balint’s picture

Status: Needs review » Needs work
nagy.balint’s picture

Status: Needs work » Needs review
StatusFileSize
new867 bytes

Maybe this patch will work.

The idea behind it is that when rendering the options form, we can check whether
$form_state['scald']['atoms']
is empty or not. If its empty then very likely its not an edit form, and it had no first add step, as otherwise it would not be empty.
And then we can run that function.

Please test :)

nagy.balint’s picture

StatusFileSize
new1.27 KB

In fact because of that we can spare one more line.

gifad’s picture

#14 works for me with

  • upload method = managed file or plupload
  • first step = options or unspecified

Also ok with scald_gallery...

I had built another workaround, based on distinction between $step passed as argument vs $step computed, but #14 is far a better approach ;)

nagy.balint’s picture

Great! Will commit tomorrow if no negative feedback til then :)

nagy.balint’s picture

Status: Needs review » Reviewed & tested by the community
nagy.balint’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, Committed.

  • nagy.balint committed 580a52a on 7.x-1.x
    Issue #2580337 by nagy.balint, gifad: PHP notices while editing atom in...

Status: Fixed » Closed (fixed)

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