I've attached a patch which makes the following changes compared to what's currently committed to SVN.

  • Standardizes indentation and spacing to match Drupal standards.
  • Cleans up some comments to make them a bit clearer.
  • Rewrites all SQL queries to be safe from SQL injection. This is my first time writing SQL queries against the CiviCRM API, so checking those would be great.
  • Changes the menu item to use dashes instead of underscores, as underscores are often missed as spaces in URLs.
  • Corrects use of hook_menu() to not translate title and description.
  • Corrects comments for the form callbacks, as they aren't implementing node form hooks, but Form API callbacks.
  • Renames "Save Rule" to "Save rule" to match UI text conventions.

I haven't actually run this patch against anything, and probably won't be able to until tomorrow.

CommentFileSizeAuthor
#8 636602_user_sync_sql_error.patch1.94 KBdeviantintegral
#3 civicrm_user_sync_review.patch15.72 KBdeviantintegral
#1 civicrm_user_sync_review.patch15.72 KBdeviantintegral
civicrm_user_sync_review.patch15.7 KBdeviantintegral

Comments

deviantintegral’s picture

deviantintegral commented
StatusFileSize
new civicrm_user_sync_review.patch15.72 KB

Here is an updated patch with a change from dlobo making the fields returned be limited to strings.

deviantintegral’s picture

deviantintegral commented

There was an issue with a user having two Drupal accounts associated with a single CiviCRM contact. When attempting to create contacts for all Drupal users, it errors out. This is a bug in CiviCRM in that it allows a user to specify an email address belonging to another Drupal account, and will likely be fixed in 3.1 or 3.2.

I've deleted the email address from http://association.drupal.org/civicrm/contact/view?reset=1&cid=9984, but since users can put it in later, it's possible for this issue to come up again.

I'll be able to test the above patch on the staging site tomorrow.

deviantintegral’s picture

deviantintegral commented
StatusFileSize
new civicrm_user_sync_review.patch15.72 KB

This update only adds a few newlines, and changes the menu item to be sentence case like the rest of the menu system.

I've run this on the stage site, and looking at 5-6 accounts it seems to be working fine. The only thing to note is that contacts without Drupal accounts won't have Drupal user names, but that will be resolved as we pare down duplicate contacts.

I'll be committing this patch in a moment. I think we need this on the live site, so it will likely be deployed as-is, but it still needs the following updates:

  • The cron script should be updated to not hard-code the table, field, and contact_id settings.
  • The cron script should be updated to use parameterized queries (though it doesn't take any input, always better to be safe).
  • Currently, there's no way to remove the sync setting without disabling the module.
  • Right now, only one field is synced, and the name of the module is even coded to be for "username_sync". If this is as far as we're going with this, then the custom table should be removed and replaced with a simple variable.
Amazon’s picture

Amazon commented

When I log into the d6-association.drupal.org site I get:

Sorry. A non-recoverable error has occurred.
DB Error: syntax error

Return to home page.

Error Details:

Array
(
[callback] => Array
(
[0] => CRM_Core_Error
[1] => handle
)

[code] => -2
[message] => DB Error: syntax error
[mode] => 16
[debug_info] => SELECT 'drupal_org_username_6' FROM 'civicrm_value_drupal_org_profile_4' WHERE entity_id = 3 [nativecode=1064 ** You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ''civicrm_value_drupal_org_profile_4' WHERE entity_id = 3' at line 1]
[type] => DB_Error
[user_info] => SELECT 'drupal_org_username_6' FROM 'civicrm_value_drupal_org_profile_4' WHERE entity_id = 3 [nativecode=1064 ** You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ''civicrm_value_drupal_org_profile_4' WHERE entity_id = 3' at line 1]
[to_string] => [db_error: message="DB Error: syntax error" code=-2 mode=callback callback=CRM_Core_Error::handle prefix="" info="SELECT 'drupal_org_username_6' FROM 'civicrm_value_drupal_org_profile_4' WHERE entity_id = 3 [nativecode=1064 ** You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ''civicrm_value_drupal_org_profile_4' WHERE entity_id = 3' at line 1]"]
)

deviantintegral’s picture

deviantintegral commented
Status: Needs review » Needs work

And here's the backtrace:

backTrace

/var/www/d6-association.drupal.org/htdocs/sites/all/modules/civicrm/CRM/Core/Error.php, backtrace, 146
, handle, 
/var/www/d6-association.drupal.org/htdocs/sites/all/modules/civicrm/packages/PEAR.php, call_user_func, 912
/var/www/d6-association.drupal.org/htdocs/sites/all/modules/civicrm/packages/DB.php, PEAR_Error, 966
/var/www/d6-association.drupal.org/htdocs/sites/all/modules/civicrm/packages/PEAR.php, DB_Error, 574
/var/www/d6-association.drupal.org/htdocs/sites/all/modules/civicrm/packages/DB/common.php, raiseError, 1903
/var/www/d6-association.drupal.org/htdocs/sites/all/modules/civicrm/packages/DB/mysqli.php, raiseError, 928
/var/www/d6-association.drupal.org/htdocs/sites/all/modules/civicrm/packages/DB/mysqli.php, mysqliRaiseError, 402
/var/www/d6-association.drupal.org/htdocs/sites/all/modules/civicrm/packages/DB/common.php, simpleQuery, 1216
/var/www/d6-association.drupal.org/htdocs/sites/all/modules/civicrm/packages/DB/DataObject.php, query, 2394
/var/www/d6-association.drupal.org/htdocs/sites/all/modules/civicrm/packages/DB/DataObject.php, _query, 1587
/var/www/d6-association.drupal.org/htdocs/sites/all/modules/civicrm/CRM/Core/DAO.php, query, 144
/var/www/d6-association.drupal.org/htdocs/sites/all/modules/civicrm/CRM/Core/DAO.php, query, 866
/var/www/d6-association.drupal.org/htdocs/sites/all/modules/civicrm_drupal_username_sync/civicrm_drupal_username_sync.module, executeQuery, 62
/var/www/d6-association.drupal.org/htdocs/modules/user/user.module, civicrm_drupal_username_sync_user, 22
/var/www/d6-association.drupal.org/htdocs/modules/user/user.module, user_module_invoke, 1374
/var/www/d6-association.drupal.org/htdocs/modules/user/user.module, user_authenticate_finalize, 1350
/var/www/d6-association.drupal.org/htdocs/modules/user/user.module, user_authenticate, 1309
/var/www/d6-association.drupal.org/htdocs/includes/form.inc, user_login_authenticate_validate, 774
/var/www/d6-association.drupal.org/htdocs/includes/form.inc, form_execute_handlers, 719
/var/www/d6-association.drupal.org/htdocs/includes/form.inc, _form_validate, 584
/var/www/d6-association.drupal.org/htdocs/includes/form.inc, drupal_validate_form, 403
/var/www/d6-association.drupal.org/htdocs/includes/form.inc, drupal_process_form, 119
/var/www/d6-association.drupal.org/htdocs/modules/user/user.pages.inc, drupal_get_form, 378
, user_page, 
/var/www/d6-association.drupal.org/htdocs/includes/menu.inc, call_user_func_array, 348
/var/www/d6-association.drupal.org/htdocs/index.php, menu_execute_active_handler, 18
Amazon’s picture

Amazon commented

After I revist my user account the error is gone.

deviantintegral’s picture

deviantintegral commented
Issue tags: +DrupalconSF

Tagging.

deviantintegral’s picture

deviantintegral commented
StatusFileSize
new 636602_user_sync_sql_error.patch1.94 KB

Apparently CiviCRM doesn't let you use parameters as a table or column name, so we're stuck with string building for those parts of the queries. I'll be committing this patch which fixes the issue.

eliza411’s picture

eliza411 commented
Status: Needs work » Closed (fixed)

Closing old issues. Please re-open if needed.

Project: Drupal.org infrastructure » Drupal Association Project(s)
Component: association.drupal.org » Other