mixpanel-php
mixpanel-php copied to clipboard
mixpanel
Update to PHP 5.3 and added factory class
Pull request to update the minimum php version to 5.3, with some small scale changes to the api which would break BC. Proposals outlined below are implemented in the attached pull request. Tests are passing: https://travis-ci.org/applestump/mixpanel-php
I know that there's a desire to hold the minimum php version at 5.0; but I do think this has resulted in some design patterns which make testing difficult and which could be rectified by adopting PHP 5.3. I wouldn't like to guess at the userbase for the library, but given that PHP 5.2 hasn't been supported since 2011, is it too conservative to try to retain BC with anything less than 5.3? (We could always start a 3.x branch which was 5.3+, too, and leave the current library unsupported?)
I originally forked the repo because it's currently not possible to easily mock the $mp->people and $mp->events properties without using reflection. I added a setPeople(), getPeople(), setEvents() and getEvents() methods to the Mixpanel class, and allowed injection of the people and events properties through the constructor.
Proposal: allow constructor injection of dependencies.
The current Mixpanel class requires a new test class to be written every time I want to alter the options passed to it because Mixpanel::getInstance() returns a singleton. Therefore the tearDown() method in the MixpanelTest class doesn't actually destroy the instance, and you get the same instance returned on a subsequent call to Mixpanel::getInstance. This means I can't test passing different arguments to the constructor without writing different test classes for each one. I can of course create a destroy() function in the Mixpanel class and call it explicitly when needed but this then violates the SRP and feels pretty hacky. So for example, this test in MixpanelTest will fail if a test has been run before it, but will pass if it's the first test to run:
//in MixpanelTest.php //This is theory, to show how it's difficult to effect constructor injection and testing with current singleton public function testCallingConstructorWithIncorrectPeopleArgumentThrowsException() { $this->setExpectedException('InvalidArgumentException'); Mixpanel::__destroy(); Mixpanel::getInstance("abc", array(), "string"); }
Proposal: Instantiate the Mixpanel client using the Factory pattern. This will allow the factory to decide when to return the same instance (and hence allow overriding within Test classes).
The Test cases themselves didn't work for me unless I bootstrapped them with a call to spl_autoload_register, since it did not find the classes needed. In addition, the current library doesn't follow PSR-0 (for example, Producers_MixpanelEvents class is within the MixpanelEvents.php class ), which meant I initially ended up writing a pretty hacked-together autoloader.
Proposal: Namespacing of classes and restructuring of the library to match the path to the classname
We'd be able to test the library more easily, we'd separate out concerns (so for example, PSR-compatible http libs and loggers can be injected through the factory) , with only minimal changes to the api. In fact, to the end user, the only difference would be that we'd lose Mixpanel::getInstance() and instead use $mpFactory = new MixpanelFactory();
What are your thoughts?
@applestump
First, thanks a ton for this very well thought out and descriptive pull request. We really really appreciate it.
I agree that life would be easier and the code would be better if we only supported >= 5.3. What you're proposing seems very reasonable. I need a bit of time to think about the side effects of this change at a high level and consider whether we'd want a new branch for it or drop support for < 5.3 altogether. Let me put a bit of thought into this before we make a decision either way and we'll start taking steps to get the code base into a more testable and flexible state.
Just wanted to let you know this is still on my radar -- you are not forgotten!
Hi @jbwyme - haven't forgotten about this, just haven't had a chance to look at it yet! Will get back to you soon!