As entities, Scald Atoms are fieldable, but when a field is added to a scald atom type, no validation of the field's widget takes place when the atom is saved. Per its own documentation, field_attach_form_validate() should be called for each atom that is being edited or created.

CommentFileSizeAuthor
#13 atom_add_edit_form-2890463-13.patch3.74 KBnagy.balint
#9 atom_add_edit_form-2890463-9.patch3.75 KBmartin_q
#7 atom_add_edit_form-2890463-7.patch3.68 KBmartin_q
#6 atom_add_edit_form-2890463-6.patch3.68 KBmartin_q
#2 atom_add_edit_form-2890463-2.patch1.63 KBmartin_q
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

martin_q created an issue. See original summary.

martin_q’s picture

martin_q
Primary language English
Location Leipzig, Germany | Manchester, UK
CreditAttribution: martin_q commented
Status: Active » Needs work
FileSize
atom_add_edit_form-2890463-2.patch1.63 KB

See patch attached, which is based on the code in scald_atom_add_form_options_submit() as well as node_form_validate().

martin_q’s picture

martin_q
Primary language English
Location Leipzig, Germany | Manchester, UK
CreditAttribution: martin_q commented
Issue summary: View changes
martin_q’s picture

martin_q
Primary language English
Location Leipzig, Germany | Manchester, UK
CreditAttribution: martin_q commented
Status: Needs work » Needs review

Whoops, wrong status.

nagy.balint’s picture

nagy.balint CreditAttribution: nagy.balint commented

Thanks for the patch!

It seems we already have similar code in "scald_atom_add_form_options_submit" that constructs a new form state related to the issue #2710927: Wrong $form_state passed to entity_form_submit_build_entity()

Maybe we could make a function or some way to make sure that we don't have the same code at several places.

martin_q’s picture

martin_q
Primary language English
Location Leipzig, Germany | Manchester, UK
CreditAttribution: martin_q commented
FileSize
atom_add_edit_form-2890463-6.patch3.68 KB

Thanks for the tip, @nagy.balint. Refactored patch from #2 and abstracted the repeated code into a separate function.

martin_q’s picture

martin_q
Primary language English
Location Leipzig, Germany | Manchester, UK
CreditAttribution: martin_q commented
FileSize
atom_add_edit_form-2890463-7.patch3.68 KB

Corrected syntax for passing of reference to $form_state and $atom_form_state arrays.

nagy.balint’s picture

nagy.balint CreditAttribution: nagy.balint commented

I guess there is a mistake at
"+function &_scald_get_atom_form_state(&$form_state, $index) {"

martin_q’s picture

martin_q
Primary language English
Location Leipzig, Germany | Manchester, UK
CreditAttribution: martin_q commented
FileSize
atom_add_edit_form-2890463-9.patch3.75 KB

I've never used the syntax in that line before, but it seems to be legit according to PHP documentation when you want to return the reference to an array, which is what I wanted to do, in order to be extremely cautious about maintaining the data in the form state correctly.

But there were errors in the lines immediately following that line. Sorry for not checking it thoroughly enough. It seems from my tests that the attached new patch fixes them.

nagy.balint’s picture

nagy.balint CreditAttribution: nagy.balint commented

--- Removed due next comment.

nagy.balint’s picture

nagy.balint CreditAttribution: nagy.balint commented

Okay now I see that then it returns with a reference.

Although Im not sure why we need that here, for performance reasons?

nagy.balint’s picture

nagy.balint CreditAttribution: nagy.balint commented

I suppose as it create a new variable, it would not have any relation with the original form_state, but to be extra careful we can also remove the & before the form state? So then modification on the form state inside the function would not be possible.

nagy.balint’s picture

nagy.balint CreditAttribution: nagy.balint commented
FileSize
atom_add_edit_form-2890463-13.patch3.74 KB

So to illustrate #12, would a patch like this work?

nagy.balint’s picture

nagy.balint CreditAttribution: nagy.balint commented
Status: Needs review » Needs work

I guess the form_state was passed by reference because the values_excluding_atoms is now stored in form state for performance reasons.

So the patch in #13 wont use that, but we can readd the reference I guess.

But Im still not sure if the return by reference is needed.

Also I noticed an issue, that in scald_atom_add_form_options_submit, the values is already built, but then not used for anything now.
And so it does not pass the same data now than it passed before.