Skip to content
This repository has been archived by the owner on Sep 29, 2023. It is now read-only.

Update CreatePlan.php #225

Closed
wants to merge 1 commit into from
Closed

Update CreatePlan.php #225

wants to merge 1 commit into from

Conversation

MartyIX
Copy link
Contributor

@MartyIX MartyIX commented Jan 27, 2015

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 72.43% when pulling bfd3016 on MartyIX:patch-2 into c269976 on paypal:master.

@jziaja
Copy link

jziaja commented Jan 27, 2015

The frequency property is case-insensitive and will accept either "Month" or "MONTH". Also, if you pass in "MONTH", you'll currently get back "Month" from the API.

@MartyIX
Copy link
Contributor Author

MartyIX commented Jan 27, 2015

PaymentDefinition.php file contains the following phpdoc:

/**
 * Frequency of the payment definition offered. Allowed values: `WEEK`, `DAY`, `YEAR`, `MONTH`.
 *
 * @param string $frequency
 * 
 * @return $this
 */
public function setFrequency($frequency)
{
    $this->frequency = $frequency;
    return $this;
}

It seems to me that the naming should follow one convention to prevent possible confusion. However, it is just a suggestion. I can close the pull request. It is not something important to me.

@jziaja
Copy link

jziaja commented Jan 27, 2015

Sorry, wasn't trying to say it was a bad suggestion. :) I prefer the consistency of having it be all caps, but I just wanted to point out that that field happens to accept any case format when making the API call. In the .NET SDK sample for creating a billing plan, I have this field specified as all caps as well.

@MartyIX
Copy link
Contributor Author

MartyIX commented Jan 27, 2015

I see. Thank you for the explanation.

@jaypatel512
Copy link
Contributor

Hey @MartyIX !! I will unfortunately not accept this PR. Even though you are right. These constants should be CAPITALIZED ! :P

However, the response from server is Camel Cased, and we would want to keep it that way. But I will definitely raise this issue internally, to see if we could have all CAPS accepted in Server Side code.

Thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants