Privatize __clone()?

[dashifen]

I usually privatize the __clone() magic method in singletons I write. Technically, if I understand cloning correctly, PHP will still make the shallow copy of the cloned object in memory, but then when it calls the __clone() method, since it's private, it creates a fatal error.

It's probably overkill, but adding the following to the classes (usually as the last thing in the class to keep it out of the way) isn't so bad:

private function __clone() { }

Thoughts?

[tommcfarlin]

I'm going to go ahead and kick this down one milestone. I like the idea, but I think some further discussion needs to happen mainly around which versions of PHP support this.

I try to keep the Boilerplate consistent with the minimum version of PHP as supported by WordPress.

Do you know, off the top of your head, if this is consist with that?

[dashifen]

The __clone() magic method came in with PHP 5.0 just like __toString(), __sleep(), _wakeup(), etc. Thus, WP's requirement for 5.2.4 puts cloning in play.

More information here: http://www.php.net/manual/en/language.oop5.cloning.php

[tommcfarlin]

Noted. This is good stuff. Deferring it to 2.7.0 because I'd like others to chime in on their thoughts, and because 2.6.0 is almost done :).

[franz-josef-kaiser]

What is the benefit of preventing cloning? Or: Why not return a real clone instead?

[dashifen]

First: this will cause some failures in WordPress because it uses cloning during some operations (i.e. saving options). As such, we can't privatize it as one usually would do to enforce a Singleton pattern in PHP.

As for the general benefit of preventing cloning, it's because PHP cloning is a shallow copy. Thus, object references in the original are also in the clone and that can create unexpected results; in other words one would expect messing with properties of the clone would leave the original alone, but that's not the case if the property of the clone is an object reference.

Instead, what we'll probably need to do here is to create a __clone() that copies non-object properties but maybe re-instantiates objects so that the clone's objects are explicitly different from the originals. It'd be nice to do a deep copy but, off the top of my head, I worry that it's not possible without also trying to enforce an interface for all objects used within the boilerplate that requires the definition of a method to create the deep copy and that's likely a step too far for what is intended for a beginner/intermediate audience.

[tommcfarlin]

At this point, I'm questioning why we would need to introduce cloning into the Boilerplate?

This doesn't seem like something that would help people beginning to start off with their plugins. It seems something more highly tailored to the intermediate-to-advanced audience.

I'm obviously open to discussing further, but I'm not seeing enough advantages or benefits for this to be included.

Thoughts?

[dashifen]

It probably doesn't need it at all, except that the generally accepted Singleton pattern for PHP would specify a private side clone to avoid multiple instances of that Singleton. Since it causes problems with respect to WP, I say we leave it out and hope that anyone who wants to use cloning knows the pitfalls.

[franz-josef-kaiser]

It seems something more highly tailored to the intermediate-to-advanced audience. (...) but I'm not seeing enough advantages or benefits for this to be included.

Me neither.

Anyway, if the boilerplate should implement something like that, then I'd write something along the lines of the following:

private $instanceCount = 0;

private $instances = array();

public $instance = NULL;

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

public function __clone()
{
    self::$instanceCount++;
    ! isset( self::$instances[ self::$instanceCount ] ) AND array_push( self::$instances, new self );

    return self::$instances[ self::$instanceCount ];
}

But that's just a quick draft from the back of my head.

[franz-josef-kaiser]

Ok, seems I'm already too tired. Had to edit a approximately five times :P

[bradvin]

I think including this clone method will just raise more questions from newbie plugin developers than anything else. I have never run into an issue with PHP singleton classes in my experience and this is the first time I have ever seen a private clone mentioned. (but then again I never had singleton issues)

@dashifen Don't get me wrong, I see the reason why you want to add this, and you explained the benefits, but this just seems extremely advanced for a boilerplate.

Just my 2c :)

[dashifen]

I 100% agree. And, it was my own idea! On Nov 14, 2013 3:16 PM, "bradvin" notifications@github.com wrote:

I think including this clone method will just raise more questions from newbie plugin developers than anything else. I have never run into an issue with PHP singleton classes in my experience and this is the first time I have ever seen a private clone mentioned. (but then again I never had singleton issues)

@dashifen https://github.com/dashifen Don't get me wrong, I see the reason why you want to add this, and you explained the benefits, but this just seems extremely advanced for a boilerplate.

Just my 2c :)

— Reply to this email directly or view it on GitHubhttps://github.com/tommcfarlin/WordPress-Plugin-Boilerplate/issues/107#issuecomment-28518697 .

[franz-josef-kaiser]

After thinking about that again (I don't use Singletons aside from drafting anymore), here're my two cents:

1. The project should receive @dashifen pull request
2. It shouldn't only be private, but final as well.

The reasons:

Singletons are a bad habit and an anti design pattern. In fact, they're nothing than objects misused as poor mans namespaces and all the self::$instance = new self "magic" is just another way of calling a global. So if we're already stuffing things in containers and telling people to do exactly the same, then we should at least share some real knowledge and help them avoiding really funky debug trips down the rabbit hole.

Even if I've tried to show above what could be done - adding a stack to avoid sharing instances - to avoid side effects, it still doesn't get to the root of the problem. Point is, that as long as we don't prevent access to __construct() and __destruct(), deny setting vars by locking down __set(), __unset(), and __invoke() as well, we won't help any one a single step further. It's just too easy to call objects like you do with functions, override stuff or extend things. A developer will never be safe from "developers" who learned (but didn't understand) a new thing, want to instantly use it and, for example treat your singleton like an abstract and extend hell out of it.

Keep in mind: Even if the singleton is used in WordPress as a poor mans namespace - and I can't repeat that often enough - it's not the intended use case. We should only use it if we really want to make something global that receives stuff from everywhere and has only one instance. I guess I can stop that here as there're enough well written articles about this out there.

To make it short: If the whole project isn't just on the fly switching routes and heading over to a more current design pattern like dependency injection (which I don't expect), then the next logic step just is to lock the singleton down. This lock down should be stated on all the phpDocBlocks and all methods should be final private.

[tommcfarlin]

I'm really with @bradvin on this one. Especially because of this:

I think including this clone method will just raise more questions from newbie plugin developers than anything else. I have never run into an issue with PHP singleton classes in my experience and this is the first time I have ever seen a private clone mentioned. (but then again I never had singleton issues)

This isn't to discredit @dashifen or @franz-josef-kaiser's commentary because I think it's both valuable and right; however, the point of the boilerplate is to help start beginner-to-intermediate developers and I don't think that introducing this particular method sticks to that goal.

It's more of an intermediate-to-advanced function that I think more experience developers will implement if they need it and if it's not already there.

Keep in mind: Even if the singleton is used in WordPress as a poor mans namespace - and I can't repeat that often enough - it's not the intended use case. We should only use it if we really want to make something global that receives stuff from everywhere and has only one instance. I guess I can stop that here as there're enough well written articles about this out there.

It's used as a poor man's namespace for now - until the minimum version of PHP is brought up to par, but I also think that it's used to share attributes across those accessing a single instance of the plugin.

This isn't to say you're wrong - because I don't think you are - but there are viable use cases for it, IMHO.

To make it short: If the whole project isn't just on the fly switching routes and heading over to a more current design pattern like dependency injection (which I don't expect).

It's not yet. This is something I'm still trying to decide for myself.

I tend to favor more advanced OOP techniques (though I don't think dependency injection is that advanced), but I still want to make sure we're striking a balance with this code and the beginner's in mind. That's my target audience.

So with that, I'm moving this ticket to the Future State milestone - I don't want to abandon it, but I think it's too soon, and I also want to ship 2.6.1 next week :).

[tommcfarlin]

Related #112