This module pulls out functionality from the project_issue module and creates a more generic module to display a JSON object for any entity. It is slightly similar to the Content as JSON module (http://drupal.org/project/contentasjson) but can be used with ANY entity, and displays more than just an entity object in JSON. It will parse the data and display the values and markup for each field.

Project page: http://drupal.org/sandbox/ChinggizKhan/1672822
Git Repo: git.drupal.org/sandbox/ChinggizKhan/1672822.git
Core: 7.x

I will try and review other modules in the list, but will also be working on patching some more work for the project_issue D7 upgrade.

Comments

iamcarrico’s picture

iamcarrico commented
Status: Active » Needs review

Oops, forgot to set the status... Also, there might be minor changes depending if there are more issues being brought into the issue queue.

FranciscoLuz’s picture

FranciscoLuz commented
Status: Needs review » Needs work

Hi,

Consider working on these issues here http://ventral.org/pareview/httpgitdrupalorgsandboxchinggizkhan1672822git
The master branch should be left empty.

Besides that your code looks good enough for me.

iamcarrico’s picture

iamcarrico commented
Status: Needs work » Needs review

34 | ERROR | The $text argument to l() should be enclosed within t() so that it is translatable --- This is done purposefully (it is a filename).

All other changes and coding standards have been adhered to.

Mark Theunissen’s picture

Mark Theunissen commented
Status: Needs review » Needs work

I've reviewed the module and it looks good save for one potential issue. The formatting is good, the use case is good (live on Drupal.org), and the approach is good too.

The issue I see is that viewing comments has a different permission, so before attaching the comments to the JSON output, make sure to check the permissions user_access('access comments')) || user_access('administer comments'). This will prevent a case where a user has access to view a node but not the comments, and could then access the comments through the json dump. This shouldn't be a problem on D.org but it would be an issue if people use it in contrib.

Will RTBC when this issue is resolved. Thanks

iamcarrico’s picture

iamcarrico commented
Status: Needs work » Needs review

Added the code, good point, did not think about permissions for comments.

-ChinggizKhan

Mark Theunissen’s picture

Mark Theunissen commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

skottler’s picture

skottler commented
Status: Reviewed & tested by the community » Closed (fixed)

This looks good to me. I agree with the change promoted by @Mark Theunissen. I've promoted the sandbox to be a full project.

Thanks for your contributions!

senpai’s picture

senpai commented

Thank you to @Mark Theunissen, @klausi, and @skottler for keeping our international Drupal.org D7 Upgrade sprint moving!

iamcarrico’s picture

iamcarrico commented

Thank you @Mark Theunissen, @klausi, and @skottler for the quick replies! I look forward to keeping this up and contributing more to the community!

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Closed (fixed) » Fixed

@skottler: now I'm confused. The goal of this process is to grant the git vetted user role to applicants, so that they can promote any future project on their own. See http://drupal.org/node/1011698

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732

manual review:

  1. entity_json_menu_callback(): do not call exit, use drupal_exit() instead (which will also invoke the exit hook for you).
  2. entity_json_json(): that assumes that an entity has a user id assigned, which might not be always the case.
  3. entity_json_help(): the break statement is unreachbale code, remove it.

But otherwise looks good to me.

Thanks for your contribution, ChinggizKhan!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

iamcarrico’s picture

iamcarrico commented
Status: Fixed » Closed (fixed)

Thank you @klausi, I think it was pushed to a full project first as it is necessary code for the project module upgrade to continue. Thank you though for doing a manual review... I have made the few changes requested, and have opened up an issue for #2 to discuss how this should best be handled. #1685010: entity_json_json(): that assumes that an entity has a user id assigned, which might not be always the case.. I will get to it ASAP.