Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Comments
Comment #1
MrMaksimize CreditAttribution: MrMaksimize commentedthis is my first patch, so be forgiving on the title. but you can rip on everything else :)
Comment #2
MD3 CreditAttribution: MD3 commentedGreat 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.
Comment #3
MrMaksimize CreditAttribution: MrMaksimize commentedOH sweet!! Switched it to needs review so the bot and maintainer can take a look :)
Comment #4
MD3 CreditAttribution: MD3 commentedAwesome! 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.
Comment #5
BerdirThanks 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?
$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 ;)
Two * and missing ().
This can't work, needs to be a module_load_include().
No need for $Id$'s anymore. Looks like a left-over...
Should only be indented two spaces I think.
tabs here instead of two spaces.
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? :)
Looks like it is, no idea what that's supposed to be.
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.
error_log() looks like a debug left-over.
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.
Comment #6
MD3 CreditAttribution: MD3 commentedOk. 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.
Comment #7
bhavin.ahya CreditAttribution: bhavin.ahya commentedIf 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