Expose Flag Service
Shongrunden - October 9, 2007 - 18:56
| Project: | Flag |
| Version: | 6.x-2.x-dev |
| Component: | Miscellaneous |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Amitaibu |
| Status: | closed |
| Issue tags: | services integration |
Description
This patch exposes a method to flag content using the flag_content module.
The new method available is:
- flag_content.flag
This patch depends on a patch I submitted to the flag_content module at http://drupal.org/node/173292
This patch file represents creating two new files (flag_content_service.module, flag_content_service.info) in a new “flag_content_service” folder in services/services.
| Attachment | Size |
|---|---|
| flag_content_service.patch | 2.51 KB |
#1
#2
Thanks for this Steve ...
I'm setting this to "won't fix" because we're only including Drupal core functionality in Services core, with the exception of Views, of course. If you or anybody else needs this, please consider making it its own contrib module, or including it inside of the flag content module.
Scott
#3
I know this was initially about flag_content module, but flag module is well worth supporting.
@Shongrunden: what do you think about adding flag support?
#4
See comment #2 - this is not part of core drupal and hence will not be considered as part of the services module. You are more than welcome to create your own flag_service as a stand alone project on d.o or indeed as an extension to the flag module.
Thanks marc
#5
Onward we march, waving our banner high!
#6
If you're willing to put views support in, then I think it's highly worth considering putting in Flag support. Flag will be on d.o, and it's probably the second most core-worthy module not currently being looked at for D7 yet.
Even if you don't end up putting it in, please let other developers work with you in your issue queue under "feature request" until it is ready to be separated. (fyi, Shongruden doesn't have cvs privileges... yet).
#7
The question isn't "Is it worth integrating with Flag?" the question is "Where does the code go?" Services has a standard that says going forward we will only expose core functionality, rather than introducing third-module dependencies into the package. Yes I know Views is in there, that happened early on, and as the maintainers determined the overhead to supporting contrib, it was decided that third-party module integration should become patches in those modules' queues. (I wasn't actually a maintainer at the time, but this is my impression, please correct me if I'm wrong.)
I will ping quicksketch later and see how he feels about a patch to Flag for Services. It wouldn't be hard to write.
One thing that would make the third party integrations easier is if our framework was a little more pluggable likes Views - just create a .inc file and drop it in the right place rather than creating a whole new module as you need to now. I'm looking into this for Deploy and I'll see if I get any ideas.
#8
None drupal core functional so this being set back to won't fix see comment #2.
#9
Moving to Flag module since quicksketch said he's open to including it.
#10
Walkah posted a helpful starting module for flag as a service in #503416: Add a Service to Flag, which I marked duplicate.
#11
subscribe
#12
* Added action to "flag/ unflag content"; "Check if a content is flagged".
* Removed white space and fix typo in flag.inc
#13
Thanks Amitaibu! This looks pretty good, a couple questions/comments:
- Do we need another permission for "access flag service"? All flag access is already checked per-user, so they can't have access to any flags than they do through the normal site. I'd like to avoid adding such permissions if possible.
- We should use $flag->access() for access checking so that the new hook_flag_access() hooks are fired.
- We should remove all the extra white space at the end of the file.
This is a good fix for the time being, but it'd be good to get a patch submitted to services if it has an autoloading mechanism that we can improve.
#14
> Do we need another permission for "access flag service"
You are right. Services allows creating access control through the user of security keys, where the admin decides which services are available. In other words - removed.
> We should use $flag->access()
Right, forgot about it. Done.
[EDIT: removed wrong sentence]
> but it'd be good to get a patch submitted to services
#597206: Allow modules to include a service file ina subdirectory, although services commit rate is very slow... :/
#15
As an fyi http://drupal.org/node/597206 is now committed :)
#16
Thanks marcingy.
re-rolled patch with services change.
#17
I tried out this patch but I couldn't get it working properly. I updated it to remove unnecessary changes (whitespace, plus changes to the install file) that have already been fixed. Regardless, it now needs to be updated to take into account session IDs added by #271582: Allow anonymous users to flag content.
#18
#19
oops, last patch was missing an inc file...
Here's a re-roll. Is there really a special thing I need to do about anonymous sessions?
I've also attached screenshots on how to setup Services, as it cab be a bit tricky.
#20
Another parameter has been added to the flag() function (and most other functions) for the SID (session ID) which helps Flag discriminate between multiple anonymous users, since they all have the UID 0. This $sid parameter should be exposed in Services so that anonymous users can Flag through services.
#21
Thanks Amitaibu, those screenshots definitely helped. I realized I forgot to include my reroll. There were some formatting things I fixed in this version (not a fan of the even-indentation in the services hooks). Following along the screenshots I could get it working great.
Regarding the anonymous SIDs, I realized that we don't actually have the option to pass in an SID at all. I'm not sure that's really a problem though (it'll use the current anonymous user).
So I think this is ready to go, I'll give you another once-over Amitaibu.
#22
Ah, I just fixed one more problem where access should be denied if trying to register a flag on behalf of a user that does not exist.
#23
I think the account check is redundent, as we already do it in flag_service_flag_access()
+ if ($uid) {+ $account = user_load($uid);
+ }
+ else {
+ global $user;
+ $account = $user;
+ }
+
+ if (empty($account) || !$flag->access($content_id, $action, $account)) {
+ // User has no permission to use this flag.
+ return FALSE;
(i.e. the
if (empty($account)part)In #19 I have changed to:
+function flag_service_is_flagged($flag_name, $content_id, $uid = NULL) {+ $flag = flag_get_flag($flag_name);
+ return $flag->is_flagged($content_id, $uid);
+}
I think it won't return a PHP notice if $uid == NULL (I saw it somewhere in core doin the same).
p.s. took me long time to write this comment as my 10 days old baby is sleeping on me :D
#24
Ha, congratulations!!
The
if (empty($account)is to ensure that an account actually is loaded,$account = user_load($uid);can fail if $uid does not exist, which was causing all kinds of PHP errors. We probably shouldn't use the current user if a $uid was intended to be passed in.#25
I think I don't get your point. UID is an optional paramter, and if not supplied then we use the current user -- this is the behaviour we wish to preserve.
If the account didn't load properly, maybe because user doesn't exist, then services will return access denied. Can you please re-explain where you think it's not working as should?
#26
Say you're user #1, and using the service API you're trying to register a flag on behalf of user #2. So $uid = 2.
+ if ($uid) {+ $account = user_load($uid);
+ }
Unfortunately user ID 2 *never* exists on a Drupal site, so this call sets $account = NULL. Now instead of registering a vote on user 2, $account is NULL and so the flag will be registered from user 1, when access should have been denied.
#27
But access will be denied:
+ if (empty($account) || !$flag->access($content_id, $action, $account)) {+ // User has no permission to use this flag.
+ return FALSE;
Account is NULL so we return FALSE and that's where it ends - it doesn't continue and try to flag the content.
#28
Er, right. I'd been explaining why we have the
if (empty($account))part, since you said, "I think the account check is redundent, as we already do it in flag_service_flag_access(). (i.e. the if (empty($account) part)".#29
sorry, my bad, I didn't explain myself properly.
The redundent part is
+function flag_service_flag($flag_name, $content_id, $uid = NULL, $action = 'flag', $skip_permission_check = FALSE) {+ global $user;
+ if (!empty($uid)) {
+ if (!($account = user_load($uid))) {
+ // User was not loaded.
+ return FALSE; // <----- we don't need this part.
+ }
+ }
+ else {
+ $account = $user;
+ }
We don't have to recheck the account exists, as we already did it in flag_service_flag_access()
#30
Ah, right! Now that makes perfect sense!
Here's a reroll that greatly simplifies flag_service_flag(). I also realized that there's no reason to put the access callback in the .module file, since the .inc is pulled in by Services module to call the access callback (which is nice). So now the only thing in the .module file is the hook_service(), everything else is in flag.services.inc.
#31
Tested and working.
One minor question I'm not sure:
flag_service_is_flagged($flag_name, $content_id, $uid = NULL) {...
// Prevent PHP notice in case we are calling user ID if it wasn't supplied.
return !empty($uid) ? $flag->is_flagged($content_id, $uid) : $flag->is_flagged($content_id);
But maybe since we have $uid = NULL in the function's signature we can safely?
return $flag->is_flagged($content_id, $uid);#32
I imagine this was because of the way Services calls its callbacks. It should work without the conditional there, but I wasn't sure if Services was passing something in even though the parameter was optional.
#33
Well I removed that conditional and I couldn't find any problems with it's removal. Removed the conditional and committed. Yay, one patch closer to a 2.x beta!
#34
Automatically closed -- issue fixed for 2 weeks with no activity.