Existing 7.x-1.x-dev code always fails QA tests

After some digging I've discovered some facts about the failed tests:

• og only started to fail the QA tests when drupal core was updated from 7.14 to 7.15 (based on running tests using identical versions of all contrib modules)
• The problem is triggered by the testPropopulateViaUrlHidden test. The url http://drupal7_clean.local/node/add/article?gids_entity_test=1 causes gid=1 to be added to OG_AUDIENCE_FIELD twice. This is presumably a new bug in core or somewhere else outside of og.

Although the source of the problem is outside of og, making og's form validation more robust by filtering duplicate gids is a fix that's within the scope of og. The attached patch makes that change and has fixed the QA tests on my test machine.

Also, for what it's worth, it seems unlikely that this is the same problem as #1559766: EntityMalformedException when deleting users, in og_get_user_roles(), even though the error messages are the same.

My apologies for the triple post. However, I continued to dig to the bottom of what suddenly triggered this problem, and in the process of banging my head against the table I've found some more og issues.

The first point is that it seems that upgrading core from drupal 7.14 to drupal 7.15 is somewhat of a red herring. Yes, it triggered some minor changes in the tests, but those minor changes shouldn't have derailed og. Specifically, it seems that in drupal 7.15 the testing module is using more clean urls. So the key url being tested changed from http://drupal7_clean.local/index.php?q=node/add/article&gids_entity_test=1 to http://drupal7_clean.local/node/add/article?gids_entity_test=1. Trivial, but enough to cause the subsequent processing changes.

The short story of what then happens in og is that gid gets set to both "1" (string value) and 1 (integer value) in the audience field's default values (within the function og_field_widget_form()). Those duplicates propagate all the way through to the final calls to og_membership_create().

My first patch cleaned up the duplicates at the validation step. I've now added a bit more cleaning during the setup phase, as well.

Although I've done the basic fixes, I didn't try to tackle some other more nebulous questions that arose while investigating this issue.

• Why is the default value for OG_AUDIENCE_FIELD being set twice? It is being set first by og_field_audience_default_value() (which includes a call to og_get_context_by_url()). That call sets the value of $items in og_field_widget_form(). Then within og_field_widget_form(), og_get_context_by_url() is called again. I suspect the second call is unnecessary, but I didn't remove it just in case it was added to handle some strange circumstance.
• Why is the processing for setting the default value different in the two cases? og_field_audience_default_value() puts the data through an access check (which happened to also convert "1" to 1), but no access check is done on the alternative default values. Also, the code in og_field_widget_form() trims deleted groups, but that processing is effectively only applied to $items, and is skipped for the alternative default_values. Shouldn't the same validation be applied to the input regardless of the data source?
• I'm guessing that the original code used $default_values[$gid] = $gid (line 238 of og.field.inc) with the intent of preventing up duplicate gids. However, it doesn't work given that $default_values starts as an unkeyed array -- in which case, the code could cause existing values to be overwritten without explanation. Therefore I changed the line even though it's not really responsible for the issues I was investigating.

I can't judge the importance of these questions/issues -- especially given that all of the relevant code has been removed from og-7.x-2.x, so this only affects a dead-end branch of the module. Nevertheless, I wanted to make the information available to those of you who are more knowledgeable about og.

In the specific case triggered by the QA tests, #hidden_selected_gids is empty, so the entire validation was being skipped. However, $form_state['values'][OG_AUDIENCE_FIELD] contained duplicate values, so it needs to be validated (regardless of whether or not #hidden_selected_gids contains data) -- and the duplicates trigger the exception in og_membership_create that's causing the test failure. In other words, removing those lines allowed the existing validation to fix the problem.

Thanks for committing it so quickly!

I've re-queued the patches that incorrectly failed because of this bug (at least the ones that I'm aware of), so I'm afraid I've probably just added more patches onto your to-review list ;)