Problem/Motivation

As part of the D7 port of project_issue, we decided to remove the custom code that's providing #112805: JSON menu callback for project issues from project_issue and instead use a generic module, RESTful Web Services. This unfortunately did not make it in before the Drupal.org D7 launch.

Known consumers of JSON data:

Potential consumers of JSON data:

Proposed resolution

We need a drupalorg_update_N() to enable and configure this module to provide equivalent service as project_issue did in D6, although the JSON structure will be different.

Remaining tasks

  • Varnish caching HTML/JSON in the same place, see #76.

Deployment

  1. Update entity to 7.x-1.x-dev.
  2. Update drupalorg module
  3. Install & enable restws 7.x-2.2.
    • Access the resource comment : anonymous, authenticated user
    • Access the resource file : anonymous, authenticated user
    • Access the resource node : anonymous, authenticated user
    • Access the resource taxonomy_term : anonymous, authenticated user
    • Access the resource user : anonymous, authenticated user
CommentFileSizeAuthor
#101 drupalorg-restws-1710850-101.patch523 bytesmr.baileys
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Senpai’s picture

Status: Active » Postponed
Issue tags: +porting
dww’s picture

Status: Postponed » Active

restws 7.x-1.0 now exists. Any final concerns to going with that? If not, we should set #1697826: Duplication of RestWS / join efforts? active and move forward to make sure RestWS does everything we need, do a security audit (although the maintainer is a member of the security team, which is a good sign), and mark EntityJSON as abandoned/deprecated. If anyone on the infra team has objections to RestWS, please speak now.

Thanks,
-Derek

patrickd’s picture

RestWS on drupal.org would be a huge win,
IMHO we should deprecate entity_json and definitely use restws.

mitchell’s picture

I prefer Entity JSON for the reason I mentioned in #1697826-7: Duplication of RestWS / join efforts?.

OTOH, since Entity JSON is only for rendering and (intentionally) doesn't add support for the POST request method, non-browser clients would still need to use html forms instead of encoded payloads. That, or potentially another request decoder + processor could be used. As a side note, I'm currently researching the potential for Rules to do this.

I think the question is, are you looking to solve the "render X as JSON" issues as well as #1737076: Provide a web service to add/update issues in one fell swoop (go with RestWS), or do you want to approach these GET & POST/PUT issues independantly and compare apples to apples and oranges to oranges (go with Entity JSON and continue researching other solutions for POST/PUT)?

Senpai’s picture

@mitchell, We're looking to "solve the "render X as JSON" issues" first and foremost, but with RestWS, we get #1737076: Provide a web service to add/update issues also, and for free, so we're going with RestWS over Entity_JSON because we'll only get the chance to deploy one of them.

Senpai’s picture

The only thing we're now waiting on is an approval from the Infrastructure team to deploy RestWS-7.x-1.0 to drupal.org.

dww’s picture

Priority: Normal » Major
Status: Active » Needs review

killes or nnewton: can someone give us the "we won't veto that" amen to restws so we can proceed?

Thanks,
-Derek

webchick’s picture

Assigned: Unassigned » nnewton

Assigning.

killes@www.drop.org’s picture

Can we do a test install on one of the dev servers?

dww’s picture

Of course. We'll deploy on git7site and try to get it working long before production launch.

nnewton’s picture

re-assigning this to killes as I believe this is more code review?

killes@www.drop.org’s picture

Discussed this a bit in IRC:

There is no principal reason why we couldn't use this module.

We agreed that we don't need a security review as the main author is part of the sec team.

We aren't sure yet what performance implications this could have.

I am aware that RESTful stuff can be cached by varnish, but caching new stuff will for example require more varnish memory or will kick out other stuff earlier.

As long as it does exactly what the current code is doing and nothing else, there's no reason to not use it.

dww’s picture

Title: Deploy either RestWS or Entity_JSON for D7 project issue JSON » Deploy RestWS for D7 project issue JSON
Assigned: killes@www.drop.org » dww
Status: Needs review » Needs work

Great, thanks!

To clarify why we don't need another sec audit -- this code is basically the same as the REST support that went into D8 core, so it's already had a lot of scrutiny.

Yes, I believe we can restrict the functionality to specific node types, etc. We'll confirm before launch.

Assigning this to myself setting back to needs work to actually merge this into bzr, put something into drupalorg to enable/configure it properly on site rebuild, etc.

dww’s picture

Assigned: dww » Unassigned
Priority: Major » Normal

Upon closer consideration, while it'd be nice to have this working when we launch, it's not really a critical blocker. All the clients of this JSON (which I believe are a drush command very few people use and perhaps some part of dreditor) are going to be broken when we launch, since the JSON schema is going to radically change. So, we could always roll this out in the first few weeks after launch if it's not ready. Unassigning myself since I've got actually critical and major stuff to work on first. When everything else is done I'll return to this. If someone who feels passionately about issue JSON wants to run with this in the meanwhile and see what it would take to configure restws for our needs, please assign yourself and knock yourself out. ;) I'm happy to review and commit someone else's patch here, I just can't justify calling this "major" and working on it right now. But I'm glad we now have a clear path forward so this isn't actually blocked, just short on resources to implement it. ;)

Cheers,
-Derek

kostajh’s picture

Thanks for the update. In addition to the Drush IQ project, just noting that I plan to use this functionality to implement a Drupal.org service for Bugwarrior when the JSON feed of project tasks is available. So that makes at least two of us who will use the feature :)

drumm’s picture

I updated the issue summary here. If anyone wants to take this on, we have dev sites with real data and configuration to work with, http://drupal.org/node/1018084.

drumm’s picture

Project: Drupal.org infrastructure » Drupal.org customizations
Version: » 7.x-3.x-dev
Component: Drupal.org module » Drupal 7 upgrade
Damien Tournoud’s picture

Status: Needs work » Postponed

So, given that RestWS interaction with the Drupal page cache is currently full of pain, let's postpone this on #1969466: Fix GET /node/1 with page caching.

Damien Tournoud’s picture

Issue summary: View changes

Focus summary.

dww’s picture

Sounds good. Thanks for the link.

I just re-updated the summary to be more accurate and to mention killes's request that we restrict this to issue nodes, at least for the initial deployment.

drumm’s picture

Priority: Normal » Minor

Is there any other path forward since #1969466: Fix GET /node/1 with page caching is blocked on #1303010: Page cache only uses URL as cache ID, not HTTP Accept headers or language? As dww said in #14, this is something that we can launch without.

greg.1.anderson’s picture

i would be fine with node/1.json as an interim implementation, if someone wanted to pick up #1969466: Fix GET /node/1 with page caching with that. I'd be happy to adjust the drush iq commands to use whatever json is produced, and adjust them again when the "final solution" is available. I could do that part really quickly.

Unfortunately, though, I just don't have time to fix the d.o-side json code right now, and probably won't until sometime after DrupalCon Prague. I would be sad to not have drush iq working on d.o for a while, but agree that this should not block the launch.

drumm’s picture

Issue tags: -drupal.org D7 +Drupal.org 7.1

Officially moving this to post-launch. If there is volunteer progress on RestWS, it can move back in-scope.

klonos’s picture

markhalliwell’s picture

x-ref #2123629: Issue JSON which was closed as a dup of this issue:

At least two utilities (that I know of and use regularly), Dreditor and Drush IQ, utilize "/project-issue/json" at the end of an issue URL to return JSON data for that particular issue.

markhalliwell’s picture

Issue summary: View changes

more accurate summary, and added details about restricting to issue nodes

clemens.tolboom’s picture

As far as I understand our tools: drush_iq and dreditor can be configured to request node/1.json and then we don't have to wait for #1969466: Fix GET /node/1 with page caching

We need to configure varnish to respond to accept headers and rewrite node/1 requests with 'Accept: application/json' to node/1.json right?

RestWS has no bundle filter afaik. Do we need custom code for that or did I miss something?

I'm here because of drush_iq: #2125121: Allow user to specify protocol (http instead of https) with --issue-site

helmo’s picture

Issue summary: View changes
Priority: Minor » Normal
Status: Postponed » Active

I did some testing on http://project-drupal.redesign.devdrupal.org together with @clemens.tolboom
Steps to install:

$ drush dl restws
$ drush en -y restws

$ drush uli
Add permission 'Access the resource node' to anonymous

curl -H 'Accept: application/json' http://username:password@project-drupal.redesign.devdrupal.org/node/1

We get JSON

curl http://username:password@project-drupal.redesign.devdrupal.org/node/1
We get HTML

The username and password are both "drupal" (I didn't fill it in because I didn't want to make a valid URL for crawlers to follow...)

So I can't reproduce the caching issue #1969466: Fix GET /node/1 with page caching ... probably because "Cache pages for anonymous users" is not enabled on Drupal.org (http://project-drupal.redesign.devdrupal.org/admin/config/development/pe...)

Now to an issue node... error.

EntityMetadataWrapperException</em>: This node is no book page. in <em class="placeholder">entity_metadata_book_get_properties()</em> (line <em class="placeholder">24</em> of <em class="placeholder">/var/www/dev/project-drupal.redesign.devdrupal.org/htdocs/sites/all/modules/entity/modules/callbacks.inc

I applied a patch for the entity module to get results for a project_issue node: #1330086-15: wrapper throws exceptions for properties that are declared in the info

clemens.tolboom’s picture

From a dreditor point of view we grab the json for a logged-in user. For our test environment we had to set permission for both anonymous and registered user for dreditor to work.

Do we need this? Or should be only allow anonymous the request json?

Another issue lies in RestWS serves only node/1 or comment/2 .

drush_iq want to generate commit messages with attribution for which it needs the comments afaik. Should we allow that?

afaik it look ok to deploy RestWS apart form what to allow (node/comment/node-type)

(we really should update the summary now)

clemens.tolboom’s picture

Some rephrasing before updating the summary:

Regarding d.o making #1969466: Fix GET /node/1 with page caching seems not to be a blocker for implementing Rest WS on d.o

Accept Header

- dreditor uses jQuery.getJSON on node/1.
- drush_iq does a _drush_download_file which has no 'Accept: application/json' in it's header. drush_iq must fall back on node/1.json to make it work with the current Rest WS implementation

Caching

- on mentioned test environment we have no page cache by drupal. It that valid for d.o too?
- d.o uses varnish which can handle any header to decide how to cache so dreditor jQuery.getJSON will work.

Fetching Data

- dreditor fetches several aspect but I only checked for commentCount (needs more info)
- drush_iq wants the whole issue (node + comments + users) to ie create a commit message. This creates to much request IMHO

greg.1.anderson’s picture

Is the new json structure documented anywhere? I was looking at this vis-a-vis drush_iq (#2127965: Fix for the new json structure on d.o D7), and it is not clear to me how to determine which comment number an attachment is attached to. This is something that drush_iq needs to know.

greg.1.anderson’s picture

I didn't say quite the right thing in #29. What I'm looking for is the module and issue # where the structure of the json is being defined. In the D6 version, project_issue held all of the logic, but here it appears to be entity based. Where is the code that composes the json?

alexpott’s picture

Priority: Normal » Critical

Affects drush_iq, dreditor, custom tools I've built that help manage the rtbc queue, and probably most importantly completely breaks http://drupalmentoring.org

patrickd’s picture

Would also be a huge win for simplytest.me and gabors rocketship, which currently both depend on scrapping the html response for needed information.

helmo’s picture

Issue summary: View changes
helmo’s picture

Priority: Normal » Critical
Issue summary: View changes

Re-added summary... somehow got lost in an edit

[edit] reported in #2130363: Old summary/body version in edit form

helmo’s picture

Issue summary: View changes
klonos’s picture

clemens.tolboom’s picture

I'd love to see some discussion about performance impact.

I've tested a issue download with drush_iq test from #2127965: Fix for the new json structure on d.o D7 which took the issue and it's related entities (comment, user, file) which took ~8 seconds.

When clients doing similar things (getting the 'whole' issue) without proper cache on client side we definitive need to solve this server side.

Maybe create a special json server node?

markhalliwell’s picture

@clemens.tolboom, @helmo, any progress on this? What do we need to do? Who needs to sign off? This is pretty important to implement, even if it relatively works. We can deal with performance down the road, yes?

helmo’s picture

I don't think the infra team would like more performance surprises/regressions... #2136119: Text search on issue queues is slow and sometimes WSODs is currently annoying enough.

We need to figure out if this format is right... https://drupal.org/comment/8150487#comment-8150487 has some thoughts on which requests would be needed.

There is also a potential security concern being investigated (97963).

greg.1.anderson’s picture

I haven't looked at this since #29, and don't quite remember what I was seeing back there, but I got stuck because I didn't know how to figure out which comment each attachment was associate with. Perhaps there is a simple field I just didn't see; I'll have to look at it again later when I have more time.

dww’s picture

Yeah, I'm a bit concerned about deploying restws with all the other issues d.o is facing these days. :/

Re #29: The JSON itself is just the default coming from restws. I still haven't actually looked at the module at all, so I have no idea how it decides exactly what to do.

Re #40: The mapping of files to comments (technically, files -> the node revision where they were attached -> the auto-generated comment from that node revision) is all being handled via Nodechanges which is implementing various hooks from Extended File Field to make that info visible when viewing/editing issue nodes. If there's a hook in restws that would allow nodechanges to add this info to the resulting JSON for each issue node, please open an issue in the nodechanges queue with the details (and/or) links to the docs.

Thanks!
-Derek

xjm’s picture

The fact that this is outstanding has pretty much broken the entire process for the core mentoring program, at a time when we need a path to contribution very badly for Drupal 8. Are there any chances of this being resolved within the next few weeks? Months? What can core contributors do to help here?

Cross-tagging, also.

drumm’s picture

Yes, in the next few weeks. Bugs are generally being fixed before features.

xjm’s picture

Thanks @drumm! I'd suggest that this is a bug and not a feature -- it is a regression from the Drupal 6 version of d.o, which had JSON data available for issues (functionality that core mentoring has relied on since the spring of 2012). Would anyone disagree with recategorizing this as a bug for that reason? (I understand of course that there are other, more-critical critical bugs, but it's still a regression with a significant impact.)

drumm’s picture

The critical issues are dwindling, I don't think quibbling about the status is needed.

It does look like the issue summary could use an update.

Damien Tournoud’s picture

I would like a complete assessment of the impact of this on the page caching. Drupal.org does use the standard page caching even if it does so in front of Varnish, so I fear that we can still end up in the wrong version on the cache for node/1 if any client sends an accept header.

klausi’s picture

As you can see in restws_page_callback() a 301 redirect is returned if you access /node/1 with Accept: application/json and page caching is enabled. Drupal will not set an Expires header on the redirect response, so Varnish will not cache that and the response will not end up in Drupal's page cache either.

I think we did a fair bit of assessment already when we resolved the cache poisoning security issue in RESTWS, right?

drumm’s picture

https://drupal.org/admin/config/development/performance says our Drupal page cache is off. We do use Varnish.

clemens.tolboom’s picture

In #2127965: Fix for the new json structure on d.o D7 the test does ~418 request to get

- 25 nodes
- 229 comments
- 50 users
- 94 files
- 19 taxonomy terms

It takes around 6 minutes to fetch all these data from the test server.

Taking average per node is ~14 seconds.

Having this locally cached by drush_iq this is no big deal for a drush_iq developer I expect.

But having dreditor active on all issues fetching meta data for ie commit message is another story.

Can we do better?

What about delivering node/1.json including all related data (same as on node/1) ?

klausi’s picture

@drumm: interesting, so how does Varnish know which page can be cached? And how does Drupal prevent Varnish from caching certain pages? As a developer you usually use drupal_page_is_cacheable(FALSE) to indicate that a page should not be cached ... so that does not work on drupal.org?

So you would have to tell Varnish to vary around the Accept header to prevent that it caches a 301 redirect response on /node/1.

@clemens.tolboom: that sounds like a custom solution for issue queue nodes would be a better fit for drupal.org, which renders all comments and possibly other entity data inline. Basically you just want the HTML page as JSON, so that sounds to me like you would use node_view_multiple() as it is used by node_show() and pass the render array to a JSON theming function.

clemens.tolboom’s picture

@klausi I'm not sure about a themed node.

Let's see what others think.

Regarding varnish see #12 #25 and #28

We should really update the summary :/

drumm’s picture

The Drupal page cache for Drupal.org is off in production. We do use Varnish, and dev/staging sites do have the same Varnish config.

Have people been testing restws 7.x-2.1 or 7.x-1.4. (We should not deploy a dev version unless we have a good reason to.)

From #26, looks like #1330086: wrapper throws exceptions for properties that are declared in the info still needs to be solved?

helmo’s picture

We were testing with 7.x-2.1 on project-drupal.redesign.devdrupal.org

I've commented on #1330086: wrapper throws exceptions for properties that are declared in the info

drumm’s picture

Assigned: Unassigned » drumm
drumm’s picture

Enabling restws causes a PHP notice in drupalorg, since the menu item's page arguments was being used to get the current node. menu_get_object() is a better way to do this. Fixed with http://drupalcode.org/project/drupalorg.git/commit/d0f20c3.

We do aim for Drupal.org to be notice-free. I'll see if I can find any others before deployment.

hass’s picture

There is a string in this commit that should better use t() and a secure % placeholder for title. I guess this may otherwise be used for code injection...

tim.plunkett’s picture

@hass, which string? You mean the one that is run through check_plain() before being printed?

hass’s picture

"Issues for " is a translatable string, but does not use t().

tim.plunkett’s picture

And what site is running the drupalorg.module that is translating strings? There are plenty in there that don't use t(), feel free to open another issue.

hass’s picture

Are you a supporter of crappy code and recommend not using our existing APIs? I'm sorry, but this is very bad code style and we also have these placeholders to prevent xss and other issues. It makes reading code really difficult to make a check_plain and concatenate strings later.

For sure I can open a new case, but I think sich code quality should not get committed. It doesn't matter at all if this code is translated or not.

tim.plunkett’s picture

#60, then open an issue against homebox.module, that's where the check_plain is. If one was included here, it would be double escaped.

drumm’s picture

homebox isn't involved, the string is used further down in

            $content .= homebox_add_link(t('Add @name to dashboard', array('@name' => $block[0])), $page, $block[1], $block[2], $options);

It should be improved, in a separate issue, #2185785: Improve t() usage in drupalorg_block_view('add_to_dashboard').

My main concern for this issue is if there was this notice, which was a symptom of that link on project pages breaking, what other side effects might there be?

tim.plunkett’s picture

homebox isn't involved,

Then

$content .= homebox_add_link(
...

clemens.tolboom’s picture

Issue summary: View changes
drumm’s picture

            $content .= homebox_add_link(t('Add @name to dashboard', array('@name' => $block[0])), $page, $block[1], $block[2], $options);

This call to t() is where $block[0] is sanitized, so we should allow variable placeholders there. This all happens before the outer call to homebox_add_link()

Please post additional followups related to that to #2185785: Improve t() usage in drupalorg_block_view('add_to_dashboard').

drumm’s picture

Status: Active » Postponed

This is now running on http://restws-drupal.redesign.devdrupal.org/node/2177923.json.

I had to re-roll #1330086: wrapper throws exceptions for properties that are declared in the info and patch #2186603: entityreference to anonymous user causes notice with access(). I'd like to see some feedback on those issues before moving forward with this. We can patch Drupal.org's codebase, but I want to know that I'm on the right track.

alexpott’s picture

clemens.tolboom’s picture

This is now running on http://restws-drupal.redesign.devdrupal.org/node/2177923.json.

@drumm what is 'this'? Please update the issue summary with a battle plan reflecting the latest knowledge.

@alexpott not all issues are fully available on the test environments. Are you ran into such an issue?

RestWS only serves entities thus entity references are 'by ref'. See #42 and drush_iq #2127965-11: Fix for the new json structure on d.o D7. That issue has a zip file with some more data.

The link provided by @alexpott http://restws-drupal.redesign.devdrupal.org/node/2176105 is broken.

Fatal error: Unsupported operand types in /home/drumm/project_issue/project_issue.module on line 2387

Call Stack:
    0.0000     629800   1. {main}() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/index.php:0
    0.1496    7241480   2. menu_execute_active_handler() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/index.php:21
    0.1497    7242256   3. call_user_func_array() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/includes/menu.inc:517
    0.1497    7242776   4. restws_page_callback() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/includes/menu.inc:517
    0.1498    7243912   5. call_user_func_array() /home/drumm/restws/restws.module:373
    0.1498    7243944   6. node_page_view() /home/drumm/restws/restws.module:373
    0.1503    7256104   7. node_show() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/modules/node/node.module:2755
    0.1503    7256536   8. node_view_multiple() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/modules/node/node.module:1470
    0.2818    9443712   9. node_view() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/modules/node/node.module:2671
    0.2818    9443712  10. node_build_content() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/modules/node/node.module:1335
    1.0428   18990152  11. module_invoke_all() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/modules/node/node.module:1445
    1.2746   24353480  12. call_user_func_array() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/includes/module.inc:895
    1.2746   24354000  13. drupalorg_project_node_view() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/includes/module.inc:895
    1.3158   24413184  14. drupal_get_form() /home/drumm/drupalorg/drupalorg_project/drupalorg_project.module:640
    1.3158   24414576  15. drupal_build_form() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/includes/form.inc:131
    1.3158   24417488  16. drupal_retrieve_form() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/includes/form.inc:345
    1.3163   24439880  17. call_user_func_array() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/includes/form.inc:806
    1.3163   24440496  18. node_form() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/includes/form.inc:806
    1.3211   24474480  19. field_attach_form() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/modules/node/node.pages.inc:326
    1.3212   24475184  20. _field_invoke_default() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/modules/field/field.attach.inc:574
    1.3212   24475640  21. _field_invoke() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/modules/field/field.attach.inc:385
    1.3355   24827696  22. field_default_form() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/modules/field/field.attach.inc:209
    1.3359   24836008  23. options_field_widget_form() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/modules/field/field.form.inc:112
    1.3359   24839264  24. _options_get_options() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/modules/field/modules/options/options.module:86
    1.3359   24839344  25. module_invoke() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/modules/field/modules/options/options.module:245
    1.3360   24843600  26. call_user_func_array() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/includes/module.inc:866
    1.3360   24844216  27. entityreference_options_list() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/includes/module.inc:866
    1.3368   24854496  28. EntityReference_SelectionHandler_Generic->getReferencableEntities() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/sites/all/modules/entityreference/entityreference.module:765
    1.3368   24854888  29. ProjectIssue_SelectionHandler_Assigned->buildEntityFieldQuery() /var/www/dev/restws-drupal.redesign.devdrupal.org/htdocs/sites/all/modules/entityreference/plugins/selection/EntityReference_SelectionHandler_Generic.class.php:162
    1.3369   24859512  30. ProjectIssue_SelectionHandler_Assigned->getAllowedAssignees() /home/drumm/project_issue/plugins/entityreference_selection/ProjectIssue_SelectionHandler_Assigned.class.php:83
    1.3372   24860416  31. project_issue_assigned_choices() /home/drumm/project_issue/plugins/entityreference_selection/ProjectIssue_SelectionHandler_Assigned.class.php:71
markhalliwell’s picture

I thought about this last night....

Is there anyway we can include the filesize() of the attached files in a node?? This would seriously make some things in Dreditor REALLY easy and faster if we did (ie: reduce HTTP requests).

markhalliwell’s picture

Also, @sun and I were just discussing that for Dreditor it would make sense to also include the JSON data directly on the page through JS settings (so we don't have to do an extra AJAX request). Maybe something like Drupal.current_node?

xjm’s picture

Re: #69, I'm sure there's a ton of improvement we can make once this is deployed, which we can file a followup for, but for now I'ld be really happy to see the pre-upgrade functionality restored. This regression has been outstanding for over three months now and it resulted in drupalmentoring.org not being nearly as useful for the Global Sprint Weekend. =/

markhalliwell’s picture

Agreed :) We can fix this upstream easily enough.

drumm’s picture

Issue summary: View changes

Slimming down issue summary to make it more readable.

drumm’s picture

Issue summary: View changes

Adding deployment instructions.

drumm’s picture

#68 - I used the restws dev site for some other tasks unrelated to restws, so you probably caught the site when I was debugging something.

drumm’s picture

I confirmed Varnish may need configuration changes. devwww's default.vcl is the same as production.

Because of the drupal/drupal http auth, all requests from outside are not cached. Requests from devwww itself, do not need http auth, so Varnish will get hits. I filed #2198927: Configure Varnish to cache requests with drupal/drupal http auth to make this remotely testable in the future.

We need to either:

  • Have a way to to turn off restws's alteration of exisitng menu callbacks, only serving json when .json is appended to the URL.
  • Configure Varnish to consider the Accept header.
  • Something in Drupal may want to return a different Vary header.
drumm’s picture

#67 - I think restws needs to know it it okay to serve JSON for the file URLs. Either append .json to the URL or add an Accept: application/json header.

drumm’s picture

Issue summary: View changes

I rebuilt the dev site with the current patches earlier today. It looks like we are blocked on:

YesCT’s picture

Issue summary: View changes
Issue tags: +Needs issue summary update

added those blocking issues to the issue summary, but I think the other remaining tasks in the issue summary still need updated.

drumm’s picture

Marking as a DSWG Dev Tools Team priority too.

drumm’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Removing the "Also remaining" section of the issue summary. It had:

Varnish config? remove cookies to only deliver anonymous json data.

Redundant with the blocker mentioning #76.

Decide for which entity types we want to allow this

Covered/decided by the permissions in the deployment section.

Is this the right format? Json of an issue node has limited data about comments, #28 )

At this point, we are pretty well committed to the format. We can add more data as followup issues. RestWS gets data straight out of Entity API; things can be added to drupalorg_entity_property_info_alter() or, ideally, via less-custom modules.

Review & move toward commit: []

Not useful.

drumm’s picture

Issue summary: View changes
Status: Postponed » Active

The two issues were resolved. We can now (carefully) upgrade Entity API on production and proceed with this issue.

drumm’s picture

Since there have been changes in the meantime, I'm rebuilding the restws dev site. It will be done in ~2hr. This also is a chance to try out the deployment instructions and add any missing details.

Entity API is most visible in the issue queue listings. Search API relies heavily on it for getting data to index, and rendering it in Views. That will need the most testing when upgrading. We will want to watch out for anything needing downtime, or affecting performance.

betz’s picture

Issue summary: View changes

I updated entity API to DEV on the restws dev site.
As patches are in entity api now, i removed them from the deployment instructions.
Gave anonymous and authenticated user access to "Access the resource node", "Access the resource file", updated this also in the deployment instructions.

betz’s picture

Issue summary: View changes

I have no idea what to do for Varnish to be honest.

Also, I added 'Access the resource comment' permission to the deployment instructions.

drumm’s picture

For Varnish, I'd actually like to see an option in RestWS to not alter the default menu paths, the first loop in restws_menu_alter(). Then JSON is only served when .json is appended to the URL. This may not be "pure REST," but it does keep Varnish working as-is, HTML always comes from the usual paths. The menu altering also makes me nervous, since I could imagine it conflicting with other hook_menu_alter() implementations, in hard-to-troubleshoot ways.

If that is not acceptable for some reason, we can change the Varnish configuration.

betz’s picture

Do we actually still want to use RestWS?
It seems to me some custom code would help us much more here.

Some items I read in this issue:

  • include all comments in json
  • include files with comments in json
  • include file meta data in json
  • dont alter existing menus
  • no way of limiting bundles

What is everyone's thought on this?

sun’s picture

I'm not a domain expert, but I'm equally concerned about the generic implementation vs. very concrete use-case mismatch.

RestWS looks like a very large dependency for generating a JSON response of an issue.

To my knowledge, not even the REST module in D8 supports the data + requirements we're looking for here. (And, since I can't stop to repeat myself, >95% of drupal.org should be powered by Drupal core only, because whatever requirement we have for d.o is almost guaranteed to be a requirement elsewhere, too, so almost all of drupal.org's requirements should be delivered natively by Drupal core. I will +1 and RTBC any kind of D8 core patch that reduces custom code on d.o.)

Either way, regarding D7, like @betz, I'm skeptical whether the abstraction of RestWS is actually worth the effort?

drumm’s picture

I believe we can add more data via hook_entity_property_info_alter(), and I've been assuming there are D8 equivalents to get into the entity system.

If this isn't actually getting us on a smooth migration path toward REST in D8 core, then we should seriously consider mimicking D8's format with custom code. We can assume the issue content type and its fields will have a relatively straightforward migration.

I should say that RestWS's support for POST/etc to update entities via REST is somewhat exciting, but totally lacking QA for Drupal.org. I imagine drush and other tools will do interesting things to post, but we have little to no policies in place for what's appropriate for robots.

rcross’s picture

release & refactor...

considering that many tools are dependant on using this (which is why this issue is marked critical), can I suggest that we move toward launching something first and then refactoring it to meet whatever standard of implementation is finally agreed on?

While sun's comments might be something to aspire to, in practice it is completely unrealistic as project* will never be part of core. Unless we were to discuss pushing the entire project hosting, issue queue, etc into a sub-domain (i.e. projects.drupal.org) which definitely has merits but would take away focus from something that people need today, not some distant future.

I also think there is some merit to implementing a generic solution where feasible, since it provides support/clout/maturity/etc to a contrib project rather than creating additional custom code that our community is then paying DA staff to maintain.

Please don't think I am ambivalent about the implementation details, but a timely functional outcome is stronger priority than an ideal implementation. I may be wrong, but watching this ticket, it feels like the current approach has traction, has gone through testing and is close to a launch. I'd rather not give that up.

betz’s picture

OK. Then I would personally update our varnish instead of patching RestWS.
I don't have experience with this, found /etc/varnish/default.vcl on the server, but not sure what to do here.
Leaving this to someone with experience on configuring varnish here.

Let me know if I can do something else.

YesCT’s picture

markhalliwell’s picture

I'm not entirely sure how to help here, but getting something up and going would be preferable than sitting on this.

  • Commit d0f20c3 on 7.x-3.x, 7.x-3.x-dev by drumm:
    [#1710850] Do not assume page_arguments is reliable for node, restws...
tvn’s picture

drumm’s picture

I talked to fago and a decent path forward looks like:

Implement drupalorg_alter_restws_resource_info() to prefix the REST path with something like api-d7/. This will make the restws endpoints available at paths like drupal.org/api-d7/node/123[.json]. While not "pure REST", this sidesteps concerns about caching and potential conflicts in menu altering. (When we upgrade to D8, we can redirect these paths, and get as much compatibility as RESTWS -> D8 gives us for free.)

RESTWS supports POST/PUT/DELETE/etc requests without a convenient way to control them. We can cut them off at Varnish, if needed. Before making those available, they need a good amount of QA and security review. And maybe a community discussion on what we want and don't want from automated tools & bots being able to post to Drupal.org, not just issues, more easily.

mr.baileys’s picture

RESTWS supports POST/PUT/DELETE/etc requests without a convenient way to control them. We can cut them off at Varnish, if needed.

I had a "Read-only REST server" requirement for a project I'm working on, and started a sandbox project to switch RESTws to read-only mode: https://drupal.org/node/2282849 . This module could serve as an alternative to blocking the requests in Varnish.

I worked on this issue during the Contribution sprint in Austin, and am happy to write the necessary drupalorg_* code to help advance this issue, if somebody can help me get set up for drupalorg development.

drumm’s picture

@mr.baileys - I want ahead and gave you access to devwww, see http://drupal.org/node/1018084 for details on connecting. If https://drupal.org/user/383424/ssh-keys wasn't current, let me know.

I went ahead and started a rebuild of the restws dev site, since I'm sure it is decently outdated. It will be ready in in 2 hours.

mr.baileys’s picture

Assigned: drumm » mr.baileys

Thanks @drumm! My keys were not current, I went ahead and updated them, could you provide access for the new key?

drumm’s picture

@mr.baileys - your key is now updated on devwww.

mr.baileys’s picture

Status: Active » Postponed
FileSize
523 bytes

I maanged to get this working on the dev server under /api-d7/node/12345.json (example: https://restws-drupal.redesign.devdrupal.org/api-d7/node/2278541.json). However, I had to patch RestWS to get this working, since it currently always takes arg(0) as resource name, arg(1) as resource ID.

Postponing on #2286515: Multipart menu_path's are not working.

Patch attached contains the hook_restws_resource_info_alter() implementation used to move the resource endpoints and have them live under api-d7.

clemens.tolboom’s picture

@mr.baileys nice work :)

Testing for #2127965: Fix for the new json structure on d.o D7 I ran into:

Fetching https://restws-drupal.redesign.devdrupal.org/api-d7/node/2125253.json gives

{"book_ancestors":[],"comments":[],"body":[]}

while https://restws-drupal.redesign.devdrupal.org/node/2125253 gives

Page not found

.

Guess that's a RestWS quirk. Reported #2286933: Getting a non empty JSON on 'Not found'

clemens.tolboom’s picture

[edit]
Make sure to use wget --no-check-certificate thanks to @helmo as we have a self-signed certificate

Sorry for the noise.
[/edit]

drumm’s picture

We could work around #2286515: Multipart menu_path's are not working by making the path something like /node-api-d7/2278541.json.

clemens.tolboom’s picture

I get a lot of 403 Forbidden while running my test.

Please enable all related REST calls like api-d7/user, api-d7/taxonomy_term, api-d7/comment.

05:05:07 {feature/2127965-json-on-d.o-D7 *%} ~/.drush/drush_iq$ ./runtests.sh iqInternalTest.php
Using phpunit at /Users/clemens/Sites/drupal-devs/drush/vendor/phpunit/phpunit/phpunit.php
PHPUnit 3.7.31 by Sebastian Bergmann.

EFetching (1): node/2278541

Fetching (2): user/370574

string(267) "{"type":2,"message":"file_get_contents(https:\/\/...@restws-drupal.redesign.devdrupal.org\/api-d7\/user\/370574.json): failed to open stream: HTTP request failed! HTTP\/1.1 403 Forbidden\r\n","file":"\/Users\/clemens\/.drush\/drush_iq\/iqInternalTest.php","line":161}"
Fetching (3): taxonomy_term/49969

string(275) "{"type":2,"message":"file_get_contents(https:\/\/...@restws-drupal.redesign.devdrupal.org\/api-d7\/taxonomy_term\/49969.json): failed to open stream: HTTP request failed! HTTP\/1.1 403 Forbidden\r\n","file":"\/Users\/clemens\/.drush\/drush_iq\/iqInternalTest.php","line":161}"
Fetching (4): taxonomy_term/10002

string(275) "{"type":2,"message":"file_get_contents(https:\/\/...@restws-drupal.redesign.devdrupal.org\/api-d7\/taxonomy_term\/10002.json): failed to open stream: HTTP request failed! HTTP\/1.1 403 Forbidden\r\n","file":"\/Users\/clemens\/.drush\/drush_iq\/iqInternalTest.php","line":161}"
Fetching (5): comment/8837189

string(271) "{"type":2,"message":"file_get_contents(https:\/\/...@restws-drupal.redesign.devdrupal.org\/api-d7\/comment\/8837189.json): failed to open stream: HTTP request failed! HTTP\/1.1 403 Forbidden\r\n","file":"\/Users\/clemens\/.drush\/drush_iq\/iqInternalTest.php","line":161}"
clemens.tolboom’s picture

Thanks to @helmo reminding me I can login as user-1 myself I checked permissions.

I now have added / checked

  • Access the resource comment : anonymous, authenticated user
  • Access the resource file : anonymous, authenticated user
  • Access the resource taxonomy_term : anonymous, authenticated user
  • Access the resource user : anonymous, authenticated user
clemens.tolboom’s picture

Issue summary: View changes

Added all "Access the resource *" to deployment section.

clemens.tolboom’s picture

With the new config I get 1337 files for my 28 nodes from the testset in drush_iq #2127965: Fix for the new json structure on d.o D7.

Gábor Hojtsy’s picture

One interesting option that was not covered here is using the JSON to manage local priorities applicable to teams within organizations combined with eg. Google Docs. Eg. at Acquia we used to use the JSON data to augment issue information with internal team priorities and notes. This is one of the options I explained in http://hojtsy.hu/blog/2014-jun-24/how-manage-drupalorg-issues-according-... with JS code readily useful with the old JSON structure at least. This will be amazing to revive that technique :)

clemens.tolboom’s picture

I reported a D8 core issue #2291811: Support for book structure related to our stalled discussion about the full issue containing all users and comments.

My request in #108 took 20 mins to load which is unusable.

Any ideas on how we should tackle this?

markhalliwell’s picture

@clemens.tolboom, not sure how to help ATM.

Follow-up from #2281763-57: Make Drupal.org user profiles more robust though:
We need to make sure that user objects includes the git attribution to use.

tvn’s picture

Status: Postponed » Active

#1697558: Properly document and implement "menu_path" is fixed so un-postponing. Thanks for all the progress mr.baileys and others!

  • drumm committed d0f20c3 on 2299191-beta_project_issue_project_searchapi
    [#1710850] Do not assume page_arguments is reliable for node, restws...
pwolanin’s picture

Do we actually need Rest or could we dump a JSON text file at a known location for each node on cron?

pwolanin’s picture

@drumm, can you explain what's being released next week?

Change affects: Drupal.org
A JSON API for Drupal.org, built with the RESTful Web Services module, will be made available.

joelpittet’s picture

So excited, dev toys!!! woot \o/

drumm’s picture

Assigned: mr.baileys » drumm

Taking this for the deployment goal this week. I'm not ready to narrow down the deployment window without a round of testing first.

The plan is to resolve this issue.

drumm’s picture

Issue summary: View changes

Crossing out the first deployment step since Drupal.org is already on Entity API 7.x-1.5, which was released relatively recently. This includes the fix for #2186603: entityreference to anonymous user causes notice with access(). It looks like that was the only reason for using Entity API -dev.

drumm’s picture

Issue summary: View changes

Updating the version number we want for RestWS, since we want #1697558: Properly document and implement "menu_path".

davidhernandez’s picture

This will be the final path, '/api-d7'?

pflame’s picture

@drumm What will be the final path to get node and comments information? Last week when I try the path https://restws-drupal.redesign.devdrupal.org/api-d7/node/2278541.json , it returned output. Now the same path is returning Page Not Found error.

drumm’s picture

I'm in the process of rebuilding the RestWS dev site to test out deployment. Those paths are correct.

It it looking likely that this deployment will not happen this week. A deployment yesterday caused downtime when cc all ran, the root cause is #1679344: Race condition in node_save() when not using DB for cache_field again. Enabling a new module will need cc all, and we can't do that until cache_field is back in memcache.

drumm’s picture

Issue summary: View changes
Status: Active » Needs review

Ok, this is getting back on track, and possible to deploy tomorrow.

The drupalorg patch does what it needs to do, and doesn't affect anything currently in production, so committed.

drumm’s picture

Since no one has tested POST requests as far as I know, I think we should 403 any requests to ^/api-d7/* that are not GET or HEAD.

We can whitelist the drupalci infrastructure, and anything else from Drupal.org servers, when it wants to talk to Drupal.org. Anything 3rd-party will need both a bunch of testing and a policy to handle situations like #2172149: Disable the Github Sync user account: it's confusing.

davidhernandez’s picture

Will there be documentation, data structure information, etc, to go along with this? This is something we want people to use, so I want to make sure people will now about it and how to use it. Also, will hosts have to be white listed? If so, what is the process for that?

drumm’s picture

Whitelisting is only for POST requests. Everything can GET. It isn't too much of scalability risk, I'm sure REST requests are lighter than HTML, and behind the same CDN, Varnish, etc.

Documenting should be a followup issue. As far as I know, the APIs available in D6 weren't documented; not having new documentation isn't good, but isn't a regression.

markhalliwell’s picture

Re: the /api-d7 prefix.

I know this is just to help sidestep caching and menu altering (#96), however I don't think we should really be using the current Drupal version as part of this path prefix. We could still, in theory, support this "version" of the API when we upgrade to 8.x... highly doubtful, but possible.

Since we don't have a separate API domain/server (that I'm aware of), we could at least follow the "industry standard" (and I'm using that very lightly btw) of prefixing with the actually API version. This is necessary for applications so when a new API is published that intentionally breaks BC, we don't all spend hours upon hours of hotfixing stuff.

I know some may see this as very trivial, but I believe if we don't do this, we're just setting ourselves up for disaster down the road. At the very least, it should reflect that truth of the matter: that this is really the first "official" API that we're implementing (regardless if it's "pure REST" or not).

That being said, I propose that it be changed to/api/v1 or /api/1.

edit (for ref): https://developer.github.com/v3/#schema

joelpittet’s picture

re #128 I was thinking the exact same thing, didn't want to say it but since you did, +1 to that idea! :)

clemens.tolboom’s picture

+1 for a /api/x

But why /api and not /rest ?

markhalliwell’s picture

APIs can evolve. REST is simply the type of API architecture we're going to be using (but not fully, ie: no PUTS, DELETES, etc... just GET).
APIs can also serve multiple formats from a single endpoint, this is common (but will not be implemented here) by sending different headers.

So this, in theory, could change in the future. Take for example:

/api/v1 -> REST (json)
/api/v2 -> REST (json, html)
/api/v3 -> REST (json, html, xml)
/api/v4 -> SOAP (xml, maybe html, because someone decided this was a good idea and tried to bring it back :-/)

Granted, this is highly unlikely, but it just illustrates that using the decided architecture type is pointless as it can simply change from version to version. It's honestly 1000x better to just give the version of the API in the path because then people know what endpoints are supported and what their responses should be.

drumm’s picture

The URL prefix, whatever it is, is temporary in the long run. The commits in #123 are a workaround for a problem; I'd be happy to remove any custom code which becomes unnecessary. When/if #1969466: Fix GET /node/1 with page caching is fixed and deployed on Drupal.org, we have the option of dropping the URL prefix. In that case, we would definitely redirect the old URLs. When Drupal.org upgrades to Drupal 8, we will want to use what core bakes in for REST, which won't have prefixes either. If the format from D8 is compatible, we will redirect to those URLs too.

drumm’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +needs drupal.org deployment

Confirmed the Varnish config

    # Block writes to RestWS, see https://www.drupal.org/node/1710850
    if (req.url ~ "^/api-d7" && req.request != "GET" && req.request != "HEAD") {
      error 403 "Forbidden. This API is read-only.";
    }

works for what we need. I believe this is ready to deploy.

drumm’s picture

I started a stub page for documentation at https://www.drupal.org/about-drupalorg/api. As always, everyone is welcome to edit book pages on Drupal.org.

drumm’s picture

https://www.drupal.org/api-d7/node/1710850.json now has JSON for this issue. I'll keep this issue open until the basic documentation is written.

davidhernandez’s picture

Even that stub is pretty useful. Thanks, Neil.

drumm’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -needs drupal.org deployment

https://www.drupal.org/about-drupalorg/api is now filled out enough to be a bit useful.

joelpittet’s picture

Just noticed that in the stub above there are URI's like https://www.drupal.org/api-d7/taxonomy_term/5328
but it seems there is no default so you'd have to suffix each with .json to get results. Maybe missed something but is that the intention?
https://www.drupal.org/api-d7/taxonomy_term/5328.json

davidhernandez’s picture

@joelpittet, yes, the instructions say, "All requests must either have an .[json|xml] extension". So https://www.drupal.org/api-d7/taxonomy_term/5328.xml also works.

Mixologic’s picture

@davidhernandez, @joelpittet - would it be better if the URI's had the extension on them based on the original request? i.e. a json payload should have .json uri's and an xml payload should have .xml uri's?

Additionally, we could look and see if we can cross reference some project strings with the numeric data in the payload. i.e. the issue status of '2' or priority of "400" is pretty inside baseball.

markhalliwell’s picture

I went ahead and added anchor #ids on that page for easier linking:
https://www.drupal.org/about-drupalorg/api#restws

Yes, you can use the specific extensions, but it's worth mentioning the entire section:

All requests must either have an .[json|xml] extension or Accept: [application/json|application/xml] request header.

Especially the "or" part there. If you pass the correct header, it will redirect to the correct extension. In (jQuery) AJAX requests, this should be rather seamless and wouldn't require any special headers to be sent (if you simply set the dataType json: http://api.jquery.com/jquery.ajax/).

I can confirm this works as expected:

$ curl -iH 'Accept: application/json' https://www.drupal.org/api-d7/node/1710850
HTTP/1.1 301 Moved Permanently
Age: 0
Cache-Control: no-cache, must-revalidate, post-check=0, pre-check=0
Content-Type: text/html; charset=utf-8
Date: Fri, 08 Aug 2014 01:18:06 GMT
Etag: "1407460686"
Expires: Sun, 19 Nov 1978 05:00:00 GMT
Front-End-Https: on
Last-Modified: Fri, 08 Aug 2014 01:18:06 +0000
Location: https://www.drupal.org/api-d7/node/1710850.json
Server: nginx/1.6.1
Vary: Accept-Encoding
Via: 1.1 varnish
X-Cache: MISS
X-Cache-Svr: www2.drupal.org
X-Drupal-Cache: MISS
X-Varnish: 898300023
Content-Length: 0

edit:
This will give you the JSON (without the .json extension) as it will follow the redirect:
curl -LH 'Accept: application/json' https://www.drupal.org/api-d7/node/1710850

Mixologic’s picture

@Mark Carver - Ah, sweet. I was just looking at it with the Chrome JSON viewer, which doesnt do any smart things like set accept headers. Given that, it seems as though those extensions are returned correctly.

webchick’s picture

Exxxxxcellent. :D :D So nice to see this polished off!

drumm’s picture

Additionally, we could look and see if we can cross reference some project strings with the numeric data in the payload. i.e. the issue status of '2' or priority of "400" is pretty inside baseball.

Documentation like this is great to add to https://www.drupal.org/about-drupalorg/api. A decent number of examples would be helpful.

Internally, those are integer fields with a labeled allowed values list:

1|Bug report
2|Task
3|Feature request
4|Support request

That's a useful pattern, not just for issues. Maybe there is a followup in RestWS or somewhere else to include the labels.

In general, followups like #1978202: Return file info instead of a callback url will be issues for the modules which power this. Our customizations, committed in #123 here, are minimal. RESTful Web Services delivers the responses, and uses Entity API to get data. Hook implementations can be added to drupalorg. Drupal 8 includes REST in core, we should aim to do things that will also be doable in D8.

joelpittet’s picture

Thanks for the example @Mark Carver

Playing with D8 Guzzle and got some stuff going with the following in a controller:

    $client = \Drupal::httpClient();
    $uri = 'https://www.drupal.org/api-d7/node/1710850';
    $res = $client->get($uri, [
      'headers' => ['Accept' => 'application/json'],
    ]);
    var_export($res->json()['field_issue_files']);
markhalliwell’s picture

Re #144:
I certainly agree that in the interim we can manually populate this list on that handbook page. What would be good (better/cool, idk) is if we could just automatically generate this. I know https://www.drupal.org/project/services_docs works well with the services module. There may be something out there that does this already for restws, just haven't really looked too deeply. The concept though seems really feasible: auto generating API docs based solely on what an entity provides.

clemens.tolboom’s picture

Leaving out the .json I get an empty result;

curl --header "Accept: application/json" --request GET http://www.drupal.org/api-d7/node/2317551.json
# Just a redirect
curl --verbose --header "Accept: application/json" --request GET http://www.drupal.org/api-d7/node/2317551

Maybe update https://www.drupal.org/about-drupalorg/api#restws to always use .json?

[edit]No need for --user option[/edit]

patrickd’s picture

This is great news!

Though I'm wondering how to resolve aliases..

eg. I know the project shortname is "faqfield"
meaning the project page is at https://www.drupal.org/project/faqfield
but how do I know that that is an alias for https://www.drupal.org/node/1295680 ?

drumm’s picture

patrickd’s picture

awesome :) thank you!

greg.1.anderson’s picture

It's cool to see some progress here, but I don't know how to get the info I need for drush_iq.

If I fetch from https://www.drupal.org/api-d7/node/2127965.json, then I get a lot of interesting information about that issue, including the field issue files, which looks like this:

"field_issue_files":[{"file":{"uri":"https:\/\/www.drupal.org\/api-d7\/file\/4688627","id":"4688627","resource":"file"},"display":"1"},{"file":{"uri":"https:\/\/www.drupal.org\/api-d7\/file\/4688631","id":"4688631","resource":"file"},"display":"1"},{"file":{"uri":"https:\/\/www.drupal.org\/api-d7\/file\/4689257","id":"4689257","resource":"file"},"display":"1"},{"file":{"uri":"https:\/\/www.drupal.org\/api-d7\/file\/4689259","id":"4689259","resource":"file"},"display":"1"},{"file":{"uri":"https:\/\/www.drupal.org\/api-d7\/file\/4733549","id":"4733549","resource":"file"},"display":"1"},{"file":{"uri":"https:\/\/www.drupal.org\/api-d7\/file\/4872343","id":"4872343","resource":"file"},"display":"1"},{"file":{"uri":"https:\/\/www.drupal.org\/api-d7\/file\/4872345","id":"4872345","resource":"file"},"display":"1"},{"file":{"uri":"https:\/\/www.drupal.org\/api-d7\/file\/4873817","id":"4873817","resource":"file"},"display":"1"},{"file":{"uri":"https:\/\/www.drupal.org\/api-d7\/file\/4875047","id":"4875047","resource":"file"},"display":"1"},{"file":{"uri":"https:\/\/www.drupal.org\/api-d7\/file\/4920553","id":"4920553","resource":"file"},"display":"1"}]

From this, I still do not know (a) how to figure out from the file id or file uri what comment number this file was attached to, (b) who posted the comment containing the file, or (c) how to get the file data itself.

(a): If I ask for https://www.drupal.org/api-d7/comment.json?node=2127965, I get a bunch of useful info about all of the comments for that node, including who posted the comment. However, there is no indication in this data which comments contain file uploads.

(b): If (a) had info about the file attachments for each comment, then I could infer who the file was posted by; however, without this information, I cannot tell who uploaded the file.

(c): If I ask for https://www.drupal.org/api-d7/file/4688627. I can get the file via https://www.drupal.org/files/issues/fix_for_the_new_json-2127965-17.patch, but there is no mapping in the json data from the file number to the file name.

I read the API documentation page, but I couldn't find any pattern similar to the "comment.json" query to get more information about the attachments. I cannot get drush_iq working again until I can determine the mappings from the file to the comment number (so I can generate the author / attribution information string). Any further help here would be appreciated.

Mixologic’s picture

@greg.1.anderson - the file data contains the 'owner' field which is the uid of the user that uploaded the file, for example

https://www.drupal.org/api-d7/file/4688627.json

{
  fid: "4688627",
  name: "drush-iq-2127965-5.patch",
  mime: "text/x-diff",
  size: "3690",
  url: "https://www.drupal.org/files/issues/drush-iq-2127965-5.patch",
  timestamp: "1383843842",
  owner: {
    uri: "https://www.drupal.org/api-d7/user/125814",
    id: "125814",
    resource: "user"
  }
}

So you should be able to key in on the owner:id to get the user id of the submitted patch for author/attribution.

I don't think there is a way with the current api to see the association between a comment and a file.

drumm’s picture

I don't think there is a way with the current api to see the association between a comment and a file.

That will have to be a follow up in either nodechanges, or extended_file_field, module to implement a hook to inject that information somewhere. I think it is nodechanges, but I could be wrong.

Files are not actually technically attached to comments at all, it is a file field on the issue node. Nodechanges keeps track of what changed in the node for each comment, including changes to files.

greg.1.anderson’s picture

@Mixologic: Thanks! I actually tried #152, but made a typo, and it didn't work. :( I tried again, and it is in fact working great. I should have most of the information I need to move forward.

Regarding the file/comment association, when you see files in the web browser, they are labeled with the comment number, as in #101 at the top of this issue. Drush iq allows the user to identify attachments via this number, so I need to know what this association is. I can get most of the drush-iq functionality working again without this one feature, though, so I'm good for now. Thanks.

clemens.tolboom’s picture

@greg.1.anderson are you completely ignoring your own issue queue @ #2127965: Fix for the new json structure on d.o D7 :-/ ?

@others how many are coding PHP to grasp the current d.o. rest api ? Let's team up to nail this bastard :-)

greg.1.anderson’s picture

Just replied there -- sorry for the delay!

greg.1.anderson’s picture

FYI, in the current drush_iq 7.x-2.x branch, iq-apply-patch, iq-info, iq-reset, and ccc are all working with the new d.o. api. Other commands still need a little work.

greg.1.anderson’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.