Hi Kalle, Sorry about the confusion - those methods and interfaces were marked for removal a *long* time ago in the Jira issue - I assumed that would have given people plenty of time for review. Since I didn't hear anything, I moved forward :)
Here are my thoughts: All of the *Registrar and *Aware interfaces added a lot of 'noise' to the codebase IMHO - I was only too happy to remove them. Things are certainly much cleaner and easier to maintain now that they're gone. My personal desire is to not add them back in unless we see a clear reason for doing so (or everyone comes to a relative consensus). Having the *Registrar interface encourages a lot of boilerplate instanceof checks in code - both in Shiro code and end-users' code - it is very ugly IMO. It just propagates that kind of stuff everywhere. But more importantly for me is: where does this stop? A SecurityManager has a SessionManager which has a SessionDAO which has a SessionIdGenerator. Do we now need to provide a SessionIdGeneratorRegistrar interface to traverse the object graph? This could quickly spin out of control. There are so many places in the API that would need support like this. I would want all configuration mechanisms to be consistent to satisfy all cases. That's why I think this stuff should not be in the API: the entire framework is JavaBeans compatible, so we pretty much imply that some JavaBeans-compatible configuration mechanism be used for all of the intricate configuration tasks. To add this Registrar for one property and not for any list of others feels really clunky to me. I'd prefer that end-users use the config mechanism of their choice and leave this stuff out of the core API. Thoughts? If there is anyone else who feels one way or another too - please speak up. We would benefit from community feedback! Les P.S. We definitely can't remove the getters and setters. It is required for all IoC type of configuration (e.g. INI): securityManager.sessionManager.sessionListeners = $listener1, $listener2, ..., $listenerN Without the getters/setters, BeanUtils would fail when trying to set that collection in a generic way (it shouldn't have to know anything about SessionListenerRegistrar or AuthenticationListenerRegistrars or anything else like that). And Spring and Guice wouldn't work either. On Wed, May 12, 2010 at 11:17 AM, Kalle Korhonen <kalle.o.korho...@gmail.com> wrote: > Hey Les, you might have gone a bit overboard with this :) > Specifically, I don't see a reason why AbstractSessionManager would > need to serve the naked SessionListener collection to anybody by > providing get/set methods for the collection itself. I think the > common idiom is you have add<Type>Listener and remove<Type>Listener > operations in the object that's holding the collection, hiding the > details of the collection implementation. Also, I don't think the > SessionListenerRegistrar was extra - it's always more flexible to > program against the interface if we ever need to relocate this > functionality. You mind if I: > 1) remove getter and setter for the collection > 2) add back the interface with add/remove operations > 3) Make AbstractSession implement the interface and adjust the > implementation accordingly > > Kalle >
