[xwiki-devs] [VOTE] Remove the xwiki-core captcha plugin and add $captchaservice VelocityContextInitializer - xwiki-devs@xwiki.org

Thomas Mortagne

21 Jan 21 Jan

10:32 p.m.

New subject: [xwiki-devs] [VOTE] Remove the xwiki-core captcha plugin and add $captchaservice VelocityContextInitializer

On Thu, Jan 21, 2010 at 22:23, Caleb James DeLisle <calebdelisle(a)lavabit.com> wrote:

...

I am adding a new captcha component, it uses jcaptcha-2.0-alpha1 and supports image captchas and sound captchas (freetts jars must be added for sound captcha support) There is a captcha plugin in xwiki-core which makes the core depend on jcaptcha-all-1.0-RC3. jcaptcha-all-1.0-RC3 causes conflicts when getting classes from jcaptcha-2.0. The plugin in querstion: http://svn.xwiki.org/svnroot/xwiki/platform/core/trunk/xwiki-core/src/main/… Here's my +1 for removing it entirely, breaking backward compatibility, lightening xwiki-core, and making the core no longer depend on jcaptcha. You may review a patch for removing the plugin here: http://jira.xwiki.org/jira/secure/attachment/16508/XWIKI-4779-removeJcaptch…

-0 for removing captcha plugin I don't like breaking something when it's not absolutely necessary, would it be too much work (maybe it's not even possible) to refactor xwiki-core captcha plugin to use jcaptcha-2.0 ?

...

To provide velocity access to the captcha component for checking the results of the captcha, I would like to add a VelocityContextInitializer which will add a binding called $captchaservice which will provide 3 methods: /** * If this is false, the captcha system will still work. * It is for velocity scripts to determine whether captchas are needed. * * @return Is the captcha service enabled in the configuration. */ boolean isEnabled() /** @return a List of the names of all registered classes implementing {@link org.xwiki.captcha.CaptchaVerifier}. */ public List<String> listCaptchaNames() /** * Get a {@link org.xwiki.captcha.CaptchaVerifier} from the service. * * @param captchaName The name of the type of captcha, use {@link #listCaptchaNames()} for a list * @return A captcha object which can be used to check a specific answer for a given challange * @throws Exception If the named captcha doesn't exist. */ public CaptchaVerifier getCaptchaVerifier(String captchaName) throws Exception

Don't you have something more precise than Exception ? It does not seems right to me, it should be a specialized exception (org.xwiki.captcha.InvalidNameException or something). Except for unit tests a methods that throws Exception generally mean there is something wrong in the design.

...

I realize throwing an Exception to velocity always results in a stack trace but I see it as the lesser evil to returning a non-functional CaptchaVerifier which always returns false. Also I see no use case where the name of the captcha is not given in hardcoded velocity so this Exception is only for the velocity developers. Here's my +1 for adding this as well. Caleb James DeLisle _______________________________________________ devs mailing list devs(a)xwiki.org http://lists.xwiki.org/mailman/listinfo/devs

-- Thomas Mortagne

Reply

Vincent Massol

11:25 p.m.

New subject: [xwiki-devs] [VOTE] Remove the xwiki-core captcha plugin and add $captchaservice VelocityContextInitializer

On Jan 21, 2010, at 10:32 PM, Thomas Mortagne wrote:

...

On Thu, Jan 21, 2010 at 22:23, Caleb James DeLisle <calebdelisle(a)lavabit.com> wrote:

I am adding a new captcha component, it uses jcaptcha-2.0-alpha1 and supports image captchas and sound captchas (freetts jars must be added for sound captcha support) There is a captcha plugin in xwiki-core which makes the core depend on jcaptcha-all-1.0-RC3. jcaptcha-all-1.0-RC3 causes conflicts when getting classes from jcaptcha-2.0. The plugin in querstion: http://svn.xwiki.org/svnroot/xwiki/platform/core/trunk/xwiki-core/src/main/… Here's my +1 for removing it entirely, breaking backward compatibility, lightening xwiki-core, and making the core no longer depend on jcaptcha. You may review a patch for removing the plugin here: http://jira.xwiki.org/jira/secure/attachment/16508/XWIKI-4779-removeJcaptch…

-0 for removing captcha plugin I don't like breaking something when it's not absolutely necessary, would it be too much work (maybe it's not even possible) to refactor xwiki-core captcha plugin to use jcaptcha-2.0 ?

I think there's some misunderstanding thomas. Caleb has developed a component-based captcha module. -1 to not remove the old captcha plugin ;) There's no point of having 2 systems that do the same thing. At worst the old plugin could be moved to retired projects in contrib but I don't even think we should do that since nobody is using the old catpcha plugin AFAIK. Plugins are meant to be removed. They're the old system that has been replaced by components. We should definitely not spend any time trying to create new plugins. In addition it would be a pain and completely unnecessary to maintain 2 modules that do the same thing but differently. Thanks -Vincent

...

To provide velocity access to the captcha component for checking the results of the captcha, I would like to add a VelocityContextInitializer which will add a binding called $captchaservice which will provide 3 methods: /** * If this is false, the captcha system will still work. * It is for velocity scripts to determine whether captchas are needed. * * @return Is the captcha service enabled in the configuration. */ boolean isEnabled() /** @return a List of the names of all registered classes implementing {@link org.xwiki.captcha.CaptchaVerifier}. */ public List<String> listCaptchaNames() /** * Get a {@link org.xwiki.captcha.CaptchaVerifier} from the service. * * @param captchaName The name of the type of captcha, use {@link #listCaptchaNames()} for a list * @return A captcha object which can be used to check a specific answer for a given challange * @throws Exception If the named captcha doesn't exist. */ public CaptchaVerifier getCaptchaVerifier(String captchaName) throws Exception

Don't you have something more precise than Exception ? It does not seems right to me, it should be a specialized exception (org.xwiki.captcha.InvalidNameException or something). Except for unit tests a methods that throws Exception generally mean there is something wrong in the design. > > > I realize throwing an Exception to velocity always results in a stack trace but I see it as the lesser evil to > returning a non-functional CaptchaVerifier which always returns false. Also I see no use case where the name of > the captcha is not given in hardcoded velocity so this Exception is only for the velocity developers. > Here's my +1 for adding this as well. > > > Caleb James DeLisle

Reply

Caleb James DeLisle

24 Jan 24 Jan

3:55 p.m.

New subject: [xwiki-devs] [VOTE] [RESULTS] Remove the xwiki-core captcha plugin and add $captchaservice VelocityContextInitializer

results: 3 +1 1 -0 0 -1 Changes committed. see: http://jira.xwiki.org/jira/browse/XWIKI-4779 and: http://jira.xwiki.org/jira/browse/XWIKI-4741 I decided to go with the name $captchaservice because it is closest to the names of other velocity bindings which are defined with VelocityContextInitializer. I don't like the idea of binding to an arbitrary velocity variable but I don't think it improves the situation to use a confusing naming convention which I don't see implemented anywhere else. see: $l10n http://svn.xwiki.org/svnroot/xwiki/platform/core/trunk/xwiki-localization/s… $officeimporter http://svn.xwiki.org/svnroot/xwiki/platform/core/trunk/xwiki-officeimporter… $ooconfig http://svn.xwiki.org/svnroot/xwiki/platform/core/trunk/xwiki-officeimporter… Caleb Thomas Mortagne wrote:

...

On Thu, Jan 21, 2010 at 23:25, Vincent Massol <vincent(a)massol.net> wrote:

On Jan 21, 2010, at 10:32 PM, Thomas Mortagne wrote:

On Thu, Jan 21, 2010 at 22:23, Caleb James DeLisle <calebdelisle(a)lavabit.com> wrote:

I am adding a new captcha component, it uses jcaptcha-2.0-alpha1 and supports image captchas and sound captchas (freetts jars must be added for sound captcha support) There is a captcha plugin in xwiki-core which makes the core depend on jcaptcha-all-1.0-RC3. jcaptcha-all-1.0-RC3 causes conflicts when getting classes from jcaptcha-2.0. The plugin in querstion: http://svn.xwiki.org/svnroot/xwiki/platform/core/trunk/xwiki-core/src/main/… Here's my +1 for removing it entirely, breaking backward compatibility, lightening xwiki-core, and making the core no longer depend on jcaptcha. You may review a patch for removing the plugin here: http://jira.xwiki.org/jira/secure/attachment/16508/XWIKI-4779-removeJcaptch…

-0 for removing captcha plugin I don't like breaking something when it's not absolutely necessary, would it be too much work (maybe it's not even possible) to refactor xwiki-core captcha plugin to use jcaptcha-2.0 ?

I think there's some misunderstanding thomas. Caleb has developed a component-based captcha module.

No there is not. This vote is about removing xwiki-core public API. The fact that it's a plugin has nothing to do with that.

-1 to not remove the old captcha plugin ;) There's no point of having 2 systems that do the same thing. At worst the old plugin could be moved to retired projects in contrib but I don't even think we should do that since nobody is using the old catpcha plugin AFAIK. Plugins are meant to be removed. They're the old system that has been replaced by components. We should definitely not spend any time trying to create new plugins.

It's not so simple, plugin or not that's xwiki-core module public API not some external plugin and i don't like removing public API if not necessary.. We don't use it by default in XE does not mean nobody is using it, if you look at the mailing list it seems at least some people are using it.

In addition it would be a pain and completely unnecessary to maintain 2 modules that do the same thing but differently.

I just talked about API here, we could maybe use the new component behind old plugin APIs. Also my vote is not blocker, i just asked a question: the proposal is to remove a public API instead of deprecate it without explaining why compared to the general rule.

Thanks -Vincent

To provide velocity access to the captcha component for checking the results of the captcha, I would like to add a VelocityContextInitializer which will add a binding called $captchaservice which will provide 3 methods: /** * If this is false, the captcha system will still work. * It is for velocity scripts to determine whether captchas are needed. * * @return Is the captcha service enabled in the configuration. */ boolean isEnabled() /** @return a List of the names of all registered classes implementing {@link org.xwiki.captcha.CaptchaVerifier}. */ public List<String> listCaptchaNames() /** * Get a {@link org.xwiki.captcha.CaptchaVerifier} from the service. * * @param captchaName The name of the type of captcha, use {@link #listCaptchaNames()} for a list * @return A captcha object which can be used to check a specific answer for a given challange * @throws Exception If the named captcha doesn't exist. */ public CaptchaVerifier getCaptchaVerifier(String captchaName) throws Exception

Don't you have something more precise than Exception ? It does not seems right to me, it should be a specialized exception (org.xwiki.captcha.InvalidNameException or something). Except for unit tests a methods that throws Exception generally mean there is something wrong in the design. > > I realize throwing an Exception to velocity always results in a stack trace but I see it as the lesser evil to > returning a non-functional CaptchaVerifier which always returns false. Also I see no use case where the name of > the captcha is not given in hardcoded velocity so this Exception is only for the velocity developers. > Here's my +1 for adding this as well. > > > Caleb James DeLisle

_______________________________________________ devs mailing list devs(a)xwiki.org http://lists.xwiki.org/mailman/listinfo/devs

Reply

Vincent Massol

8:54 a.m.

New subject: [xwiki-devs] [VOTE] Remove the xwiki-core captcha plugin and add $captchaservice VelocityContextInitializer

On Jan 21, 2010, at 10:23 PM, Caleb James DeLisle wrote:

...

I am adding a new captcha component, it uses jcaptcha-2.0-alpha1 and supports image captchas and sound captchas (freetts jars must be added for sound captcha support) There is a captcha plugin in xwiki-core which makes the core depend on jcaptcha-all-1.0-RC3. jcaptcha-all-1.0-RC3 causes conflicts when getting classes from jcaptcha-2.0. The plugin in querstion: http://svn.xwiki.org/svnroot/xwiki/platform/core/trunk/xwiki-core/src/main/… Here's my +1 for removing it entirely, breaking backward compatibility, lightening xwiki-core, and making the core no longer depend on jcaptcha. You may review a patch for removing the plugin here: http://jira.xwiki.org/jira/secure/attachment/16508/XWIKI-4779-removeJcaptch…

+1 I'll ask around to see if anyone is using actively the old captcha plugin and if so then we could retire it to contrib retired projects location (ie not make it part of the core and of the default distribution but still make it available to anyone who need it). Caleb: would it still work if it's repackaged as a jar that people could drop in their WEB-INF/lib directory?

...

To provide velocity access to the captcha component for checking the results of the captcha, I would like to add a VelocityContextInitializer which will add a binding called $captchaservice which will provide 3 methods:

+1 for now but note that we want a different way to do this in the future so this binding should be considered internal for the moment (ie not public). We could name it with some special chars to mention this (same for other bindings we've added such as the officeimporter one or the syntax factory one). Maybe something like $_captchaInternal (ie "_" + name + "Internal")

...

/** * If this is false, the captcha system will still work. * It is for velocity scripts to determine whether captchas are needed. * * @return Is the captcha service enabled in the configuration. */ boolean isEnabled() /** @return a List of the names of all registered classes implementing {@link org.xwiki.captcha.CaptchaVerifier}. */ public List<String> listCaptchaNames()

getAvailableCaptchaTypes() maybe to be similar to getAvailableSyntaxes()? So this will return the component role hints right? So for now "image" or "sound", right?

...

/** * Get a {@link org.xwiki.captcha.CaptchaVerifier} from the service. * * @param captchaName The name of the type of captcha, use {@link #listCaptchaNames()} for a list * @return A captcha object which can be used to check a specific answer for a given challange * @throws Exception If the named captcha doesn't exist. */ public CaptchaVerifier getCaptchaVerifier(String captchaName) throws Exception I realize throwing an Exception to velocity always results in a stack trace but I see it as the lesser evil to returning a non-functional CaptchaVerifier which always returns false. Also I see no use case where the name of the captcha is not given in hardcoded velocity so this Exception is only for the velocity developers. Here's my +1 for adding this as well.

+1 Thanks -Vincent

Reply