I'm writing a module to integrate userpoints with the migrate module, which would allow people to import userpoints based off of views, or any sql table (even a remote one). After I run the API function, I need to get the txn_id so I can update our mapping table, but params are not passed by reference, and I can't think of a good way to do it otherwise (hooks seem like overkill) . How about this 1 char change, to make params passed by reference?

function userpoints_userpointsapi(&$params) {
CommentFileSizeAuthor
#9 userpoints-441678-drupal_write_record2.patch2.64 KBBrandonian
#6 userpoints-441678-drupal_write_record.patch2.53 KBBrandonian
#3 userpoints_params_by_ref_and_use_drupal_write_record.patch2.26 KBfrankcarey

Comments

frankcarey’s picture

frankcarey commented
Title: Use drupal_write_record(). Needed to be able to import transactions with migrate_extras » have userpoints_userpointsapi params passed by reference
Version: 6.x-1.x-dev » 6.x-1.0
Assigned: frankcarey » Unassigned
Status: Needs work » Active

hmm. not working, i get this passed back out... no txn_id

* description (String, 20 characters ) points from 4.7 site
* display (String, 1 characters ) 0
* moderate (String, 5 characters ) FALSE
* points (String, 2 characters ) 31
* tid (String, 2 characters ) 31
* uid (String, 4 characters ) 1110
* operation (NULL)
* reference (NULL)
* entity_id (NULL)
* entity_type (NULL)
* status (Integer) 1
* expired (NULL)
* parent_txn_id (NULL)
* expirydate (NULL)

frankcarey’s picture

frankcarey commented

OK, I see why. We're not using drupal_write_record, so we never know the txn_id of a new record.

I see there has been a few months btw 1.0 and 1.dev, maybe you already did this, but if you didn't how about using drupal_write_record instead?

http://api.drupal.org/api/function/drupal_write_record/6

frankcarey’s picture

frankcarey commented
Status: Active » Needs review
StatusFileSize
new userpoints_params_by_ref_and_use_drupal_write_record.patch2.26 KB

OK, this patch accomplishes both $params being passed by reference, and using drupal write record instead of the insert. so you get the whole txn back (including txn_id) for passing to the post-save hooks, and then passing the final $params object back through the $params argument.

I tested programatically and it worked fine, haven't tried any contrib modules.

Patched against version 6.1.0

frankcarey’s picture

frankcarey commented
Title: have userpoints_userpointsapi params passed by reference » Use drupal_write_record(). Needed to be able to import transactions with migrate_extras
Assigned: Unassigned » frankcarey

bump. Can we implement this? It's needed for the integration with the Migrate / Migrate Extras modules.

kbahey’s picture

kbahey commented
Version: 6.x-1.0 » 6.x-1.x-dev
Status: Needs review » Needs work

Patch no longer applies.

$ patch -p0 --dry-run < /tmp/x.patch
patching file userpoints.module
Hunk #1 FAILED at 464.
Hunk #2 succeeded at 649 (offset 80 lines).
Hunk #3 FAILED at 765.
2 out of 3 hunks FAILED -- saving rejects to file userpoints.module.rej

Please re-roll against DRUPAL-6--1 CVS tag.

Brandonian’s picture

Brandonian commented
Title: have userpoints_userpointsapi params passed by reference » Use drupal_write_record(). Needed to be able to import transactions with migrate_extras
Version: 6.x-1.0 » 6.x-1.x-dev
Assigned: Unassigned » frankcarey
Status: Active » Needs review
StatusFileSize
new userpoints-441678-drupal_write_record.patch2.53 KB

Half of the previous patch's intent (using drupal_write_record()) has already been applied in Drupal-6--1. This patch fixes the other half of the problem (passing &params by reference), as well as adding another drupal_write_record in _userpoints_update_cache(). Passes all the same Simpletests prior to the patch, but has 3 fewer notices! Woot!

kbahey’s picture

kbahey commented
Status: Needs review » Fixed

Committed.

Thanks.

kbahey’s picture

kbahey commented
Status: Fixed » Needs work

This is causing API problems #786378: Last DEV release incompatible with UserPointsContrib module.

Therefore I am rolling this back.

Brandonian’s picture

Brandonian commented
StatusFileSize
new userpoints-441678-drupal_write_record2.patch2.64 KB

Patch reapplies the drupal_write_record() in _userpoints_update_cache(). Passing $params by reference is a non-issue for me, and since it seems to break contrib, no reason to include it. SimpleTests pass the same as they did with the previous patch (which is two more than without the fix), and enabling userpoints contrib modules doesn't seem to break anything.

Patch also leaves the previous Simpletests, just as a backup in case the reference addition is attempted again.

Brandonian’s picture

Brandonian commented
Status: Needs work » Needs review

Forgot to mark needs review...

kbahey’s picture

kbahey commented

If 2 people review this and test that it is working with contribs, I will commit it.

Antoine Lafontaine’s picture

Antoine Lafontaine commented

I've submitted a patch here http://drupal.org/node/786382 that I believe would allow patch in #6 to be committed including passing &$params by reference (which I would like to see happen).

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
+++ userpoints.module	30 Apr 2010 17:28:52 -0000
@@ -802,28 +802,20 @@ function _userpoints_update_cache(&$para
+    $update = array('uid', 'tid');
   }
+  ¶
+  drupal_write_record('userpoints', $record_params, $update);

Bad dreditor always makes these these trailing whitespaces red :)

+++ tests/userpoints_api.test	30 Apr 2010 17:28:52 -0000
@@ -48,8 +48,9 @@ class UserpointsTestCase extends DrupalW
+    $this->assertNotNull($points, t('Sanity check, Points is assigned something !points', array('!points' => $points)));
     $db_points = (int) db_result(db_query($sql, $user->uid, $points));
-    $this->assertTrue($db_points === $points, t('Successfully verified points in the txn table'));
+    $this->assertTrue($db_points === $points, t('Successfully verified points in the txn table !dbpoints !points', array('!dbpoints' => $db_points, '!points' => $points)));

I don't get this part...

- $points is rand(1, 500), so it can never be NULL?

- The other part just adds both points values to the description, while that might be nice for debugging, isn't necessary?

So, I don't see what's actually being tested here.

Other than that, the actually proposed change looks good to me. While passing $params by reference would be nice, I see why it can break contrib modules. Maybe that's something we can add to the D7 version (Or even make params an object there, the it simply is by reference by default).

Powered by Dreditor.

Fidelix’s picture

Fidelix commented
Component: Code: userpoints API » Code: userpoints

Subscribing...

manuel.adan’s picture

manuel.adan
Location 🌌
commented
Assigned: frankcarey » Unassigned
Issue summary: View changes
Status: Needs work » Closed (outdated)

Closing this as outdated, 6.x version is no longer maintained.