Ajax Nodeloader

Project page URL? Very useful!

The GIT URL is wrong. Only you can log in like nick-denry@... Good URL: git clone http://git.drupal.org/sandbox/nick-denry/1447152.git ajax_nodeloader

Yeah, the project's page: http://drupal.org/sandbox/nick-denry/1447152

Also, explain what it does better. I shouldn't have to download it to know that. Load what? Load how? And do what with it?

Put all this in the original issue, not a comment.

Fix git url for non-mantainer

Good URLs

Oeh I like this one...

A few functional things though:

1. HTML 5 introduces the History API. (Is that what it's called?) history.pushState() and the popstate event in particular. Use that. Nobody likes #.

2. The node view isn't complete. Reproduction is simple: add and display a few fields. Load them via nodeloader. Only the body will show. What if I don't have a body, but I do have a lot of fields?

3. If I link to /, I get a 404 back. The frontpage isn't ajax_nodeloadable?

And a few technical things:

1. Don't define functions like load_node if your module is called ajax_nodeloader. Prefix everything with your module's name. I.e. ajax_nodeloader_load_node.

2. Don't do this:

    $language = 'und';

What's und? Use LANGUAGE_NONE, if you really must. But don't. Use field_view_field to load a node's field values. Explore the Field API. It's awesome. Drupal contains (almost) everything you need. Things like this aren't necessary:

		$system_path = drupal_lookup_path('source',$nid);
		$path = split('/',$system_path);
		if ($path[0] == 'node')

3. Automated code review issues here. You can use this tool yourself.

Nope. Not sarcasm. I like it. (That's why there's "though" in first issues title.) Are you worried?

PS. There's a Quote button in your editor. Use that instead of >>. For code, use the code or PHP button. Etc.

A module's usefulness is a condition. There are way too many useless, bad, broken and identical modules around. It also means a module maker has to think about their module (and its usefulness) and that's good.

I can't find a module that does what (I think this module) does/should do, so that's good. Can you?

1. You can use HTML 5 as progressive enhancement. If it's not available, use the #path option. Next version is fine.

2. I mean any and all fields. Try it. Add a few fields to your node type, fill them and load that load via your module. The fields won't show. As bad: remove the body field and you'll get a notice. Not all node types have a body field. You can remove it and some people do that. So instead of showing the node's title & body, show all of it, or whatever the user has set in his node type's "Manage display" tab. Follow and explore the URL I gave you. The Field API is awesome.

3. You could make any page ajax loadable by sending the full URI. Substract Drupal.settings.basePath from the link's href and send that part. Might be "node/1" or "" (empty for frontpage) or "content/some-alias/349". You can use Drupal's router to find the node. Not difficult, but very powerful. (You might have to google some.)

Don't expect responses as quick as this any more =) There are over a hundred modules in line for review, so I should've let you wait a few days. Take a few days to perfect it. Your friends: field_view_field, node_view, menu_get_item etc.

Welcome with your application!

A short manual review:

Master Branch

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

License

Please remove the LICENSE.txt file. Drupal will add the appropriate version automatically during packaging so your repository should not include it.

ajax_nodeloader.info

Please remove $Id$ and package = Other and version (Drupal does not use this kind of versions)

ajax_nodeloader.module

$output = false should be $output = FALSE, if you really need to set a FALSE value here?

Always name your functions with the modul name first, function load_node() should be ajax_nodeloader_load_node or maybe better _ajax_nodeloader_load_node.

ajax_nodeloader.js

It says: Original JavaScript code. Is this by somebody else?

ajax-loader.gif

Have you created this image, or is it taken from somewhere?

Automatic review:
http://ventral.org/pareview/httpgitdrupalorgsandboxnick-denry1447152git

You have some code formatting to take care about.

Please ask if you have any questions. We are here to help each other.

Welcome with your application!

A short manual review:

Master Branch

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

License

Please remove the LICENSE.txt file. Drupal will add the appropriate version automatically during packaging so your repository should not include it.

ajax_nodeloader.info

Please remove $Id$ and package = Other and version (Drupal does not use this kind of versions)

ajax_nodeloader.module

$output = false should be $output = FALSE, if you really need to set a FALSE value here?

Always name your functions with the modul name first, function load_node() should be ajax_nodeloader_load_node or maybe better _ajax_nodeloader_load_node.

ajax_nodeloader.js

It says: Original JavaScript code. Is this by somebody else?

ajax-loader.gif

Have you created this image, or is it taken from somewhere?

Automatic review:
http://ventral.org/pareview/httpgitdrupalorgsandboxnick-denry1447152git

You have some code formatting to take care about.

Please ask if you have any questions. We are here to help each other.

Had a brief look as I'm looking into doing this properly myself.

It doesn't seem to use a lot of the standard drupal 7 javascript stuff (which is why I checked this module, I'm looking for examples)
Also has some drupal coding standards errors (like the placement of the { bracket on a new line)

I also dont think it will attach js behaviors and css, which seems like a must to me?

google is always a good friend: http://drupal.org/coding-standards

Fix typeerrors

Might be interesting to you:
http://www.computerminds.co.uk/comment/2014

(optional) Before committing a major change, add a change log entry to CHANGELOG.txt that summarizes the change. Wrap the text at 80 characters. Some projects are listing all changes in CHANGELOG.txt; you should follow the existing or agreed-on practice.

source: http://drupal.org/node/363367
Just check the changelog of drupal core to see how it goes..
http://api.drupal.org/api/drupal/CHANGELOG.txt/7

On topic though, your JS part may be very minimal, everything you need is in core.
I'm not even sure that you need a JS file at all..

First: The drupalcs -pareview of the 7.0-branch ...

FILE: ...w/sites/all/modules/pareview_temp/test_candidate/ajax_nodeloader.module
--------------------------------------------------------------------------------
FOUND 13 ERROR(S) AND 2 WARNING(S) AFFECTING 9 LINE(S)
--------------------------------------------------------------------------------
  20 | WARNING | Format should be "* Implements hook_foo()." or "Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar()."
  50 | WARNING | Line exceeds 80 characters; contains 88 characters
  84 | ERROR   | Inline comments must start with a capital letter
  84 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
  87 | ERROR   | Inline comments must start with a capital letter
  87 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
  91 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
  91 | ERROR   | There must be no blank line following an inline comment
  91 | ERROR   | Comments may not appear after statements.
  93 | ERROR   | Expected "foreach (...) {\n"; found "foreach(...) {\n"
  94 | ERROR   | Inline comments must start with a capital letter
  94 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
  97 | ERROR   | Inline comments must start with a capital letter
  97 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
 100 | ERROR   | Closing brace indented incorrectly; expected 6 spaces, found 7
--------------------------------------------------------------------------------


FILE: .../sites/all/modules/pareview_temp/test_candidate/css/ajax_nodeloader.css
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
 4 | ERROR | Multiple CSS properties should be listed in alphabetical order
 6 | ERROR | Multiple CSS properties should be listed in alphabetical order
 9 | ERROR | Multiple CSS properties should be listed in alphabetical order
--------------------------------------------------------------------------------

Source: http://ventral.org/pareview - PAReview.sh online service Retry: (http://ventral.org/pareview/httpgitdrupalorgsandboxnick-denry1447152git-...)

There's nothing serious, just comments, whitespace etc. - easy to fix.

Micha

This isn't working for me. With the following code, when I click the link, all fields from the target node load replacing all of my current page including the current page title. It isn't refreshing the page so it is kind of working.

Also, I have a field with multiple images and it is only pulling up one of the images. Take a look and let me know if I am doing something wrong???

<a href="/node/139" class="nodeloader" rel={'title':'#selector1'}>Only title custom target</a>
<br />
<br />
--over--
<div id="selector1"></div>
--under--

Thanks

Manual review:

Branch names

You need to change the branch names, it should be like 7.x-1.x, not 7.x-0.x

Suggestion

Why do you use echo for the result in ajax_nodeloader_load_node? Why not return?

Anyhow, this looks RTBC for me.

The module has security vulnerabilities. It doesn't check node access rights.
Here is some usefull information about the node access system.
http://api.drupal.org/api/drupal/modules!node!node.module/group/node_access/7

Added security tag.

With your module anybody who has «access content» permission can view unpublished nodes.

Isn't it a duplicate for http://drupal.org/project/ajaxit project? Looks like ajaxit can make any page to load through AJAX request using jQuery library, not just a node page.

And one more similar project - http://drupal.org/project/ajax_regions

AndyB

@Nick Denry, you misunderstood me. The issue is not about published and unpublished nodes. It's just a particular case. You should use a node access system provided by Drupal api. There is a lot of content access modules that govern access to nodes but your module allows users to bypass the access control from these modules.

Manual Review;

1. load_node should be ajax_nodeloader_load_node().

2. you should use field_get_items to get field value.
for example : to get body field value
field_get_items('node', $node, 'body', $node->language);

3. If someone enters the path through node_load/node/nid in browser, anonymous users can see the node contents even if unpublished. May have some security issues for some sites.

4. Deprecated function: Function split() is deprecated in load_node() (line 43 of /var/www/html/drupal-7.14/sites/all/modules/ajax_nodeloader/ajax_nodeloader.module).
This function has been DEPRECATED as of PHP 5.3.0.
you should use explode function.

5. drupal_add_http_header('Content-Type', 'text/javascript; charset=utf-8'); is this necessary?

@Nick Denry, please do not review the reviews. We have trouble to getting people to do reviews for me beginning.

And your repo have a lot of branches. @xenyo reviewed the 7.x-0.x branch, which still looks like stated in #47. That branch should be deleted, like 6.x-0.x.

So please clean up your repos, so people more easy could do reviews. Thanks.

To delete remote branches you have to do like:
git push origin :6.x-0.x

Offtopic: It is drupal, we are all volunteers.

I think you should remove drupal.org from the the push.

I'd like to set this to RTBC, it's been dormant for a few weeks. The remaining ventral issues are minor, http://ventral.org/pareview/httpgitdrupalorgsandboxnick-denry1447152git, just Line exceeds 80 characters; and Expected 1 space before "+";. I can't speak to the JS security, but the issues warranting the security tag in #38 were resolved on June 27 and the rest of the code looks good to me. Nick is definitely responsive in resolving issues that arise and dedicated to this module. I think this module has a lot of real-world use cases; I am actively using this on a site in development and hope it gains some traction before D8.

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 :-)

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 .
Delete master branch:

git checkout 7.x-1.x
git branch -D master
git push origin :master

Review of 7.x-1.x:

• In drupal 7 should add css and javascript files that should always be loaded into your .info so they can be aggregated - don't load them everytime in hook_init()!
• variable_get and _set can work with TRUE and FALSE without problems - always use TRUE/FALSE for boolean values instead of 0/1
• Only hook functions should be documented with "Implements ..", for example hook_admin() does not exist!
• hook_init() IS a hook, document it with "Implements hook_load()."
• * @nid Incorrect doxygen documentation, should be like @param int $encoded_url
• Use /**/ for block comments in global space, use // within functions
• 'title' => 'load node via alias', sounds like a comment, not like a page title - use a comment
• 'access arguments' => array('administer ajax_nodeloader'), this permission does not exist - you have to define it by hook_permission()
• In drupal 7 there is drupal_json_output(), you don't have to make use of json_encode and set the conent type manually.
• If node access is false your using the $output variable, which is still a boolean, as array.

Review of 6.x-1.x

• variable_get and _set can work with TRUE and FALSE without problems - always use TRUE/FALSE for boolean values instead of 0/1
• Only hook functions should be documented with "Implements ..", for example hook_admin() does not exist!
• hook_init() IS a hook, document it with "Implements hook_load()."
• * @encoded_url Incorrect doxygen documentation, should be like @param int $encoded_url

• // Unfortunally, we need to use pure json_encode instead
    // drupal_to_js function to prevent wrong utf-8 json encoding
    // @see http://drupal.org/node/1086098
    // TODO: json_encode for php < 5.2
    $result = json_encode($output);
    drupal_set_header('Content-Type: text/javascript; charset=utf-8');

  What about drupal_json()?

• If node access is false your using the $output variable, which is still a boolean, as array.

None of these points are super-important, anyway I'd like to see some of them fixed.
Set it back to RTBC yourself after your done.

@Nick Denry

Keep going Nick. I think this is a good idea, I like the fact that you have it in both D6 and D7 and I bet you could get a D8 version up and running pretty quickly. I'll try it out the module next chance I get.

In the meantime, your sandbox still has a master branch. I had trouble with getting rid of this myself.

To delete it, you must first set a different 'Default branch' on http://drupal.org/node/1447164/edit

Set it to 7.x instead of master. Then you will be able to delete master from the remote origin.

git ls-remote origin
git push origin :master

Cheers
-Dio

+1 for "Keep going". Cool idea. I disagree with anyone suggesting you are duplicating existing modules, from what I've found.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

If you reopen this please keep in mind that 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 :-)

Typeerrors fix