foreach ($form_state['values'][$field_name][$lang] as $value) {

This doesn't work if the field is e.g. inside a field collection.

CommentFileSizeAuthor
#20 inline-edit.png16.95 KBandypost
#18 media-element-validate-1558830-18.patch662 bytesaaron
#15 media-element-validate-1558830-15.patch965 bytesazinck
#13 media-element-validate-1558830-13.patch909 bytesParisLiakos
#8 media-to_tip_update-1789720-2.patch714 bytesjosefnpat
#1 media-element-validate-1558830-1.patch1.03 KBberdir

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: media_element_validate() has hardcoded $form_state values reference » media_element_validate() has hardcoded $form_state values reference and does not work with unlimited
Priority: Normal » Major
Status: Active » Needs review
StatusFileSize
new media-element-validate-1558830-1.patch1.03 KB

Actually, there are 3 different bugs in that function :)

- the already mentioned $form_state issue, fixed in the patch by using drupal_array_get_nested_value() with the #array_parents of $element. Tested this with field groups, field collections and a bare field.
- It is completely broken for unlimited field items because then there is an array entry "Add another item". And if ($value['fid']) is TRUE because 'fid' is cast to 0 and the string does have a character at index 0. PHP--
- Additionally, the validation error message does not contain the correct field name for multiple values.

I'm not sure if the whole thing shouldn't be in hook_field_validate()...

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented 
+++ b/media.moduleundefined
@@ -991,13 +991,19 @@ function media_element_validate(&$element, &$form_state) {
+      $instance = field_widget_instance($element, $form_state);
+      form_error($element, t('%element_title is required.', array('%element_title' => $instance['label'])));

This seems to be field-module specific and does this mean it would not be possible to use a media FAPI element in a non-field context?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

Yes it is, but the old code asssumes fields and field languages as well:

+++ b/media.moduleundefined
@@ -991,13 +991,19 @@ function media_element_validate(&$element, &$form_state) {
-    foreach ($form_state['values'][$field_name][$lang] as $value) {
-      if ($value['fid']) {

I guess if it needs to work for non-fields, then this needs to be re-designed completely, maybe two different functions?

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Marked #1691500: Validation fails if mediafield is in a nested form - Inline Entity Form as a duplicate of this issue.

ParisLiakos’s picture

ParisLiakos commented

closed as dups #1801284: Media field validation doesnt work with #required Media Fields in a Field Collection. and #1789720: Required field validation fails with profile2

ParisLiakos’s picture

ParisLiakos commented
Status: Needs work » Needs review

#1: media-element-validate-1558830-1.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, media-element-validate-1558830-1.patch, failed testing.

josefnpat’s picture

josefnpat commented
StatusFileSize
new media-to_tip_update-1789720-2.patch714 bytes

The same patch submitted by axel.rutz, but from this commit (hopefully tip):

commit 283d06b3742f23e491a99ce324295716232e1412
Author: Paris Liakos <rootatwc@gmail.com>
Date:   Thu Oct 11 22:20:07 2012 +0300

    Issue #1810296 encoding problem in media_default.view.inc

Someone please also confirm that this patch works for them.

ParisLiakos’s picture

ParisLiakos commented
Status: Needs work » Needs review

switching to correct status

iamEAP’s picture

iamEAP commented
Status: Needs review » Reviewed & tested by the community

Can confirm this works for me. Switching to RTBC, but obviously the more reviews, the merrier.

ParisLiakos’s picture

ParisLiakos commented
Status: Reviewed & tested by the community » Needs work 
+++ b/media.moduleundefined
@@ -1016,11 +1016,13 @@ function media_element_process(&$element, &$form_state, $form) {
+    $values = $form_state['values'];
+    foreach ($element['#array_parents'] as $parent) {
+      $values = $values[$parent];
+    }

this doesnt seem quite right to me..
seems $values is being overriden and my guess is that we want to append those values in the array

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

No, it's supposed to work like that, it's working itself through the nested $values array to the one it wants.

However, it should probably drupal_array_get_nested_value() or whatever that was called again ;)

ParisLiakos’s picture

ParisLiakos commented
Status: Needs work » Needs review
StatusFileSize
new media-element-validate-1558830-13.patch909 bytes

Ok, i think i got it..thanks for the tip Berdir..
i actually took patch @#1..and used $element['#title'] for the label..right?
this way this is not field specific..

peteruithoven’s picture

peteruithoven commented

The #13 patch doesn't give me a warning when I'm leaving a Media file selector field that is set to unlimited but is required, empty.

azinck’s picture

azinck commented
StatusFileSize
new media-element-validate-1558830-15.patch965 bytes

peteruithoven: Try this. Minor change incorporated from #999594: media form elements don't respect '#required' property

aaron’s picture

aaron commented
Status: Needs review » Reviewed & tested by the community

This looks good to me.

ParisLiakos’s picture

ParisLiakos commented
Version: 7.x-2.x-dev » 7.x-1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

committed to 2.x! thanks all:)
http://drupalcode.org/project/media.git/commit/d10a1ed

anyone up for a backport? should be fairly easy, this function is pretty much the same iirc

aaron’s picture

aaron commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new media-element-validate-1558830-18.patch662 bytes

Here you go.

peteruithoven’s picture

peteruithoven commented

Great and fast work azinck!
In my installation the error is missing the field label. For some reason $element's title is empty. I was able to retrieve the label from the $form_state. But I'm not sure if that's the way to go. 

    $fieldInfo = $form_state['field'][$field_name];
    $fieldLabel = $fieldInfo['und']['instance']['label'];
    if (!$has_value) {
      form_error($element, t('%element_title is required.', array('%element_title' => $fieldLabel)));
    }
andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new inline-edit.png16.95 KB

I think it's related...
I've updated media(2.x) and inline_entity_form(1.1) but for some reason I see all links. Edit link points to /media/0/edit/nojs and thrown error, Add & Remove just scrolls the page.
When I edit products without inline everything works fine

inline-edit.png

azinck’s picture

azinck commented

andypost: I haven't used inline_entity_form so I'm not sure of the interactions there, but have you updated to the corresponding version of File Entity?

andypost’s picture

andypost
he/him
Primary language Russian
commented

@azinck Thanx, update File Entity helps. I forget that it no more a part of media

jibize’s picture

jibize commented

I was having problems with an unlimited media field within a field collection and #18 worked for me, thanks!

ParisLiakos’s picture

ParisLiakos commented
Status: Needs review » Fixed

thanks aaron for the backport and jibize for reporting back:)
committed!
http://drupalcode.org/project/media.git/commit/0dd8bde

Status: Fixed » Closed (fixed)

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

azinck’s picture

azinck commented

The problem reported in #19 still exists. Using the latest dev and a fresh installation of D7 the validation message is not showing the field name:

I've opened a new issue: #2024609: Field label missing from error message when a "required" unlimited cardinality file field is left empty