wecobble / Subtitles

Add subtitles into your WordPress posts, pages, custom post types, and themes. No coding required. Simply activate Subtitles and you're ready to go.
http://wordpress.org/plugins/subtitles/
GNU General Public License v2.0
117 stars 186 forks source link

Subtitles::getInstance uses unnecessary array #14

Closed rmccue closed 10 years ago

rmccue commented 10 years ago

getinstance currently uses self::$instances[ get_called_class() ] = new static. However, since the plugin assumes PHP 5.3+ (with use of both get_called_class() and static), this could be set as static:: instead.

public static function getinstance() {
    $called_class = get_called_class();

    if ( empty( self::$instance ) ) { 
        static::$instance = new static;
    }

    return static::$instance;
}

(Also, contrary to the comment 'self' in this context refers to the current class in use, it actually refers to the class where the method is defined.)

philiparthurmoore commented 10 years ago

I'm fixing the PHP 5.3+ issue today. I'll leave this open as part of that.

I like what you've done above. Cheers.

rmccue commented 10 years ago

I'm fixing the PHP 5.3+ issue today. I'll leave this open as part of that.

For PHP 5.2, singletons with extended classes are actually much harder. You'll need to use debug_backtrace to find which class was actually called, or override the method in each subclass.

If you're going with 5.2 compatibility, I'd actually get rid of the forced singleton (with getinstance) and just store the instance in a global variable. It's much nicer to extend.

philiparthurmoore commented 10 years ago

For PHP 5.2, singletons with extended classes are actually much harder.

I'm frustrated that I didn't catch this before shipping v1.0.0.

If you're going with 5.2 compatibility, I'd actually get rid of the forced singleton (with getinstance) and just store the instance in a global variable. It's much nicer to extend.

I think I agree.

philiparthurmoore commented 10 years ago

Addressed in #8.