Update to Zend's coding style #43

jpiasetz · 2013-07-04T07:46:05Z

I went through the project with CodeSniffer and fixed some of the easy places where it diverged from the Zend style guide.

Indentation should consist of 4 spaces. Tabs are not allowed.

There is an issue with the indentation being 2 vs 4 spaces but I'm not sure it's worth addressing.

ebroder · 2013-07-15T06:38:25Z

Hey - thanks a lot for putting this together, and sorry for not getting back sooner - was dealing with some internal stuff last week.

So admittedly I haven't really done a lot of PHP dev work in a while, so I'm curious - I know I told you we intended to use Zend style, but does it get a lot of usage generally? I mostly like it, though I'm suspicious of something like the closing parens after a multi-line conditional. e.g.

    if ($errno == CURLE_SSL_CACERT ||
        $errno == CURLE_SSL_PEER_CERTIFICATE ||
        $errno == 77 // CURLE_SSL_CACERT_BADFILE (constant not defined in PHP)
        ) {

If I was writing that code, I'd put the close paren on the line before - that formatting looks slightly strange to me. Does it look strange to you? (Totally fine if not - I'm happy to accept that my tastes are just unusual when it comes to PHP)

Same for multi-line function arguments, e.g.

  public function __construct($message, $httpStatus=null,
      $httpBody=null, $jsonBody=null
  ) {

Is there some style that you think would be more appropriate?

jpiasetz · 2013-07-15T07:10:09Z

The styles that I've seen in general use are: PSR2, Zend, and PEAR.

I personally hate the multi-line function having it's ) { on a new line but I think they all call for it. I often just treat it like a code smell and refactor so I don't need it (extracting a class or an options array).

I think it might be slightly better if Stripe switched to PSR-2. The voting member list is fairly extensive.

ebroder · 2013-07-15T16:46:23Z

Ok, good, I'm glad I'm not the only one with that reaction. We could always just ignore that one rule? 😄

I'm not particularly attached to the Zend style if you think PSR-2 would be better. One thing I wonder if we're making any style switches is if we can add something to the test suite to enforce the style guide, otherwise I suspect we'll deviate over time.

ebroder · 2013-07-15T16:56:26Z

(By the way, sorry if I'm being a bit waffly on this. I'm usually pretty opposed to sweeping style changes on existing code bases, but that's mostly in cases where I (and the rest of the people working on the code) have better familiarity with the language in question. In this case, I'm OK with doing a style change if it means that we'll be more in line with what the PHP community expects)

jpiasetz · 2013-07-15T18:39:11Z

No problem. I think it's important to measure twice cut once for public facing code too 👍 . There isn't a huge benefit to switching to PSR-2 right now. If Stripe has plans to drop PHP 5.2 at some point I think it's worth it to switch (PSR-0 autoloading is sweet!).

I'm OK with doing a style change if it means that we'll be more in line with what the PHP community expects)

Probably most of the PHP code in the wild isn't use any style 😦 so I think most people expect the worst when they open something.

One thing I wonder if we're making any style switches is if we can add something to the test suite to enforce the style guide, otherwise I suspect we'll deviate over time.

I use a pre-commit hook with PHP_CodeSniffer to make sure I keep to a certain style. I think it could probably be added to travis. I'll see if I can send a push with it set to zend for now.

ebroder · 2013-07-15T18:47:42Z

Yeah, I can check again, but last time I looked we still had several users using PHP 5.2. We've also gotten a bunch of requests to support namespacing, too, which we also can't do because of 5.2.

jpiasetz · 2013-07-15T19:38:15Z

You're still support Ruby 1.8.7 right? Maybe when you end of life that do the same for PHP 5.2?

ebroder · 2013-07-15T20:00:34Z

That's right, though basically my thinking about Ruby 1.8.7 is that it's hard to imagine something that would pressure us to drop support for 1.8.7 (it works, and it's easy to keep working), so we shouldn't be the impetus that forces our users to change their code. (So basically we're not currently planning to drop support for 1.8.7)

jpiasetz · 2013-07-16T16:21:43Z

Okay it sounds like sticking with Zend is the best option then and ignore the ) on a new line rule.

ebroder · 2013-07-26T05:46:47Z

lib/Stripe/ApiRequestor.php

-    if (is_string($value) && mb_detect_encoding($value, "UTF-8", TRUE) != "UTF-8")
+    if (is_string($value)
+        && mb_detect_encoding($value, "UTF-8", TRUE) != "UTF-8"
+    ) {


Were you going to look into not putting the ) on a newline?

Done, not sure about how to update #47 I'll look into how to exclude a sniff.

jpiasetz · 2013-07-29T04:49:50Z

I mucked up my rebase a bit so the commits might not be as atomic as they should be. I'll correct them if you spot any.

michelle · 2014-03-07T23:57:28Z

Ah, sorry for all the delay on this--this is a bit out of date now :(. I'll put it on my list to manually merge this in soon.

jpiasetz · 2014-03-08T17:20:42Z

I rebased it back onto master 😄 . Might be good to get #47 at the same time to keep it all up to date?

michelle · 2014-03-08T19:49:48Z

test/Stripe/CustomerTest.php

@@ -115,13 +115,14 @@ public function testUpdateInvalidMetadata()

  public function testCancelSubscription()
  {
-    $plan_id = 'gold-' . self::randomString();
-    self::retrieveOrCreatePlan($plan_id);
+    $planID = 'gold-' . self::randomString();


I think there were some $planIds and $couponIds lurking around, but no big deal.

jpiasetz · 2014-03-08T20:04:52Z

Did I not catch them in eda7aa0?

michelle · 2014-03-08T20:07:03Z

You did! I was pointing out the minor discrepancy between planID v. planId.

michelle · 2014-03-08T20:08:19Z

lib/Stripe/ApiRequestor.php

@@ -175,15 +195,21 @@ private function _curlRequest($meth, $absUrl, $headers, $params)
    curl_setopt_array($curl, $opts);
    $rbody = curl_exec($curl);

+    if (!defined('CURLE_SSL_CACERT_BADFILE')) {
+      define('CURLE_SSL_CACERT_BADFILE', 77);  // constant not defined in PHP


Hmm, this is probably just me being unfamiliar with PHP conventions--is it typically acceptable to define global constants in libraries?

I think it's a lesser of two evils. I wouldn't normally do it in case of a collision with another library however in this case it a curl constant that's missing. Alot of other people seem to have come up with the same solution https://github.com/search?l=php&q=CURLE_SSL_CACERT_BADFILE&ref=cmdform&type=Code.

I meant to track down why PHP didn't have it / open a bug if there wasn't one when I first wrote this.

jpiasetz · 2014-03-08T20:17:47Z

Oh haha yes I see. Good catch 👍 lowercasing / uppercasing that
is my bain.
On Mar 8, 2014 3:07 PM, "Michelle Bu" notifications@github.com wrote:

You did! I was pointing out the minor discrepancy between planID v. planId
.

Reply to this email directly or view it on GitHubhttps://github.com//pull/43#issuecomment-37108271
.

jpiasetz · 2014-03-10T15:30:02Z

I fixed the Id vs ID. How much do you care about history? Do you want me to squash it in?

Update to Zend's coding style

jpiasetz mentioned this pull request Jul 15, 2013

Add phpcs to travis to check that code matches zend style #47

Closed

ebroder reviewed Jul 26, 2013
View reviewed changes

jpiasetz added 12 commits March 8, 2014 10:58

Switch tabs to spaces for indenting lines

a0a3b09

Change lines to be less then 80 characters

97da793

Change control structure spacing

56125af

Update multiline function calls indentation

2c8c009

Rename variables to not contain numbers

a986e9c

Change variables to Camel Case

f15eb90

Change member variables declared "public" that start with an underscore.

2f207fb

Putting function opening brace on a new line

06e75e1

Keep ( on a seperate line from the brace

e1b8a2a

Clean up long line

6ceb314

Opening brace should be on new line

9a2d2f1

Multi-line function call not indented correctly

a4716b5

Change one last snake_case to camelCase

eda7aa0

michelle reviewed Mar 8, 2014
View reviewed changes

Fixing case on planId vs planID/couponId vs couponID

fe965cf

michelle added a commit that referenced this pull request Mar 10, 2014

Merge pull request #43 from jpiasetz/master

1a07e2c

Update to Zend's coding style

michelle merged commit 1a07e2c into stripe:master Mar 10, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to Zend's coding style #43

Update to Zend's coding style #43

jpiasetz commented Jul 4, 2013

ebroder commented Jul 15, 2013

jpiasetz commented Jul 15, 2013

ebroder commented Jul 15, 2013

ebroder commented Jul 15, 2013

jpiasetz commented Jul 15, 2013

ebroder commented Jul 15, 2013

jpiasetz commented Jul 15, 2013

ebroder commented Jul 15, 2013

jpiasetz commented Jul 16, 2013

ebroder Jul 26, 2013

jpiasetz Jul 29, 2013

jpiasetz commented Jul 29, 2013

michelle commented Mar 7, 2014

jpiasetz commented Mar 8, 2014

michelle Mar 8, 2014

jpiasetz commented Mar 8, 2014

michelle commented Mar 8, 2014

michelle Mar 8, 2014

jpiasetz Mar 10, 2014

jpiasetz commented Mar 8, 2014

jpiasetz commented Mar 10, 2014

Update to Zend's coding style #43

Update to Zend's coding style #43

Conversation

jpiasetz commented Jul 4, 2013

ebroder commented Jul 15, 2013

jpiasetz commented Jul 15, 2013

ebroder commented Jul 15, 2013

ebroder commented Jul 15, 2013

jpiasetz commented Jul 15, 2013

ebroder commented Jul 15, 2013

jpiasetz commented Jul 15, 2013

ebroder commented Jul 15, 2013

jpiasetz commented Jul 16, 2013

ebroder Jul 26, 2013

Choose a reason for hiding this comment

jpiasetz Jul 29, 2013

Choose a reason for hiding this comment

jpiasetz commented Jul 29, 2013

michelle commented Mar 7, 2014

jpiasetz commented Mar 8, 2014

michelle Mar 8, 2014

Choose a reason for hiding this comment

jpiasetz commented Mar 8, 2014

michelle commented Mar 8, 2014

michelle Mar 8, 2014

Choose a reason for hiding this comment

jpiasetz Mar 10, 2014

Choose a reason for hiding this comment

jpiasetz commented Mar 8, 2014

jpiasetz commented Mar 10, 2014