drupal_to_js() and drupal_json() are very confusing function names. I suggest we do the following:

  • drupal_to_js() is just a wrapper around PHP json_encode(), let's rename it drupal_json_encode(), as we do elsewhere
  • drupal_json() outputs a PHP datastructure after converting it to JSON, let's rename it drupal_output_json()
Files: 
CommentFileSizeAuthor
#13 556018-rename-drupal-to-js-drupal-json_4.patch11.14 KBdawehner
Unable to apply patch 556018-rename-drupal-to-js-drupal-json_4.patch
[ View ]
#11 556018-rename-drupal-to-js-drupal-json_3.patch10.6 KBdawehner
Failed: Failed to run tests.
[ View ]
#7 556018-rename-drupal-to-js-drupal-json.patch13.05 KBDamien Tournoud
Failed: Failed to run tests.
[ View ]
#5 556018-rename-drupal-to-js-drupal-json_1.patch13.05 KBdawehner
Failed: Failed to run tests.
[ View ]
#3 556018-rename-drupal-to-js-drupal-json.patch11.4 KBDamien Tournoud
Failed: Failed to run tests.
[ View ]
#1 556018-rename-drupal-to-js-drupal-json.patch42.44 KBDamien Tournoud
Failed: Failed to apply patch.
[ View ]

Comments

Damien Tournoud’s picture

Status:Active» Needs review
StatusFileSize
new42.44 KB
Failed: Failed to apply patch.
[ View ]

Here is the patch.

Status:Needs review» Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

Status:Needs work» Needs review
StatusFileSize
new11.4 KB
Failed: Failed to run tests.
[ View ]

Something weird happened ;)

dmitrig01’s picture

The names look logical and much better. i can't really test the patch because i don't have the hardware to do so at the moment, but i'm giving it my +1. I've read through the code and it *looks* right.

dawehner’s picture

StatusFileSize
new13.05 KB
Failed: Failed to run tests.
[ View ]

Why starts drupal_output_json with output, but drupal_json_encode starts with json, after the drupal.

Shouldn't be both functions be named drupal_json_%

I just rewrote the patch.

Status:Needs review» Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

Status:Needs work» Needs review
StatusFileSize
new13.05 KB
Failed: Failed to run tests.
[ View ]

Resubmitting for the test bot.

Status:Needs review» Needs work

The last submitted patch failed testing.

sun’s picture

Tests failed for whatever reason :-/

Due to the new function names, some PHPDoc exceeds 80 chars in that patch.

sun’s picture

Issue tags:+API clean-up

Tagging.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new10.6 KB
Failed: Failed to run tests.
[ View ]

Rerole to remove hunks.

Additional fixed the 80 chars PHPDoc

Status:Needs review» Needs work

The last submitted patch failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new11.14 KB
Unable to apply patch 556018-rename-drupal-to-js-drupal-json_4.patch
[ View ]

I missed one rename.

Dave Reid’s picture

Status:Needs review» Reviewed & tested by the community

Looks really good and +1 for the name changes to help make it more clear.

This review is powered by Dreditor.

sun’s picture

yeah, looks ready to fly.

Too bad that we don't support renderable arrays for json. Because, if we would, my first guess was that we'd like to rename drupal_json_output() to drupal_render_json() - going hand in hand with drupal_render_page()... :) However, since these are simple arrays currently, that doesn't make sense. Anyway, just ignore this paragraph ;)

Dries’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs Documentation

OK, that looks like a nice clean-up. Committed to CVS HEAD. Thanks all.

Let's document this in the upgrade instructions and mark as 'fixed' after.

dawehner’s picture

Status:Needs work» Needs review

Add update instructions http://drupal.org/update/modules/6/7

sun’s picture

Status:Needs review» Fixed

Status:Fixed» Closed (fixed)
Issue tags:-Needs Documentation, -API clean-up

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