Vote Details for "Mailman" by till

» Details
» Comment
What Christian and Ken said.

In addition:

1. Can you have an empty password on a mailman installation? If not, an exception should be thrown -- IMHO the error needs to be explicit. In most of these cases is_string is nice, but kinda meaningless. I'd check for empty() if something is required.

I'd also leave out a default (in __construct()) if something is required.

Same for admin url and listname. Asking the object for an error shouldn't be necessary since these are the most basic requirements for the class to work.

If you disagree, please let me know why. Maybe I'm wrong, or don't know the setup well enough.

2. I'd also like to emphasize the unittest thing.

The following are minors (aka nice to see in the next release, etc.):

3. Speaking of 'hasErrors()' 1 is not true and 0 is not false.

4. @version tags are mixed up.

5. I like set-methods with a fluent interface. E.g. "return $this;" -- makes the calls chainable. If there's an error, just throw an exception. Makes the API easier and cleaner (IMHO).

6. Your checks in setReq() are redundant -- if it wasn't an instance of HTTP_Request2, it would never get so far since the type hint would throw it off.

7. List could be its own object Service_Mailman_List with methods to subscribe/unsubscribe, list members, etc.. But that's just a suggestion not mandatory to make my vote +1.


Thanks again for proposing this code and thanks for incorporating my previous suggestions! :-)