JSON menu callback for project issues

this feature request is not totaly clear to me.

I can think of two parts:

1.) receiving issues - this is now possible through RSS

2.) updating issues or creating issues through the blog-API

the second part is not done now.

So the issue could be renamed or moved to express the second part.

Best
Thomas

Subscribe (drush could use this)

The specific implementation probably won't just be XML-RPC (if we support that at all). But yes, there are all sorts of things that would be made better if there were (a) web service(s) to interact with the issue queues:

http://groups.drupal.org/node/126529
http://3281d.com/projects/remote-issue-queues

Attached is a patch which enabled node/nid/json to deliver a JSON representation of an issue to the browser. There is a hook at the end of the menu callback where modules can enrich the JSON. comment_upload uses this hook to add information about followups, attachments, etc.

This was prompted by the upcoming drush issue queue commands. See #1078108: Drush issue queue commands.

Lets do a *little* bikeshedding on the JSON contents and then push this along. We should do a write web service as well, but I imagine we'll wait on the D8 web services initiative for guidance. Lets do more complex services in a different issue.

Example output from my tiny drupal.org instance:

{
    "json-version": "1.0",
    "title": "dev1",
    "description": "sdfsdf",
    "id": "2",
    "url": "http://mm/dorg/node/2",
    "component": "Code",
    "priority": "1",
    "version": "0",
    "assigned": "0",
    "status": "1",
    "project-id": "1",
    "project-title": "devel",
    "project-url": "http://mm/dorg/project/deve",
    "project-project": "deve",
    "attachments": {
        "2": {
            "1": {
                "contributor": "1",
                "url": "http://mm/dorg/sites/default/files/issues/1.csv",
                "comment-id": "2"
            }
        }
    },
    "contributors": {
        "1": {
            "name": "admin",
            "uid": "1",
            "url": "http://mm/dorg/user/1"
        }
    },
    "comments": {
        "2": {
            "contributor": "1",
            "url": "http://mm/dorg/node/2#comment-2",
            "status": "0",
            "thread": "02/"
        }
    }
}

subscribe

Better access callback for the JSON menu item.

This is very cool. I am having problems with the output, though. When I try to parse it with json_decode(), I get a NULL result whenever there is i18n characters in the output, as is the case with sample issues generated by drupalorg_testing, for example. With 7-bit output, it works great.

I find this to be a bit confusing, because you compose the output with drupal_json(), which uses json_encode(). Shouldn't json_encode and json_decode compliment each other, and work regardless of the encoding of the input?

In the comment_upload patch, we need separate queries. The delete query needs to delete any and all files uploaded, whereas the json_alter query should filter by comment status, at minimum.

+++ b/includes/project_issue.json.inc
@@ -0,0 +1,33 @@
+function project_issue_json($node_issue) {
+  if ($node_project = node_load($node_issue->project_issue['pid'])) {

Elsewhere, pi calls these variables simply $issue and $project for clarity. Let's be consistent here.

+++ b/includes/project_issue.json.inc
@@ -0,0 +1,33 @@
+      'description' => $node_issue->body,

Not sure whether we really want or need to output the full body...? Would have to be formatted/rendered with check_markup().

However, I'm not able to see a use-case for that currently (not even in Dreditor). Much more important and interesting are all the other meta properties, as already contained here. Perhaps leave out for now and only add later, if necessary?

+++ b/includes/project_issue.json.inc
@@ -0,0 +1,33 @@
+      'project-project' => $node_project->project['uri'],

Let's use 'project-name' here.

+++ b/includes/project_issue.json.inc
@@ -0,0 +1,33 @@
+    drupal_alter('project_issue_json', $return);

I'd like to additionally see meta-data for the comments on the issue; i.e., not only for comments having attachments. However, as with node body above, I'm only thinking of meta data for comments; i.e., as in your example output, only cid, uid, and thread. (url can be rebuilt manually, I think)

Also love the 'contributors' (or 'users') reference list. uid and name definitely make sense there.

+++ b/project_issue.module
@@ -193,6 +193,16 @@ function project_issue_menu() {
+  $items['node/%node/json'] = array(

Ideally, we'd use .json, but I'm not sure whether D6's menu system properly understands %node.json (guess not).

@greg.1.anderson: I guess you need to use drupal_json_decode() to properly decode the generated output. Drupal's JSON format is buggy currently, there's an open and still unresolved core issue for that.

@sun: Thanks for the clarification. It's a bit unfortunate about drupal_json_decode; in drush, Drupal APIs are only available when a site is bootstrapped, and some of the issue queue commands are useful in a non-bootstrapped context. I'll look at drupal_json_decode and see if maybe we can just take a copy of it in drush.

The attached patches made the following changes:

+ Added missing 'component' field
+ Removed 'description' element
+ Normalized the attachment array so that the urls are all containd in a single array. This is more convenient for clients such as the drush issue queue commands, and also reduces repetition of metadata in the output.
+ I renamed $issue and $project variables from $node_*.

I agree that we should make multiple db queries so that we get metadata info for all comments, not just comments with attachments, but ran out of time to make that adjustment.

Here is an update of #11 that includes a separate query for comments, so that comments w/out attachments will appear in the issue info json.

Updated #1078108: Drush issue queue commands to use this.

In my previous patches I accidentally overwrote previous entries in the 'urls' element, so there was never more than one attachment included in the json output. This is fixed in this patch.

Please also use with project_issue_12.patch, above.

Code is looking great. I have added the check for published comments in comment_upload query.

#15 looks same as #14; maybe uploaded wrong file?

Scan for COMMENT_PUBLISHED. That's the bit that changed.

Sorry, I get it now -- you are filtering out the unpublished comments, not marking the state. That makes sense.

Changing the query in comment_upload_fetch_all isn't right, though, as this will prevent nodeapi('delete') from removing attachments from deleted, unpublished comments.

We'll want to keep unpublished comments out of the data structure built in the project_info patch too.

New patches with fixes for issues described in #19.

That works for me ... I pinged netaustin about this issue since he is ostensibly comment_upload maintainer. That module is infrequently maintained, unfortunately.

+++ b/includes/project_issue.json.inc
@@ -0,0 +1,47 @@
+      'title' => $issue->title,

I wonder whether we need a check_plain() here?

+++ b/project_issue.module
@@ -193,6 +193,16 @@ function project_issue_menu() {
+    'type' => MENU_LOCAL_TASK,

Needs to be a MENU_CALLBACK.

+++ b/project_issue.module
@@ -259,6 +269,10 @@ function project_issue_menu_access($type) {
+function project_issue_menu_access_task($node) {

1) Should be renamed to project_issue_menu_access_issue_view().

2) Missing phpDoc "Menu access callback; Checks whether $node is an issue and the user is allowed to view it."

+++ b/comment_upload.module
@@ -194,6 +194,35 @@ function comment_upload_file_download($filepath) {
+function comment_upload_project_issue_json_alter(&$issue) {

Missing phpDoc "Implements hook_project_issue_json_alter()."

+++ b/comment_upload.module
@@ -194,6 +194,35 @@ function comment_upload_file_download($filepath) {
+  $result = comment_upload_fetch_all($issue['id'], FALSE);
...
+function comment_upload_fetch_all($nid, $include_unpublished = TRUE) {

I'd suggest to reverse the default argument value for $include_unpublished for security; these are public general purpose modules and developers may sloppily call the function, forgetting to set the second argument to FALSE.

I had another thought about this the other day. Should we perhaps make the path "node/%/project_issue.json"? Seems that "node/%/json" is rather general; this patch would get in the way of any other module that also wanted to use the same path. I'm not sure what Drupal does elsewhere for for json paths, but based on #6 I am guessing they usually end in ".json", and since this info is specific to project issues, I thought that a more specific path using that term would be appropriate.

+1 to everything that sun said in #22.

In addition,

1) I would like to see check_plain on both the issue and project title.

2) Lose the 'weight' from hook_menu.

3) Please document hook_project_issue_json() in project_issue.api.php.

4) In comment_upload_fetch_all please follow sun's suggestion and reverse the unpublished option.

Re #23, I'm mixed on this, since there are lots of theories on content negotiation. In effect we're just saying to give a json representation of the issue, and if we were going to have a generic version for all nodes, followed by a version for issues, I'd rather see it prefixed as issues/%node/json or something along those lines. Not sure if that makes sense or not, but I'd rather make the identifying bit come up front before the resource ID and format.

Bonus points if any one can figure out a clean way to make this work at node/%node just using Accept: text/json headers ;)

Maybe we should use the services module. Version 3.x supports json output standard. Our URL would then be: http://drupal.org/services/plist/project_issue/112805 to get the json project issue info for this issue.

Upside: Standard. We could use POST and PUT events to create and edit issues as a future enhancement.

Downside: We'd have to get d.o. to accept the services module, or this feature would not be available. (Presumably we'd wrap the endpoint definitions in if (module_exists('services')) { ... if we went this route.)

Attached patch implements all of sun and mikey's feedback except for check_plain(). One could make arguments either way here. I'm slightly favoring omitting those calls. I consider these JSON responses to be data interchange. As such, we aim to send raw data. It is up to consumer to make the data safe for its chosen output format. Assuming HTML is unhealthy, IMO. Drupal itself doesn't assume HTML or else we would not have stored the unfiltered text in our DB.

Sorry I've been offline for nearly a month, just catching up now on things...

This is mostly looking great! A few concerns:

project_issue.patch

A) As sun pointed out in #9:

+++ b/includes/project_issue.json.inc
@@ -0,0 +1,33 @@
+function project_issue_json($node_issue) {
+  if ($node_project = node_load($node_issue->project_issue['pid'])) {

Elsewhere, pi calls these variables simply $issue and $project for clarity. Let's be consistent here.

If we do anything more specific, we'd call these $project_node and $issue_node (that's basically what we do in project_release, for example).

B)

+      'priority' => $node_issue->project_issue['priority'],
+      'version' => $node_issue->project_issue['rid'],
+      'assigned' => $node_issue->project_issue['assigned'],
+      'status' => $node_issue->project_issue['sid'],
<code>
All of these are just ints, and not necessarily all that useful to other sites in all cases.  Maybe we want something like this?
<code>
     'priority-id' => $issue->project_issue['priority'],
     'priority' => project_issue_priority($issue->project_issue['priority']),
     'version-id' => $issue->project_issue['rid'],
     'version' => //$release = node_load($issue->project_issue['rid']); $release->title,
     'assigned-uid' => $issue->project_issue['assigned'],
     'assigned-name' => //$assigned_user = user_load($issue->project_issue['assigned']); $assigned_user->name,
     'status-id' => $issue->project_issue['sid'],
     'status' => project_issue_state($issue->project_issue['sid']), // evil function name, see below

Re: project_issue_state() see #353248: Standardize terminology on either 'status' or 'state', not both.

The version and assigned-name pseudo code is obviously bogus as written, but hopefully you get the idea. Also note that in both cases, the int might be 0 (or perhaps even NULL in the case of version-id, I forget off the top of my head) so we need to code defensively for that.

Anyway, I think you get the idea. Transforming these ints into human-useful data has to happen on the site where these JSON callbacks live, and either we need a separate JSON lookup service or we just need to do the transformations as part of the same request. Seems better for both the server and clients to do them all at once instead of requiring clients to make separate requests. In fact, we're already applying this principle to the project that the issue is attached to, so it seems like a no-brainer to do the same for the issue metadata itself.

C) This drives me nuts:

\ No newline at end of file

;)

D) If you have this:

+    drupal_alter('project_issue_json', $return);

we need a hunk for project_issue.api.php to document it. I *believe* this is the first hook project_issue will be invoking (and a quick grep seems to agree), hence the need to add a new file.

E) This is missing a PHPdoc comment:

+function project_issue_menu_access_task($node) {

(although it's so damn self-documenting, I'm not sure I really care, but standards are there for good reasons and I'd like to try to keep moving Project* towards following them).

comment_upload.patch

F)

+function comment_upload_project_issue_json_alter(&$issue) {

Calling this variable "$issue" seems to invite confusion, especially given A. ;) I'd rather this was $json or maybe $issue_json so it's more obvious what we're altering. I could for example see needing to node_load() the actual issue in one of these alter hook implementations, so $issue_json and $issue_node seems better.

G)

+    $issue['attachments'][$row->cid]['contributor'] = $row->uid;

Same as B above about user_load(). Not sure we need it, but might be useful. I'm open to hearing what others think.

H)

+ * @param $include_unpublished
+ *   If TRUE, return all nodes; if FALSE, only return nodes where status == COMMENT_PUBLISHED

This comment doesn't match itself, nor the code. ;)

I)

+  if ($include_unpublished) {
+    return db_query("$q ORDER BY cu.cid ASC, fid ASC", $nid);
+  }
+  else {
+    return db_query("$q AND c.status = %d ORDER BY cu.cid ASC, fid ASC", $nid, COMMENT_PUBLISHED);
+  }

It's weird to duplicate the whole ORDER BY clause just to get a conditional WHERE. I'd rather we either created a single $order_by variable that's shared, or just conditionally build up WHERE (which is typical in most of the rest of Drupal) and use an array for all the query placeholder values.

Lots of these are on the more pedantic end of the spectrum, and I don't want the perfect to be the enemy of the good. The main thing I think this "needs work" for is B. But if someone's going to re-roll anyway, I'd love to see the others fixed while we're at it.

Thanks again!
-Derek

p.s. I'm 100% in support of moshe's stance on the check_plain() from #27, I just failed to mention that in my reply. It's implicit in that it didn't show up as point J or something, but I wanted to be explicit about it. ;)

Oy. I had reuploaded an old project_issue.patch. Here is the one I meant to upload. I will review dww's list and post another tomorrow.

Oy. ;) Well, looks like most of my feedback is still valid, even on the newer patch, so it wasn't a total waste of a good review.

Other thoughts with a fresh pair of morning eyes:

J) Do we want any timestamps anywhere in this JSON output? Issue creation? Comment timestamps? Seems potentially useful.

K) Do we want filesize in the comment_upload altered output? That's another thing I tend to look at when scanning an issue and I could see wanting/needing to know for some reason. Could be punted to a separate issue/patch if we wanted to.

L) What's the story with json-version given that there's the alter hook? Anyone who alters the output is supposed to also alter the json-version? Folks who alter shouldn't mess with it? Seems fragile. Either way, I think the expectation should be documented in project_issue.api.php.

M) We probably want the testbot status for each attachment in the JSON output, too. This should *definitely* move to a separate issue (in the pift issue queue) but wanted to mention it here while I'm thinking about it.

N) Don't we want a menu loader placeholder here to ensure this path only matches issue nodes?

+  $items['node/%node/json'] = array(

I.e. something like this:

$items['node/%project_issue_node/json'] = array(

and then one of these:

function project_issue_node_load($arg) {

@see project_node_load() in project.module for example. There's no real extra cost since we already need to do the node_load() anyway...
Doh! Scratch that. I see you snuck this into the menu access checking function. Sneaky! And now that it's documented (E is done) it even says so in plain English so it's more obvious for other folks to notice, too. ;)

O) What about taxonomy terms associated with issue nodes? In our case, those are issue tags, which sometimes are really critical to understanding the status of an issue.

P) What about files attached to the original issue post? Sadly, those aren't handled the same way as files attached to comments. Seems like we need to handle that directly in the main project_issue_json() function on behalf of core's upload.module.

Q) What about the author of the original issue? I see we're doing all this stuff about comment contributors, but nowhere do we know who wrote the original node.

Implemented dww's feedback. I didn't add the -name elements in comment_upload (G) because those names are already in the 'contributors' element. I did add name (and url) for Assigned since that user isn't necessarily a Contributor.

F) was only partially implemented, and this function is now broken:

+function comment_upload_project_issue_json_alter(&$issue_json) {
+  $result = comment_upload_fetch_all($issue_json['id'], FALSE);
+  $account = user_load($row->uid);
+  while ($row = db_fetch_object($result)) {
+    $issue['attachments'][$row->cid]['contributor-id'] = $row->uid;
+    $issue['attachments'][$row->cid]['comment-id'] = $row->cid;
+    $issue['attachments'][$row->cid]['urls'][$row->fid] = file_create_url($row->filepath);
+  }
+}

Also, looks like nothing I wrote in #31 (e.g. points J-Q) has been addressed. Do we care?

Thanks for sticking with this!
-Derek

Now I've looked at J-Q. Here's my proposal:

J) added all timestamps. nice catch.

K) Punting on this for now. I'd have to complicate the array another level. Client can get this with a second request if he really needs it.

L) json-version is not meant to be changed by modules like comment_upload. those modules just enrich existing data so they are API compatible. In general, I only expect project_issue itself to mess with this. Added text to api file.

O) Punting on taxo terms since we'd want to term_load() each one for its name. If folks think this is important, I'll do it.

P) Added.

Q) Added

Sweet, thanks! I think punting on the ones you're punting on is fine.

However, as I was pondering this the other night, I was wondering if what I wrote at N in #31 (and crossed out) is actually valid. Even if the menu access callback provides access denied on paths where the node isn't an issue, isn't this still a problem? First of all, seems like a 404 would be more appropriate than a 403 in this case. More to the point, what if some other module wanted to do the same trick and provide a node-type-specific JSON callback at node/N/json? If they both claim their menu item lives at node/%node/json isn't the menu system itself going to be confused with multiple items trying to live at the same path? Isn't it safer and better to just do what I said and use a node-type-specific menu loader placeholder so that the paths are in fact different if multiple modules try to do this? I can easily imagine wanting node/%project_node/json in the near future, and probably node/%project_release_node/json not far behind...

Yeah, based on an IRC chat with chx and mikey_p, #31.N isn't going to work, either. Since the %project-issue-node just gets converted back to % by the D6 menu system. We apparently only have two options here:

- Have a single menu item that lives at node/%node/json that then looks up the node type and hands off control to the right module that "owes" the node type in question. Not sure what module should provide this menu item.

- Have separate menu items for each module trying to provide this data. Something like node/%node/project-issue/json for issue nodes, etc. Then everyone's responsible for their own (non-conflicting) menu items.

p.s. If we stick with node/%node/json as a single path, I guess we can just roll this out for now, and then refactor it all into a shared menu item once we have another node type trying to provide json data. Not ideal, but then again, I don't want perfect to be the enemy of progress.

IMO, we should keep the URLs simple and do the gymnastics in the menu system (D7) or page callback (D6) to route to proper module. By the time we get a second instance of this, we might be on D7 or have a need for Services, or whatever. So, I propose to go with latest patch. See YAGNI.

The only problem with applying YAGNI here is that once we deploy this as node/%node/json and clients (drush, dreditor, whatever) start using it, if we change it later, we're going to break all the clients. Or, vastly more likely, we'll be painted into a "well, we just have to keep it at that URL so we don't harm the clients" and we'll be stuck with this path forever. It's easy to change this path now. It's hard to change it later. That's different than "we think we'll need something later so let's code it up now".

Anyway, assuming we can always find a way (even if ugly) to make it work at node/%/json for the rest of time, I'm okay rolling this out as a hack now and fixing it once we start having collisions (as I wrote in #37).

I think of it as painting ourselves into a happy corner, with clean simple colorful walls all around ... That sounded a bit like an RTBC, so tentatively changing status. Let me know if I can help with getting this committed or deployed.

@Greg - a couple element names changed so drush commands will need a reroll.

First of all, seems like a 404 would be more appropriate than a 403 in this case

i re-read the W3C definitions for 403 and 404 -- it's debatable which one we should use here, but i lean towards the existing 403, as in "you're not allowed to get the issue json of this non-issue".

however, i'm pretty strongly opposed to using node/%node/json as the path for this -- it's too basic a path for any of the project* suite to be providing, and is begging for conflicts. i don't see any downside to using either node/%node/project-issue/json or node/%node/project-issue-json -- they avoid future namespace conflicts, and provide a sensible solution for which module should provide the path. furthermore, they are self-describing urls, whereas node/%node/json reads more like "hey, whatever node is here, this is where you get the json for it."

Well, I actually think that hey, whatever node is here, this is where you get the json for it. is the right way to go. But I'm weary so here is same patch using node/%node/project-issue/json. The comment_upload part of this issue still in #34.

+++ b/project_issue.moduleundefined
@@ -231,9 +231,18 @@ function project_issue_menu() {
-    'type' => MENU_CALLBACK,
+    'type' => MENU_CALLBACKLBACK,

I think this is a typo.

Just removing that change.

I've applied #44 PI patch with the #34 comment upload patch to a local install of the testing profile, and it seems to work thus far. When I visit an issue node json path, e.g. node/287/project-issue/json, I get JSON output like so:

{ "json-version": "1.0", "title": "Paratus Antehabeo Cogo Similis", "id": "287", "created": "1320962498", "changed": "1320967237", "submitter-id": "15", "submitter-name": "auth1", "submitter-url": "http://localhost:8888/drupalorg/user/15", "url": "http://localhost:8888/drupalorg/node/287", "component": "Documentation", "category": "support", "priority-id": "1", "priority": "critical", "version-id": "191", "version": "af 4.7.x-1.x-dev", "assigned-id": "19", "assigned-name": "caricicruvo", "assigned-url": "http://localhost:8888/drupalorg/user/19", "status-id": "2", "status": "fixed", "files": [  ], "project-id": "104", "project-title": "Afrikaans Translation", "project-url": "http://localhost:8888/drupalorg/project/af", "project-name": "af", "contributors": { "14": { "name": "gitvetted2", "uid": "14", "url": "http://localhost:8888/drupalorg/user/14" }, "23": { "name": "todetrar", "uid": "23", "url": "http://localhost:8888/drupalorg/user/23" } }, "comments": { "87": { "contributor": "14", "url": "http://localhost:8888/drupalorg/node/287#comment-87", "created": "1320962504", "status": "0", "thread": "01/" }, "92": { "contributor": "23", "url": "http://localhost:8888/drupalorg/node/287#comment-92", "created": "1320962505", "status": "0", "thread": "02/" } } }

If I try to visit a handbook node with the same path pattern, I get an access denied message.

With some comment file attachments:

{ "json-version": "1.0", "title": "Vulputate Lobortis Haero Haero Cogo Dignissim Commodo", "id": "301", "created": "1320962499", "changed": "1320974239", "submitter-id": "0", "submitter-name": "", "submitter-url": "http://localhost:8888/drupalorg/user/0", "url": "http://localhost:8888/drupalorg/node/301", "component": "Miscellaneous", "category": "support", "priority-id": "1", "priority": "critical", "version-id": "202", "version": "project 4.7.x-1.0", "assigned-id": "0", "assigned-name": "", "assigned-url": "http://localhost:8888/drupalorg/user/0", "status-id": "16", "status": "postponed (maintainer needs more info)", "files": [  ], "project-id": "105", "project-title": "Project", "project-url": "http://localhost:8888/drupalorg/project/project", "project-name": "project", "contributors": { "5": { "name": "site1", "uid": "5", "url": "http://localhost:8888/drupalorg/user/5" }, "53": { "name": "rabrilouuv", "uid": "53", "url": "http://localhost:8888/drupalorg/user/53" }, "18": { "name": "frucri", "uid": "18", "url": "http://localhost:8888/drupalorg/user/18" }, "1": { "name": "admin", "uid": "1", "url": "http://localhost:8888/drupalorg/user/1" }, "16": { "name": "auth2", "uid": "16", "url": "http://localhost:8888/drupalorg/user/16" } }, "comments": { "9": { "contributor": "5", "url": "http://localhost:8888/drupalorg/node/301#comment-9", "created": "1320962500", "status": "0", "thread": "01/" }, "81": { "contributor": "53", "url": "http://localhost:8888/drupalorg/node/301#comment-81", "created": "1320962504", "status": "0", "thread": "02/" }, "98": { "contributor": "18", "url": "http://localhost:8888/drupalorg/node/301#comment-98", "created": "1320962505", "status": "0", "thread": "03/" }, "101": { "contributor": "1", "url": "http://localhost:8888/drupalorg/node/301#comment-101", "created": "1320968392", "status": "0", "thread": "04/" }, "102": { "contributor": "16", "url": "http://localhost:8888/drupalorg/node/301#comment-102", "created": "1320974221", "status": "0", "thread": "05/" }, "103": { "contributor": "16", "url": "http://localhost:8888/drupalorg/node/301#comment-103", "created": "1320974239", "status": "0", "thread": "06/" } }, "attachments": { "101": { "contributor-id": "1", "comment-id": "101", "urls": { "155": "http://localhost:8888/drupalorg/sites/default/files/issues/stream_wrappers-allow_symlinks-1008402-40-d7.patch" } }, "102": { "contributor-id": "16", "comment-id": "102", "urls": { "162": "http://localhost:8888/drupalorg/sites/default/files/issues/tac tweet.txt" } }, "103": { "contributor-id": "16", "comment-id": "103", "urls": { "163": "http://localhost:8888/drupalorg/sites/default/files/issues/gdo_bug_1.png" } } } }

1. what's the rationale for using 'id' instead of 'nid', 'uid', and so forth? using the latter seems more accurate to me, are you trying to preserve some flexibility there?
2. 'version-id' => $issue->project_issue['rid'], <-- that should be using a check on $release
3. 'assigned-id' => $issue->project_issue['assigned'], <-- should also be using a check on $assigned_user
4. as a matter of fact, i think it would be cleaner to use the $release, $assigned_user, and $submitter_user vars created earlier to do all the relevant checks for the existence of optional settings. since node_load() user_load() both return FALSE on failure, a straight if() check seems most appropriate.
5. shouldn't 'status' be 'state'? project_issue_state() seems to suggest so, and i'm *pretty* sure we standardized on state in the code base already?
6. 'url' => url('user/' . $row->uid, array('absolute' => TRUE)), <-- this could use the $options var created earlier, for consistency.
7. 'url' => url('node/' . $return['id'], array('absolute' => TRUE, 'fragment' => 'comment-' . $row->cid)), <-- i'd rather see that use $issue->nid instead of $return['id'], seems more straightforward -- had to do a double take to figure out that line.

Re: #47.5: No, #353248: Standardize terminology on either 'status' or 'state', not both. was moving towards "status" not "state".

Thanks for the review,
-Derek

1. I think that many consumers of these feeds will not be Drupalists, and this standardizing on id is clearer for them. For Drupalists, id-specific names are perhaps slightly clearer. Could go either way here. Left unchanged for now.

Implemented everything else. The comment_upload patch is unchanged, so see #34.

talked this over with dww, and there's one more big thing we need to take care of here: backwards compatibility.

while in the drupal world we don't have much respect for it, in this particular case we're exposing the data to non-drupal clients, so it needs to be accounted for. simply broadcasting what api version we're on isn't good enough, as major api version changes could break clients.

what we'll want to do is something like:

1. provide a query parameter like api_version=[major version number].
2. if a client wants to stay locked into a major version, they pass this parameter
3. if the parameter is not passed, then the latest api version is returned.
4. a separate function that returns the latest minor release of each major version (ie, we don't need to support every minor release, only the most recent, since they don't break backwards compatibility)
5. internally, we'll need code to detect this parameter, and to correctly map to the right function to return what the client wants.

so, to commit this code, we'll need at least the minimal infrastructure to support this from the client side. it would probably be nice to frame out the callback code as well, but not strictly necessary since it's not client facing.

Thats a good approach. I'm wondering what minimal infrastructure really means here. Strictly speaking, this client can already pass whatever api_version he wants. The code will return the same json since we only support one version. Couldn't we add api_version support in a future release? If not, then please elaborate on the most minimal we need to do here.

i'd say most minimal would be:

1. some client doc on how to use this interface, which would include how to use the api_version parameter. this is so they can build their requests from day one using that parameter if they choose.
2. a TODO: at the appropriate spot in the project_issue code (with a link pointing to the relevant comment in this issue) that indicates what change will need to be made if/when we add another major api version.

Added a README section (and a little sectioning) and a comment at top of page callback for json.

i think we're pretty close. couple of small things:

1. went back and had a look at the comment upload patch, and found this line: + $account = user_load($row->uid);. pretty sure that needs to be removed.
2. i think the comment for the json menu callback would be more helpful if we had a line in there about when we'll need to pay attention to that TODO. something like "When the major API version of this callback changes, it will need to be refactored to..."
3. The first paragraph of the help text for the callback feels like it might be a bit confusing for first-time readers. here's an alternate version for consideration:
   

   In addition to an HTML page at node/[nid], each issue's metadata may be viewed
   in JSON format at node/[nid]/project-issue/json.  The following query
   parameters are supported:
     api_version=[major]: If passed, the specified major version of the JSON API
                          will be used for the call. If not passed, then the
                          newest version of the API will be used.
   The current API Version is 1.
   

Yup, thats better. Implemented all suggestions.

Is it technically possible to include the metadata changes for each comment too? I.e. in the previous comment the issue went from 'needs work' to 'needs review'. Seems like that might be useful data to include if it's not a horrific challenge to do so.

That can be easily derived. I'd rather not burden the server with that. Please open a separate feature request for that if you really want server to provide. Thanks.

Please open a separate feature request for that if you really want server to provide.

Done: #1421268: Comment meta attributes in JSON callback

Release data is exposed through a separate interface; the data is in XML. There is code in Drush to parse it; you could look there for an example. Edit: To be clear, this is the same data that the update module uses. In the past, Drush used the update module to determine release info, but it now goes directly to the release xml, and pulls it via http.

found a couple more problems while doing final testing:

1. trailing whitespace in README
2. setting Assigned to 'Unassigned' gives you this in the JSON:
   

         "assigned-id": "0", 
         "assigned-name": "", 
         "assigned-url": "http://localhost/drupal/project6/user/0", 
       

   clearly not right. how do we handle this case? set all these to NULL?

How about setting assigned-id to FALSE, and just leave assigned-name and assigned-url out of the record when the issue is unassigned?

Those are now NULL in the unassign case. Also removed trailing line from README.

+++ b/README.txt
@@ -24,7 +28,22 @@ Send feature requests and bug reports to the issue tracking system for
+In addition to an HTML page at node/[nid], each issue's metadata may be ¶
+viewed in JSON format at node/[nid]/project-issue/json. The following ¶
+querystring paramters are supported:

Still white line problems

+++ b/README.txt
@@ -24,7 +28,22 @@ Send feature requests and bug reports to the issue tracking system for
+Modules may alter the JSON with hook_project_issue_json_alter(). See project_issue.api.php. ¶

wl

+++ b/includes/project_issue.json.inc
@@ -0,0 +1,71 @@
+ * @todo ¶

wl

Spaces fixed.

Revised and cleaned up the code and comments. This looks ready to fly for me.

+++ b/project_issue.module
@@ -300,6 +309,13 @@ function project_issue_menu_access($type) {
   return user_access('access project issues') || user_access('access own project issues');
...
+function project_issue_menu_access_view($node) {
+  return $node->type == 'project_issue' && node_access('view', $node);
+}

The only thing I'm not sure about is whether the router access permissions are correct or sufficient.

It looks like comparable router items are using project_issue_menu_access('any'), which somehow looks as if it would bypass node access, and instead rely on simple user access permissions only.

Further tweaks:

1. Changed 'files' into 'attachments', special-casing the OP as post/comment #0, so that API consumers have a unique set of issue attachments to process.
2. Changed the order of basic issue properties for better code readability.

Sorry, forgot to update project_issue.api.php accordingly.

Thanks ... Back to RTBC.

Note to all: I just got back online after 10 days. This is a high priority patch for me. I plan to review/commit/deploy this week (inshallah!). If this isn't live by Friday, y'all have my wholehearted permission to start regularly pinging me by any means available until it's done. ;)

Thanks!
-Derek

Crap. I really, really wanted to commit and deploy this tonight. The code is mostly looking fine. I only found one problem, which is that y'all blindly took my advice from #28 and were doing this:

      'version' => $release->title,

which would be something like "project_issue 6.x-1.x-dev", when what we really need is this:

      'version' => $release->project_release['version'],

which is just the "6.x-1.x-dev" part like our issue versions really are. Fixed that locally.

HOWEVER... I realized that to deploy this, I'd also need to deploy the change to comment_upload. And that's a whole new can of worms. :( I'm not even sure that the latest code in Git in comment_upload even works. :( And there doesn't appear to be much evidence from re-reading this thread that anyone's actually tested that. http://drupal.org/project/issues/comment_upload is looking like a bit of a barren wasteland. July 14, 2009 12:00 was the last non-translation/non-Git-migration commit. So, uh, yeah. We need some love on that before this can go live. Or, we live without whatever data comment_upload was adding to the JSON.

For now, marking this postponed until some of these are resolved:
#625212: Neither 6.x-1.x-dev or 6.x-1.0-alpha5 work on a completely clean site.
#1358866: Create a stable release of Comment Upload for 6.x
#902880: Preview doesn't work + comment couldn't be saved
...

Apparently the other "maintainers" of comment_upload are all totally MIA (netaustin and heine). I guess that just leaves me. Great. :/ Anyone else interested in helping maintain that? At least it's probably going to die by D7 since comments are field-able entities. But, I really don't have the bandwidth to clean that all up on my own. Help?

Or, set this back to RTBC with a "screw the data from comment_upload" argument.

Thanks/sorry,
-Derek

p.s. Here's the commit I was in the middle of, complete with the fix I mentioned at the top of #72.

Upon further discussion with moshe, we realized comment_upload wasn't quite as broken as I thought. I think I was having a flashback to the 5.x days.

Even still, I just spent over an hour doing triage on the comment_upload issue queue, rolled a 6.x-1.0-alpha6 release (including the patch in #55 from here) and deployed that on d.o. Also committed and pushed #72, and deployed that, too.

Behold: http://drupal.org/node/112805/project-issue/json

;)

Great work, everyone! Sorry for the delays...

Awesome! I'm so glad to see this deployed on d.o.

I noticed just one small defect. If the issue contains an attachment in #0, then the attachment information contains a 'contributor' item, whereas comments #1 and onward contain 'contributor-id'. See: http://drupal.org/node/1068294/project-issue/json

Oh, additionally there is no entry for the OP (user who posted the issue) in the 'contributors' list unless that user also posted at least one comment.

I updated #1078108: Drush issue queue commands (see #60) to use the latest json schema from d.o. I don't have time to do a patch for #75 or #76 right now, but I did include workarounds for these in the Drush command.

this is awesome, thank you all

Let's fix that inconsistency.

I'd also like to change the properties to use camelCasing, as that is way more in line with JSON/JavaScript and RESTful web standards.

FWIW, a property named "anything" is expected to contain the actual anything data, whereas a property named "anythingId" denotes that it's a reference to something else.

greg.1.anderson and moshe: sun is changing a lot of stuff in here. Are y'all in favor? I don't want to just commit without sign-off from some other consumers of this data.

Thanks,
-Derek

I have not had time to test it, but I am in favor of all of the changes in #79, and I will update the issue queue commands to use the new ids as soon as they are rolled out to d.o. The issue queue commands have not been committed to Drush yet, so there's no worry about breaking existing code.

Also, I am not sure that #79 fixes #76.

OK with me.

back in #50, i clearly laid out concerns for breaking backwards compatibility. this should be respected by a) bumping the major version when backwards compatibility breaks, and b) implementing the backwards compatible api menu code based on the passed api_version parameter. the patch in #79 clearly break backwards compatibility.

i'm not saying this to necessarily create more work now -- i understand this api is fresh out, probably almost nobody is using it, etc. but skipping over the good plan we laid out for backwards compatibility the first time we break it doesn't exactly give me confidence that we'll actually stick to it... ;)

my two cents.

@hunmonk: Yes, I'm well aware of a) our plans and b) the need for respecting them. However, this service hasn't been announced anywhere, and the only changes here are bug fixes proposed by the people trying to use it for the first time. Once this is "live" and announced and documented, we can, should, and will stick to the plan around an API version number. But, it's not worth doing that for "alpha" versions of the API...

@dww: yep, makes sense, i'm totally satisfied with that reasoning -- given the general drupal culture of breaking backwards compat, i felt it necessary to at least give voice to the concern. :)

Let's get this done, please. Attached patch also fixes #76 and some additional things.

The corresponding patch for comment_upload is still the same, see #79.

Interdiff is against #79.

Looks fine to me.

Sorry for the delays. Reviewed, committed, pushed, and deployed. Behold:

http://drupal.org/node/1068294/project-issue/json

Everyone happy with that? Can we publicly announce this API now?

Also, I'd very much appreciate input from all y'all who care about this service over at #1585800: Document how to do issue JSON in D7 ...

Thanks!
-Derek

Hrm. I just noticed (both before and after this change) that the "thread" attribute on the comments seems to be rendered in hex. ;)

"thread":"0a\/"

Not sure what's up with that. Apparently no one noticed or cares. Just wanted to point it out in case it matters and we want to fix this...

Also, although I saw this when looking at the diff, I wanted to ask here, too:

Why do we have both 'id' and 'uid' (holding the identical data) on the 'contributors' array (which is already duplicate with the keys for that array)?

Finally, what gives with the 'parentId' attribute in the 'comments' array? Appears to always be 0. That can't be too helpful. ;)

Yeah, let's fix those.

IIRC, I left the additional 'uid' for BC, but I guess that's moot.

As long as threading is disabled for issue comments, there's no point in exposing that info. Can be (properly) added later, if and when required.

Also, we collectively forgot that Comment module in D6 has these funny 'status' values, which are reversed Boolean flags... (zomg)

Additionally, API consumers should be able to rely on the 'comments' and 'contributors' keys to exist in the response, even if there are none.

Updated the Drush issue queue command patch to match the latest changes on d.o. See #1078108-74: Drush issue queue commands.

Did not test with #91 -- did this just before that comment was posted. However, Drush does not use any of the info changed in that patch, so it won't affect the issue queue commands to deploy it.

Reviewed, committed, pushed and deployed. Seems sane:

https://drupal.org/node/1068294/project-issue/json

Thanks,
-Derek

FYI: next step is #1618262: Provide JSON output from issue queue listing views

Tagging.

Opened #1620952: Provide taxonomy terms (e.g. issue tags) in issue node JSON.

Found an important bug / omission with this; see: #1730728: Project issue json needs comment index number included in data structure.

FYI: In D7 we're no longer doing a bunch of custom code in project_issue for this. The plan is to just use restws. However, see #1710850-14: Deploy RestWS for D7 project issue JSON. We don't currently have development resources to try out restws, configure it, etc, and we're not considering this issue JSON a critical launch-blocking feature. Every client is going to be broken, anyway, since the JSON will have changed. So, either we'll fix this in a post-D7-launch deployment, or someone who cares about this functionality will have to run with it in the next few weeks while the rest of the d.o D7 upgrade team is working on actually critical and major functionality.

Thanks/sorry,
-Derek

I don't have time to fix the restws issue for d.o, but if as long as there will be -some- json output for https://drupal.org/node/1068294/project-issue/json, or some other issue/node-related URL, then I can easily roll new drush iq releases with the launch of the d7 d.o, and again for the restws launch, as necessary. Hm, re-reading the above, it looks like the plan is to not have any issue json until the restws version is available. I guess drush iqa will be broken for a while. Maybe I can roll back and use the original html-scraper code in the interim. :p

Yeah, we didn't port the custom JSON we added here to D7. However, there seem to be other people interested in the JSON, so perhaps someone else will get it working before we launch. It's probably only an hour of work, tops. We just need a drupalorg_update_N() that enables and configures the module. If that's done and working, I can easily commit it and deploy the module on our D7 test sites. Then y'all would have D7 issue JSON to rewrite your stuff against.

I guess that means you'd want a patch on 7.x-3.x of http://drupal.org/project/drupalorg? I'm pretty taxed in the next couple of weeks, so I might miss launch; if no one else picks it up sooner, I can probably do as described in #101 later.

Let's continue on #1710850: Deploy RestWS for D7 project issue JSON. The whole project will take more than a couple weeks to launch, so we have some time.

Yeah, that's it (although we should really be discussing these details at #1710850: Deploy RestWS for D7 project issue JSON but it's cool). :) Don't worry if you can't personally get to it. Hopefully someone can.

Cheers,
-Derek