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()
CommentFileSizeAuthor
#13 556018-rename-drupal-to-js-drupal-json_4.patch11.14 KBdawehner
#11 556018-rename-drupal-to-js-drupal-json_3.patch10.6 KBdawehner
#7 556018-rename-drupal-to-js-drupal-json.patch13.05 KBDamien Tournoud
#5 556018-rename-drupal-to-js-drupal-json_1.patch13.05 KBdawehner
#3 556018-rename-drupal-to-js-drupal-json.patch11.4 KBDamien Tournoud
#1 556018-rename-drupal-to-js-drupal-json.patch42.44 KBDamien Tournoud
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Active » Needs review
FileSize
556018-rename-drupal-to-js-drupal-json.patch42.44 KB

Here is the patch.

Status: Needs review » Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs work » Needs review
FileSize
556018-rename-drupal-to-js-drupal-json.patch11.4 KB

Something weird happened ;)

dmitrig01’s picture

dmitrig01 CreditAttribution: dmitrig01 commented

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

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
556018-rename-drupal-to-js-drupal-json_1.patch13.05 KB

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

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs work » Needs review
FileSize
556018-rename-drupal-to-js-drupal-json.patch13.05 KB

Resubmitting for the test bot.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Tests failed for whatever reason :-/

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

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue tags: +API clean-up

Tagging.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
556018-rename-drupal-to-js-drupal-json_3.patch10.6 KB

Rerole to remove hunks.

Additional fixed the 80 chars PHPDoc

Status: Needs review » Needs work

The last submitted patch failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
556018-rename-drupal-to-js-drupal-json_4.patch11.14 KB

I missed one rename.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
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

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

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

Dries CreditAttribution: Dries commented
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

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review

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

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
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.

Issue tags: +Needs documentation, +API clean-up

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.