-
Notifications
You must be signed in to change notification settings - Fork 847
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update to Zend's coding style #43
Conversation
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 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.
Is there some style that you think would be more appropriate? |
The styles that I've seen in general use are: PSR2, Zend, and PEAR. I personally hate the multi-line function having it's I think it might be slightly better if Stripe switched to PSR-2. The voting member list is fairly extensive. |
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. |
(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) |
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!).
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.
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. |
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. |
You're still support Ruby 1.8.7 right? Maybe when you end of life that do the same for PHP 5.2? |
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) |
Okay it sounds like sticking with Zend is the best option then and ignore the |
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" | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were you going to look into not putting the )
on a newline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, not sure about how to update #47 I'll look into how to exclude a sniff.
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. |
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. |
I rebased it back onto master 😄 . Might be good to get #47 at the same time to keep it all up to date? |
@@ -115,13 +115,14 @@ public function testUpdateInvalidMetadata() | |||
|
|||
public function testCancelSubscription() | |||
{ | |||
$plan_id = 'gold-' . self::randomString(); | |||
self::retrieveOrCreatePlan($plan_id); | |||
$planID = 'gold-' . self::randomString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there were some $planId
s and $couponId
s lurking around, but no big deal.
Did I not catch them in eda7aa0? |
You did! I was pointing out the minor discrepancy between |
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is probably just me being unfamiliar with PHP conventions--is it typically acceptable to define global constants in libraries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Oh haha yes I see. Good catch 👍 lowercasing / uppercasing that
|
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
I went through the project with CodeSniffer and fixed some of the easy places where it diverged from the Zend style guide.
There is an issue with the indentation being 2 vs 4 spaces but I'm not sure it's worth addressing.