I started rewriting some of the of the user_relationship_service to be compatible with services 3.x
Please take a look and let me know what you think - not everything has been changed yet. Attaching in comment so I'd know the issue number

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

MrMaksimize’s picture

this is my first patch, so be forgiving on the title. but you can rip on everything else :)

MD3’s picture

Great start MrMaksimize! I went ahead and did the full conversion here. This is against the latest version of user_relationship_service for 6.x-1.3 (April 30th, 2012) version.

For the module maintainers, I understand that until Services finishes their version feature, you may not want to roll this out for legacy support/backwards compatibility. I completely understand and support that stance. I hope this patch starts a conversation on upgrading and moving this module forward with Services as it upgrades.

The beauty is, from what I can tell, that this same exact code should work for 7.x. I made sure to leave the original functions in tact as much as possible. However, do we need the version variable? I can't seem to find where it is used.

MrMaksimize’s picture

Status: Needs work » Needs review

OH sweet!! Switched it to needs review so the bot and maintainer can take a look :)

MD3’s picture

Awesome! Thanks for that! I want to do 3 things before this gets committed:
-Review the comments to make sure that arguments are laid in the address like og_services
-I think the retrieve array is missing an int/id param (but still works!)
-Have Kyle Browning review it to see I did everything the way he envisions it.

Berdir’s picture

Status: Needs review » Needs work

Thanks for the patch. A 7.x-1.x version of this would be great to have as well.

Not sure what to do about the services 3.x vs. older versions thing. Replacing it like this breaks BC. Maybe introduce a new user_relationship_service3.module?

+++ b/user_relationship_service/user_relationship_service.moduleundefined
@@ -1,4 +1,5 @@
 <?php
+// $Id: user_relationship_service.module,v 1.1.2.1 2009/07/06 10:24:44 alexk Exp $

$Id$ tags need to be removed. They are a thing of the luckily long gone CVS times. No idea where it is coming from exactly here ;)

+++ b/user_relationship_service/user_relationship_service.moduleundefined
@@ -18,104 +19,12 @@ function user_relationship_service_help($path, $arg) {
- * Implementation of hook_service().
+ *  * Implementation of hook_services_resources.

Two * and missing ().

+++ b/user_relationship_service/user_relationship_service.moduleundefined
@@ -18,104 +19,12 @@ function user_relationship_service_help($path, $arg) {
+  require_once("user_relationship_service_resource.inc");

This can't work, needs to be a module_load_include().

+++ b/user_relationship_service/user_relationship_service_resource.incundefined
@@ -0,0 +1,333 @@
+// $Id: user_relationship_service.inc,v 1.1.2.3 2010/05/14 07:47:40 alexk Exp $

No need for $Id$'s anymore. Looks like a left-over...

+++ b/user_relationship_service/user_relationship_service_resource.incundefined
@@ -0,0 +1,333 @@
+  $resources['user']['targeted_actions']['relationship'] = array(
+        'help' => t('Requests a relationship with another user'),
+        'callback' => 'user_relationship_service_request',
+        'file' => array('type' => 'inc', 'module' => 'user_relationship_service', 'name' => 'user_relationship_service_resource'),

Should only be indented two spaces I think.

+++ b/user_relationship_service/user_relationship_service_resource.incundefined
@@ -0,0 +1,333 @@
+    	  array(
+        	'name' => 'uid',
+        	'type' => 'int',
+        	'description' => 'UID to request a relationship with',
+        	'source' => array('path' => '0'),
+        	'optional' => false,
+      	  ),

tabs here instead of two spaces.

+++ b/user_relationship_service/user_relationship_service_resource.incundefined
@@ -0,0 +1,333 @@
+          //NOTE: This is left here for future purposes of being able to only show

Comments should have a space after the //. Also, I don't think the NOTE: prefix is not necessary, isn't a commit a note by definition? :)

+++ b/user_relationship_service/user_relationship_service_resource.incundefined
@@ -0,0 +1,333 @@
+ * @param $version
+ *   Version of the user_relationships_api to use.
+ *   NOTE: Is this depricated? Should it be removed?

Looks like it is, no idea what that's supposed to be.

+++ b/user_relationship_service/user_relationship_service_resource.incundefined
@@ -0,0 +1,333 @@
+ *   Version of the user_relationships_api to use.
+ *   NOTE: Is this depricated? Should it be removed?
+ * @param $uid ¶
+ *     user id of person to request relationship with
+ * @param $type_name ¶
+ *     name of type of relationship to request. ¶
+ *     @see user_relationships_types_load(); or [GET] {endpoint}/relationships/types

Some trailing spaces. @param description should be indented two spaces as well and start with an uppercase character.

@see is only for standalone lines at the end, embedded function names are linked automatically. So this should either be moved at the bottom or the @ removed.

+++ b/user_relationship_service/user_relationship_service_resource.incundefined
@@ -0,0 +1,333 @@
+    error_log("Relationship request with '$uid' type '$type_name'");

error_log() looks like a debug left-over.

+++ b/user_relationship_service/user_relationship_service_resource.incundefined
@@ -0,0 +1,333 @@
+    global $user;
+    $ret = user_relationships_request_relationship($user, $uid, $type);
+    if (!$ret) {
+      throw new Exception(t('Unknown failure'));
+    }
+    elseif (!is_object($ret)) {
+      throw new Exception($ret);
+    }
+    return $ret;
+  } catch (Exception $ex) {
+    return services_error(t('Error requesting relationship: @msg', array('@msg' => $ex->getMessage())));

The problem with this pattern is that the actual error messages are not translatable, so I'm not sure if a direct return services_error(t()) wouldn't be better instead of the indirection through Exceptions which are then could immediately again.

MD3’s picture

Ok. Thanks for the review! I did everything but the final item. I'm not sure how you want to proceed on that. A quick look at Services and it seems they don't use that method so we don't really have a precedent to follow.

It has been created as a separate module now (user_relationship_service_3). I thought about renaming the existing one to user_relationship_service_2, but realized that would cause a lot of BC issues.

Please let me know how you want to proceed on the catching of exceptions. In the meantime I'm going to ask Kyle Browning to review my endpoints to make sure I'm using the proper ones he envisioned.

bhavin.ahya’s picture

Category: feature » bug
Priority: Normal » Major

If it has been a separate module then wherever you posted the following code :
'file' => array('type' => 'inc', 'module' => 'user_relationship_service', 'name' => 'user_relationship_service_resource'),

It should be :
'file' => array('type' => 'inc', 'module' => 'user_relationship_service_3', 'name' => 'user_relationship_service_3'),

Now it will work properly

Status: Needs review » Needs work