Captcha class harder to extend than necessary

[zyspec]

Extending the XoopsCaptcha class requires the getInstance() method be overloaded since the method is static and uses CLASS instead of using get_called_class(). If a new class extends XoopsCaptcha and then attempts to get a new instance, an instance of XoopsCaptcha class is created instead of creating a new instance of the calling class.

    /**
     * Get Instance
     *
     * @return XoopsCaptcha
     */
    static function getInstance()
    {
        static $instance;
        if (!isset($instance)) {
            $class = __CLASS__;
            $instance = new $class();
        }
        return $instance;
    }

need to replace $class = CLASS; with $class = get_called_class();

The same issue exists in Xoops 2.5.x series. Obviously this can be circumvented in the new calling class by overloading the getInstance() method, but it shouldn't have to...

[geekwright]

Captcha is due for a major overhaul -> #259. That will remove this particular issue completely.

There are a lot of static issues like this though 😞

The best solution is to get rid of the static singleton stuff.

If there is a compleling reason to keep the singleton, the best approach would be to use PHP's late static binding notation, rather than trying to simulate it:

if (!isset($instance)) {
    $instance = new static();
}

[txmodxoops]

But that does not force me to change the modules for XOOPS 2.5.8 forcibly at this time right?

There is always time?

[geekwright]

But that does not force me to change the modules for XOOPS 2.5.8 forcibly at this time right?

Right. Nothing about captcha is changing in the 2.5 series.

But in 2.6, captcha handling will change to use our service model. Instead of talking to the captcha classes, modules will talk to the captcha service.

[zyspec]

Understand... I posted an issue since it still exists in the 2.6 code. I wouldn't recommend changing this in the 2.5.x codebase. Any modules that are currently released presumably work so there's no reason to mess with those and any new modules can be made to work - so it's not a required change.

The intent of this issue was to hopefully highlight it for change "in the future" (2.6 codebase). Since there's still more refactoring to do in the captcha classes it can be taken care of then.

Richard, to answer your question - I don't know of a compelling reason for this to be kept as a singleton.

[geekwright]

I'm happy to see the issue. Your insights are always appreciated!

Issues are like questions - the only bad one is the one that wasn't presented.

[mambax7]

But for individual modules that use currently singleton internally, what would be the best replacement, so we could standardize around it for all modules?

[txmodxoops]

I found this:

class Singleton
{
    private static $instance = null;

    private function __construct()
    {
         //...instance initialization...
    }

    private function __clone()
    {
        // avoids the object cloning
    }

    public static function getInstance()
    {
        if (static::$instance === null) {
            static::$instance = new Singleton();
        }
        return static::$instance;
    }
}

[geekwright]

Sorry. Didn't mean to stir things up so much. Let me back up just a bit.

This was more strategic planning comment than a call to action right now.

Here is a overview of the singleton pattern. It tagets java, but the issues are common to all OOP languages including PHP:
https://dzone.com/articles/singleton-patterns-20-years-later

When you have to have a singleton, a good PHP implementation of the pattern is decribed here:
http://www.phptherightway.com/pages/Design-Patterns.html

Singletons are easy, and they are tempting, especially when the system is filled with them, I've used them recently. But that ease comes with a cost.

Singletons reduce the reuseability of code, both the singleton class and any consumers. They form tight explicit dependencies. We need to steer away from them, but the pattern is quite entrenched in XOOPS. If we don't rethink this, it will limit us.

When you realize you are talking about extending a singleton, you face the question "Why do I need many, when it is designed to be one?"

Once you start thinking about that, there are more questions:

• Do I really need only one?
• Why can't I share a single instance without using a singleton?

Ask these questions in future development and refactoring. This isn't a quick fix, it is a paradigm shift.