weierophinney/helper.php

The Helper class (or at least its name) violates the Single Responsibility principle, this class will be used to tack on additional methods that may serve no purpose in your intended example.

The MongoPasteService violates the Dependency Inversion principles by having a hidden dependency to the Helper class; also the class would be more extendible if a class instance were to be passed so that multiple hash types are possible or to extend the MongoPasteService if needed (Liskov Principle).

Here is my version:

<?php

interface PasteHashInterface
{
    public function create(Paste $paste, PasteService $service); 
}

class PasteSha1Hash implements PasteHashInterface
{
    public function create(Paste $paste, PasteService $service)
    {
        $seed = $paste->language . $paste->timestamp . $paste->content;
        do {
            $seed . = uniqid();
            $hash   = hash('sha1', $seed);
        } while ($service->exists($hash));
        return $hash;
    }
}

class MongoPasteService implements PasteService
{
    protected $collection;
    protected $hash_builder;

    public function __construct(MongoCollection $collection, PasteHashInterface $hash_builder)
    {
        $this->collection = $collection;
        $this->hash_builder = $hash_builder;
    }

    public function create(Paste $paste)
    {
        $hash = $this->hash_builder->create($paste, $this);
        $paste->hash = $hash;
        $this->collection->save((array) $paste);
        return $paste;
    }

    public function exists($hash)
    {
        /* .. check if paste exists .. */
    }
}

This example is actually one I rewrote to work with PHP 5.3. Using 5.4, I'd do it as a trait:

trait HashCreation
{
    public function createHash(Paste $paste)
    {
        $seed = $paste->language . $paste->timestamp . $paste->content;
        do {
            $seed . = uniqid();
            $hash   = hash('sha1', $seed);
        } while ($this->exists($hash));
        return $hash;
    }

    abstract function exists($hash);
}

class MongoPastService implements PasteService
{
    use HashCreation;

    /* ... */

    public function create(Paste $paste)
    {
        $hash = $this->createHash($paste);
        /* ... */
    }
}

The above creates no coupling, and does not violate the LSP. I could have also avoided LSP by using extension instead:

abstract class AbstractPasteService implements PasteService
{
    protected function createHash(Paste $paste)
    {
        $seed = $paste->language . $paste->timestamp . $paste->content;
        do {
            $seed . = uniqid();
            $hash   = hash('sha1', $seed);
        } while ($this->exists($hash));
        return $hash;
    }
}

class MongoPasteService extends AbstractPasteService
{
    /* ... */

    public function create(Paste $paste)
    {
        $hash = $this->createHash($paste);
        /* ... */
    }
}

Extension is messy, however, as it requires all classes wanting to implement or consume the shared functionality to extend, which can lead to a spiraling abstraction. Traits are elegant, but are only reasonable if you can control the PHP environment on which you install; for shared libraries, they're not a feasible solution yet, as 5.4 is nowhere close to critical mass in terms of installation.

While I completely see the academic purity of the solution you provide, I also charge that it violates the YAGNI principle -- is that level of abstraction really needed? It forces an additional level of composition, which requires the end-user to go to extra work simply to consume the service. In my case, I'd made a decision that there was a single appropriate hashing style: an 8 character hexadecimal string. Abstracting it into its own interface and implementations made no sense, as I wasn't going to need that level of abstraction.

This left me extension and static helper methods as the only recourse for 5.3 users.

I would argue that using a static method that does not alter or store state is no different than using a function call -- something done all the time in PHP for obvious reasons, and which does not create arguments of SOLID violations. If I'd simply called uniqid() in the create() method, you would not have batted an eye, and you'd not have suggested a violation of LSP. It was because it was a static class method that the suggestion arose.

My point is this: yes, we should adhere to SOLID as much as possible. But we should also consider carefully as to when an additional layer of abstraction is truly necessary, and when that abstraction may get in the way of consuming the code we write.

I agree, this is an accademic issue, and in some cases the additional layer of abstraction is not needed. Being aware of the shortcoming is one thing, but knowing whether it matters is more important

I consider your point regarding YAGNI to be valid and as such I'd be tempted to write the following more pragmatic approach:

<?php

class PasteHash
{
    public function create(Paste $paste, PasteService $service)
    {
        $seed = $paste->language . $paste->timestamp . $paste->content;
        do {
            $seed . = uniqid();
            $hash   = hash('sha1', $seed);
        } while ($service->exists($hash));
        return $hash;
    }
}

class MongoPasteService implements PasteService
{
    protected $collection;
    protected $hash_builder;

    public function __construct(MongoCollection $collection, PasteHash $hash_builder = null)
    {
        $this->collection = $collection;

        $this->hash_builder = $hash_builder ?: new PasteHash();
    }

    public function create(Paste $paste)
    {
        $hash = $this->hash_builder->create($paste, $this);
        $paste->hash = $hash;
        $this->collection->save((array) $paste);
        return $paste;
    }

    public function exists($hash)
    {
        /* .. check if paste exists .. */
    }
}

The above code does not introduce an additional abstraction layer but still offers me all the benefit of being able to mock the hash generation. The big assumption in the code above is that PasteHash is re-used; otherwise I'd just use a method and test that using the test cases for this class.

By using statics you require every unit tests that involves this hash creation to test the hashing code, to not test the hashing code at all or pick a leading implementation where you test the hashing code (which is unsafe). In my opinion the static helper hurts testability and provokes abuse of the helper class.

I get your point, Mike -- but I have another question: would you make the same comment if a function were used versus a static method? In both cases, the implementation would be tied to a specific set of code -- and yet I've not heard anybody talk about the LSP with regards to functions. (I'm truly curious, as I don't have an answer here.)

I would have made the same, or at least a very similar, comment.

As I see it, functions introduce the same issues as static methods; they induce a hard and untestable coupling between two structural elements.
For example: if I were to write a unit test for the create method in your example I would be required to test whether the has is correctly generated and thus write testing code that checks the createHash() static method. The same thing would happen if createHash() would be a function and thus the same comments apply.

Please note that this comment even applies to Traits; these are untestable, deeply coupled bits of code.

With regards to LSP and functions; in my opinion LSP is only applicable to objects as you cannot substitute methods or functions (monkeypatching not included in this statement); just objects. To even be a bit bolder: LSP applies to object oriented programming, (userland) functions are to be considered procedural programming. As such these two are hard to unite.