On Wed, May 12, 2010 at 1:40 PM, Les Hazlewood <[email protected]> wrote: > 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 :)
No confusion and you know I fully support removing the plain convenience methods. > 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. Agree.. didn't want to imply that Shiro should internally use it. > 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. Ok, I buy that. > 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. BeanUtils might fail, but otherwise that's weak reasoning. Obviously you could get the syntax to work even with add..(). To me, it's more about which side of the fence is the collection created at - and if you do things dynamically, certainly add is more favorable. If we really wanted to support Guice etc., we'd favor constructor injection - after all setter injection is the weakest form of injection. Not a big issue for me though, as long as there are strong reasons to do it this way that's fine. I can always add a thin layer on top of this to hide the details of creating a collection. Kalle > On Wed, May 12, 2010 at 11:17 AM, Kalle Korhonen > <[email protected]> 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 >> >
