search_api_admin_add_server_validate() used to pass $service->configurationFormValidate() a reference to $form_state['values']['options']['form']. It now passes the $values variable to ensure that it is an array. However the $values variable isn’t a reference, so any changes that $service->configurationFormValidate() makes to it are lost. The server has to be edited and saved to get the values to save correctly. search_api_admin_add_server_validate() should pass a reference as search_api_admin_server_edit_validate() does.

CommentFileSizeAuthor
#3 new-server-validation-2414425-2.patch1.07 KBdarren oh
#2 2414425-2--empty_server_form_check.patch1.3 KBdrunken monkey
#1 new-server-validation-2414425.patch1.04 KBdarren oh

Comments

darren oh’s picture

darren oh
Primary language English
Location Lakeland, Florida
commented
Assigned: darren oh » Unassigned
Status: Active » Needs review
StatusFileSize
new new-server-validation-2414425.patch1.04 KB
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
StatusFileSize
new 2414425-2--empty_server_form_check.patch1.3 KB

Thanks for spotting this problem, and already providing a patch!
There's one typo/mistake in there, though:

+++ b/search_api.admin.inc
@@ -303,8 +303,8 @@ function search_api_admin_add_server_validate(array $form, array &$form_state) {
+  $form_state['values']['options']['form'] += array();

That's certainly not what you want to do there. You probably mean:
$form_state['values']['options'] += array('form' => array());

Also, I guess validating empty configuration forms doesn't really make sense anyways? And also, it seems we don't check at all when editing the server – that should also be fixed, definitely!
So, here's a revised patch. Do you think that takes care of the problem, too?
In any case, thanks for the intiative here!

darren oh’s picture

darren oh
Primary language English
Location Lakeland, Florida
commented
StatusFileSize
new new-server-validation-2414425-2.patch1.07 KB

I think a developer who writes a validation method would expect it to run.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Related issues: +#2445251: Plugin form validation/submission logic

I think a developer who writes a validation method would expect it to run.

Hm, interesting viewpoint. I would disagree with that, but it's probably best to discuss this in a larger context to have a definite decision here. See #2445251: Plugin form validation/submission logic.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs review » Fixed

Since there's no discussion in the other issue, I'm just committing my version for now. If we reach a contrary conclusion in the other issue, we'll have to unify this stuff in all the other places anyways.
In any case, thanks again for spotting the issue and providing a patch!

Status: Fixed » Closed (fixed)

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