Helper classes should not have static methods

[dantleech]

NodeHelper class is a utility class which implements a set of static methods. This is a problem because the Session object is passed to some of the methods, which makes really widens the scope of unit tests.

e.g.

NodeHelper::createPath($session, '/foo/bar');

Ultimately we should be able to use the NodeHelper::createPath method from an injected class, e.g.

$this->nodeHelper->createPath('/foobar');

This would make unit testing classes which need to create paths much cleaner. https://github.com/symfony-cmf/RoutingAutoBundle/pull/73/files#r10602618

[dbu]

to avoid a BC break, i would love to keep the static methods along with
instance methods where we could pass a session to the instance and then
the helper to the classes in question.

any idea for the naming?

[dbu]

removed the milestone

[dantleech]

Running into this issue again - one solution here would be simply to make the __construct public again - as static methods can be called from an instance, so I could use the NodeHelper as follows:

class MyClass
{
    private $session;
    private $helper;

    public function __construct(SessionInterface $session, NodeHelper $helper)
    {
        $this->helper = $helper;
    }

    public function doSomething()
    {
        $this->helper->createPath($this->session, '/path/to');
    }
}

And so be able to unit test my class without implicitly unit testing the NodeHelper..

[dbu]

if we want to change this, making the constructor public would be a first step, and deprecating to call the methods statically. we then would need to refactor jackalope to pass around the helper.

does calling static methods on an instance not raise a warning or something?

[dantleech]

Well, if we were to deprecate then we would also need to think about removing the first argument and making it a constructor argument. It would be a massive pain as I think everybody who uses PHPCR probably uses this method, so I would just quietly remove the __construct restriction.

does calling static methods on an instance not raise a warning or something?

... ??