Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Change values of FREQ when get the RFC string to match the convention #16

Merged
merged 2 commits into from
Aug 19, 2016

Conversation

antitoine
Copy link
Contributor

Hi,

In the RFC convention the frequency is a string ("WEEKLY", "YEARLY" ...) not a number when we want to get the RRule string : https://tools.ietf.org/html/rfc5545#page-39

This pull request fix this example :

$rrule = new RRule([
    'FREQ' => 'MONTHLY',
    'INTERVAL' => 1,
    'DTSTART' => '2015-06-01',
    'COUNT' => 6
]);
$copyRrule = new RRule($rrule->rfcString()); // FREQ need to be 'MONTHLY' not 2

@rlanvin
Copy link
Owner

rlanvin commented Aug 17, 2016

Hello,

Thank you for this. The code you are showing is working just fine for me though. Based on your pull request, I'm guessing that the bug you are describing occurs when using the constant RRule::MONTHLY to set the frequency, instead of the string 'MONTHLY'. Is that it?

Before I merge, could you write a test that demonstrate the bug and add it to your PR?

Also, it's kind of a minor thing but still... I use snake case for the variables names, so $frequencyKey should be $frequency_key.

Thanks!

@antitoine
Copy link
Contributor Author

In making these tests, I understood why I needed this modification.
I create a new rule by using the frequency constant in RRule (like RRule::YEARLY), so the integer value is stored in the object and is directly displayed in the RFC string.
Maybe, it will be better if you convert the integer to the real frequency value at the creation of the RRule ?

I hope this new commit meets your expectations

@rlanvin
Copy link
Owner

rlanvin commented Aug 19, 2016

Maybe, it will be better if you convert the integer to the real frequency value at the creation of the RRule ?

Internally it's easier to work with integers and constants. Your fix is the right way to do it. Thanks!

@rlanvin rlanvin merged commit ebd3428 into rlanvin:master Aug 19, 2016
@rlanvin rlanvin mentioned this pull request Dec 29, 2016
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants