unclecheese/silverstripe-event-calendar — pull request #18 by tractorcow (2013-06-19 04:03:00+00:00)
Title: BUG Fixes and API Updates (and some cleanup)

Hi Unclecheese,

I've done a bit of upgrading work on a SS 2.4 site to SS 3.1, and in the process have been going over this module testing and bugfixing (as well as cleaning up some issues I noticed).

- In my previous pull request I issued an update for SS 3.1 compatibility - Well, I made a few errors I would like to fix in this pull request. `$jquery_included` should not have been a config value. Also, these values should be accessed via the Config API, not via static accessors, so I've fixed these were appropriate.
- I've also fixed some references to non-existing database fields. `Calendar` had a `$has_many` to `CalendarEvent`, although this relation already exists (i.e. via the `SiteTree::Children` method), and there was no corresponding `$has_one` on `CalendarEvent`. There were also some references to various fields through the code, `$defaults`, and other places where fields from 2.4 were removed, but not cleaned up.
- I've rewritten `Calendar::getStandardEvent` to simply use the `AllChildren` method (since it supports subclasses of `CalendarEvent` for selecting children). Why was this added but commented out? It's IMHO the better implementation to use here.
- Various fixes and cleanup of cms fields, etc.
- I also had a few issues with SQL queries not being properly generated. It turned out it was my own fault, but in the process of trying to resolve this I've cleaned up a lot of the query code. `DataList->filter` has been used here to promote better SQL generation (as it properly defines Table.Field predicates for each condition automatically). Table names in `->where` conditions have been added, and both Table and Field names have been properly ANSI quoted.

Please review and let me know what you think of this. The code did raise a few questions I don't really understand.

Why is getEventClass (and others) used so frequently? I was scared to touch these because I wasn't sure about the use-case they covered. In my own project I extend both Calendar and CalendarEvent and it works fine without having to adjust these values. I would propose simply removing these for simplicity sake. other classes like CalendarAnnouncement can be extended through DataExtension easily enough.

I also made CalendarWidget use the injector pattern (`Object::create`) because I wanted to use my own class here. :)