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

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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

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

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

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

ParisLiakos’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

josefnpat’s picture

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

Status: Needs work » Needs review

switching to correct status

iamEAP’s picture

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

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

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

Status: Needs work » Needs review
FileSize
909 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

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

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

aaron’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me.

ParisLiakos’s picture

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

Status: Patch (to be ported) » Needs review
FileSize
662 bytes

Here you go.

peteruithoven’s picture

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

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

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

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

jibize’s picture

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

ParisLiakos’s picture

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

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