#validate, #submit and ajax callbacks have already been improved to support method callbacks. The same thing should be done for #element_validate. This would be especially cool for field widgets.

#1802072: Ajax callbacks should be callbacks defined as function or class method

Change record for supporting FAPI #validate, #submit method callbacks: http://drupal.org/node/1734540

CommentFileSizeAuthor
#9 1805688-9-element_validate.patch5.06 KBfubhy
#5 1805688-5-element_validate.patch4.67 KBfubhy
#1 1805688-1-element_validate.patch4.66 KBfubhy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fubhy’s picture

fubhy CreditAttribution: fubhy commented
FileSize
1805688-1-element_validate.patch4.66 KB

The attached patch is the most simple solution to that. However, I am a little bit worried that, especially in case of field widgets, we might add multiple, identical object references to the form array. Not sure how we could easily solve that or if that is even a problem.

fubhy’s picture

fubhy CreditAttribution: fubhy commented
Issue summary: View changes

Adding related issue references.

fubhy’s picture

fubhy CreditAttribution: fubhy commented
Issue summary: View changes

Adding link to change record.

fubhy’s picture

fubhy CreditAttribution: fubhy commented
Title: Add support methods as #element_validate callbacks » Support methods as #element_validate callbacks

Fixing typo in title.

Status: Needs review » Needs work

The last submitted patch, 1805688-1-element_validate.patch, failed testing.

Stalski’s picture

Stalski CreditAttribution: Stalski commented 
+++ b/core/modules/system/tests/modules/form_test/form_test.moduleundefined
@@ -385,11 +424,13 @@ function system_form_form_test_alter_form_alter(&$form, &$form_state) {
+    '#element_validate' => array($object, 'validateName'),

This should be
'#element_validate' => array(array($object, 'validateName')) since the callback function or array(object,method) is an array.

fubhy’s picture

fubhy CreditAttribution: fubhy commented
FileSize
1805688-5-element_validate.patch4.67 KB

Whoops... Of course :). Thanks @stalski.

Stalski’s picture

Stalski CreditAttribution: Stalski commented
Status: Needs work » Needs review

Setting the status to have the patch tested

Stalski’s picture

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

Tested this and reviewed it. Straight forward patch imo

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work

This looks good to me, and consistent with what we're doing elsewhere in form.inc, with one exception:

+++ b/core/modules/system/tests/modules/form_test/form_test.moduleundefined
+++ b/core/modules/system/tests/modules/form_test/form_test.moduleundefined
@@ -372,6 +372,45 @@ function system_form_form_test_alter_form_alter(&$form, &$form_state) {
   drupal_set_message('system_form_form_test_alter_form_alter() executed.');
 }
 
+class FormTestValidateForm {

It's not kosher to declare classes inside module files. They need to be in PSR-0 files/folders.

While we're at it, can you please add PHPDoc for this class?

fubhy’s picture

fubhy CreditAttribution: fubhy commented
Status: Needs work » Needs review
FileSize
1805688-9-element_validate.patch5.06 KB

Sure.

Stalski’s picture

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

Just reviewed it. Again looking good.
I think this one is good to be committed.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Title: Support methods as #element_validate callbacks » Change notice: Support methods as #element_validate callbacks
Category: feature » task
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Awesome, thanks!

Committed and pushed to 8.x.

We'll need a change notice for this; can likely just be added to the existing FAPI one.

fubhy’s picture

fubhy CreditAttribution: fubhy commented
Status: Active » Needs review

Done.

http://drupal.org/node/1734540

swentel’s picture

swentel CreditAttribution: swentel commented
Title: Change notice: Support methods as #element_validate callbacks » Support methods as #element_validate callbacks
Priority: Critical » Normal
Status: Needs review » Fixed

Looks good.

Stalski’s picture

Stalski CreditAttribution: Stalski commented
Title: Support methods as #element_validate callbacks » Change notice: Support methods as #element_validate callbacks
Priority: Normal » Critical

Perfect fubhy.

Stalski’s picture

Stalski CreditAttribution: Stalski commented
Title: Change notice: Support methods as #element_validate callbacks » Support methods as #element_validate callbacks
Priority: Critical » Normal

submissions at the same time

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

I'm unhappy that the existing test code was moved.

This means that we have a dedicated test for #element_validate using a class method now, but the dedicated test for a procedural function got lost.

The proper action would have been to add a second #element_validate entry for the class method and specifically assert that.

fubhy’s picture

fubhy CreditAttribution: fubhy commented

There are other tests that involve #element_validate and are not using method callbacks. What we are testing here is if #element_validate works. And we didn't change any #element_validate logic. We just changed is_function into is_callable. Actually I was unsure if it even makes sense to add a test for it at all. I mean.. We do not set up an array or an object in our tests to see if is_array() or is_object() works... Right? ;)

So... It doesn't matter what we do actually. And just in case - There are still other tests that use #element_validate with plain function callbacks.

fubhy’s picture

fubhy CreditAttribution: fubhy commented

doubleclick, sorry

Status: Fixed » Closed (fixed)

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

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: -Needs change record
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue summary: View changes

Woot, change records can be linked with node numbers :(