benplummer / calendarful Goto Github PK

Within the comments, you've noted that PHP 5.3+ is required, yet the composer.json doesn't require this. composer.json needs updating to reflect the dependency:

...
require: { 
    'php': '>=5.3'
}

Currently getStartDate and getEndDate have a return type of mixed, which is totally unnecessary. If an event is supposed to represent an absolute moment in time, the start and end values must be exact and can therefore be represented as DateTime objects.

This also simplifies logic later, such as in Calendar::removeOutOfRangeEvents because you can compare without formatting:

// existing code
if ($event->getStartDate() <= $toDate->format('Y-m-d H:i:s') && $event->getEndDate() >= $fromDate->format('Y-m-d H:i:s')) { ... }

// becomes
if ($event->getStartDate() <= $toDate && $event->getEndDate() >= $fromDate) { ... }

Hi,

First let me say that your package looks great. But with greatnes comes remarks. I hope you'll take mine as constructive 👍

When you think about it, what is a Calendar event ? It's something "unique" that occurs between 2 specific timepoints. When you modify at least one of these timepoints or the Event ID, you change the event as a whole. Basically a calendar event is a TimeRange immutable value object with a unique ID. So IMHO, the simplest event interface should be written as below:

interface Event
{
    public function getId();

    public function getTimeRange(); //returns a Time Range Immutable Value Object
}

Now to be completely honest, I'm the author of such package, League\Period\Period, so you may think that I'm trying to push my code into your package. But that's not the intent. I like your code and I want it to focus on what it does best creating recurring events and not "wasting your time" in time range manipulation. Your package needs a real TimeRange class to greatly simplify its code.

You would then end up with two Interfaces but with clear functionalities. All calendar events do not need to be recurrent but recurrent events must be calendar events.

interface RecurrentEvent extends Event
{
    public function getParentId();
    public function getRecurrenceType();
    public function setRecurrenceType($type = null);
    public function getRecurrenceUntil();
    public function getOccurrenceDate();
}

What do you think ?

Currently in the default package Recurrence Types, when an occurrence of a Recurrent Event is generated it is a clone of the Recurrent Event as shown here:
https://github.com/benplummer/calendarful/blob/master/src/Recurrence/Type/Daily.php#L93

This means that every occurrence will be an implementation of the RecurrentEventInterface. This is not ideal as only Recurrent Events should follow that interface.

Generated occurrences should be instances of the standard EventInterface as occurrences of Recurrent Events ideally should not recur themselves.

The default Recurrence Type code does call methods which should have implementations that remove recurrent related property values (https://github.com/benplummer/calendarful/blob/master/src/Recurrence/Type/Daily.php#L102-L108) however this makes assumptions that the user is going to do this. If they don't, occurrences could also be recurrent which may not have nice results.

Ideally, occurrences should be implementations of the EventInterface but the solution is not clear.

Custom Recurrence Types that users inject into the package are fine as they can address app code and instantiate models explicitly. The issue resides in the default package Recurrence Types that are based on Event abstractions.

I am not entirely sure the cause of this, but an odd problem showed up when I ran the following line:

$recurrenceFactory->addRecurrenceType('weekly', 'Plummer\Calendarful\Recurrence\Type\Weekly');
$calendar = Calendarful::create($recurrenceFactory)->populate($eventRegistry, new DateTime('first day of this month'),new DateTime('last day of this month'));

When the code in the Weekly.php line 75 is run I get an error of:
DateTime::modify(): Failed to parse time string (P1D) at position 1 (1): Unexpected character

If I change the code to

while ($startMarker->format('w') != $day) {
    $startMarker->modify('+1 day'); // <-- was 'P1D'
}

it works fine. I am still rather new to PHP so I am not exactly sure whats going on, but Im guessing its a PHP bug, but thought I'd pass this along if it was of any help.

Hi!

Thanks for the great library!

What is the recommended database setup?
Do you maybe have something like migration scripts and db seeders?

There is a really fantastic package called Carbon that could be used to make a lot of the interval and comparison code easier to understand.

For example, this code:

// existing
list(, $dailyEventTime) = explode(' ', $dailyEvent->getStartDate());
...
$newStartDate = new \DateTime($date->format('Y-m-d').' '.$dailyEventTime);

// becomes
$hour = $dailyEvent->hour;
$minute = $dailyEvent->minute;
$second = $dailyEvent->second;
...
$newStartDate = $date->setTime($hour, $minute, $second);

Carbon can also be used for intervals, so all of your recurrence loops will be simplified as well.

Is it possible to run this solution with Doctrine ORM ?