Skip to content
This repository has been archived by the owner on Dec 17, 2021. It is now read-only.

Subtitles::getInstance uses unnecessary array #14

Closed
rmccue opened this issue Jun 19, 2014 · 4 comments
Closed

Subtitles::getInstance uses unnecessary array #14

rmccue opened this issue Jun 19, 2014 · 4 comments
Labels

Comments

@rmccue
Copy link

rmccue commented Jun 19, 2014

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
Copy link
Member

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
Copy link
Author

rmccue commented Jun 19, 2014

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
Copy link
Member

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
Copy link
Member

Addressed in #8.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants