Coding standards and comments

Andy,

One of us needs to re-roll this patch - I've made a bunch of commits and have broke it (sorry!)

I'll get to it this week if you don't get to it first.

Thanks,
-mike

If you love Coder module, then this is an easy patch for you!

We need a re-roll for the 2.x branch.

Thanks,
-mike

@ultimike

I got this....

Awesome! Thanks for jumping in!

Thanks,
-mike

Here is the patch.

I only have two warnings and I believe that I can not fix it because this is the XML call that goes to UPS. Changing the tag name will have and effect on UPS end (I believe, correct me if I'm wrong). Because XML is case sensitive.

commerce_ups.xml.inc

severity: minorreview: style_lowercase_html
 Line 66: use lowercase html te will have and effect on UPS eags to comply with XHTML [style_lowercase_html]  $access_request = new SimpleXMLElement('<AccessRequest/>');
severity: minorreview: style_lowercase_html
Line 71: use lowercase html tags to comply with XHTML [style_lowercase_html]  $rating_request = new SimpleXMLElement('<RatingServiceSelectionRequest/>');

Other than warning this patch should be able to make this Drupal Coding Standards friendly.

Darryl,

Great job so far! I found a few issues though...

1. +++ b/commerce_ups.admin.inc
   @@ -5,14 +5,16 @@
   +    '#description' => check_plain('In order to obtain shipping rate estimates, you must have an account with UPS. You can apply for UPS API credentials at !ups', array('!ups' => $ups_link)),
   

   I think we want to stay with the t() function for translations.

2. +++ b/commerce_ups.admin.inc
   @@ -232,9 +233,9 @@ function commerce_ups_settings_form($form, &$form_state) {
   +      UPS has a ' . t('maximum package size', 'http://www.ups.com/content/us/en/resources/prepare/oversize.html', array('attributes' => array('target' => '_blank'))) . '.',
   

   This should remain with a l() not changed to a t() function.

3. +++ b/commerce_ups.module
   @@ -125,7 +128,7 @@ function commerce_ups_commerce_shipping_service_rate_options_alter(&$options, $o
   +      foreach ($options as $key => & $option) {
   

   Whoops - surely there shouldn't be a space between "& $option"...

4. +++ b/commerce_ups.module
   @@ -251,7 +260,7 @@ function commerce_ups_encryption_available($options = array()) {
   +      $warnings[] = 'AES Encryption is installed but not configured securely. Please go ' . ('configure AES Encryption to use file storage', 'admin/settings/aes') . ' to enable encryption for UPS credentials.';
   

   Looks like you mistakenly remove the l() function here...

5. +++ b/commerce_ups.xml.inc
   @@ -113,10 +118,12 @@ function commerce_ups_build_rate_request($order) {
   +    /*
   

   Should be /**

6. +++ b/tests/commerce_ups.test
   @@ -1,8 +1,13 @@
   + * Administration tests for my module.
   

   While we're here, this should probably refer to the "Commerce UPS" module, not "my module".

7. +++ b/tests/commerce_ups.test
   @@ -32,18 +37,24 @@ class CommerceUPSAdministrationTestCase extends CommerceBaseTestCase {
   +/**
   + * Log in as store admin.
   + * $this->drupalLogin($this->site_admin);
   + * Access to the config page for checkout forms
   + */
      public function testCommerceUPSSettingsForm() {
   -    // Log in as store admin.
   -    //$this->drupalLogin($this->site_admin);
   -    // Access to the config page for checkout forms
   

   I think we can probably remove the comment completely.

Thanks,
-mike

@Mike,

I have made all the changes that you request. Does changes cause some warning/errors in the code. And I just wanted to make sure that you are aware of them.....

commerce_ups.module

Line 263: The $text argument to l() should be enclosed within t() so that it is translatable. [i18n_0]
      $warnings[] = 'AES Encryption is installed but not configured securely. Please go ' . l('configure AES Encryption to use file storage', 'admin/settings/aes') . ' to enable encryption for UPS credentials.';
HIDESITES/ALL/MODULES/COMMERCE_UPS/COMMERCE_UPS.ADMIN.INC

commerce_ups.admin.inc

Line 17: Potential problem: FAPI elements '#title' and '#description' only accept filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized. (Drupal Docs) [security_fapi_title]
    '#description' => t('In order to obtain shipping rate estimates, you must have an account with UPS. You can apply for UPS API credentials at !ups', array('!ups' => $ups_link)),

Line 236: The $text argument to l() should be enclosed within t() so that it is translatable. [i18n_0]
      UPS has a ' . l('maximum package size', 'http://www.ups.com/content/us/en/resources/prepare/oversize.html', array('attributes' => array('target' => '_blank'))) . '.',

Anyway here is the patch... Please let me know if you find something wrong.

Duplicated Post Sorry...

One minor note: the link to AES settings page is not correct.
Instead of admin/settings/aes it should be admin/config/system/aes.

Thanks for the work guys!

Actually there are some other issues with AES settings form, so I will post that together in a separate patch.

Few little nitpicks but this looks like a great clean-up thank you!

1. +++ b/commerce_ups.module
   @@ -49,7 +51,8 @@ function commerce_ups_commerce_shipping_service_info() {
   +    // if you find a selected one...
   

   Minor nitpick, this should be a Capitol I in If as the start of a sentence.

2. +++ b/commerce_ups.module
   @@ -119,13 +122,13 @@ function commerce_ups_service_rate_order($shipping_service, $order) {
   +      foreach ($options as $key => & $option) {
   

   This change is not part of the drupal standards.

3. +++ b/commerce_ups.xml.inc
   @@ -27,12 +27,14 @@ function commerce_ups_build_rate_request($order) {
   +  // this returns $weight['unit'] and $weight['weight']
   ...
   +  // this returns $volume['unit'] and $weight['volume']
   

   Capitalize 'this' and put a period at the end.

4. +++ b/commerce_ups.xml.inc
   @@ -113,10 +118,12 @@ function commerce_ups_build_rate_request($order) {
   +     * If the weight is less than 0.1, set it to 0.1. I tried to find some "official" documentation
   +     * for this on the UPS site, but could not. I did find that other ecommerce platforms are
   +     * using this same logic though, I think it is safe for now. mta
   

   Should be wrapped on 80 characters and likely should be inline commenting styles. Also what is mta?

I have accomodate all changes as per #13 except 2 as it is not clear to me.

Oh sorry about point 2, that is totally not clear, lol.

I meant the space between the reference ampersand and the variable there.
&$ is the convention we follow.

Thanks for the cleanup!

@joelpittet,

I have added a patch that does not have space between &$.

Very nice cleanup, thanks @darol100!

Shouldn't this:

+++ b/commerce_ups.xml.inc
@@ -27,12 +27,14 @@ function commerce_ups_build_rate_request($order) {
+  // this returns $weight['unit'] and $weight['weight']
...
+  // this returns $volume['unit'] and $weight['volume']

be:

+++ b/commerce_ups.xml.inc
@@ -27,12 +27,14 @@ function commerce_ups_build_rate_request($order) {
+  // this returns $weight['unit'] and $weight['weight']
...
+  // this returns $volume['unit'] and $volume['volume']

@jcherbert yes you are right, feel free to roll that into the patch or the maintainer can fix that on commit.

Thanks these have been committed.