Closed manchuck closed 5 years ago
Hi, thanks for the clear description of your issue.
When final
is removed, the architecture code will suffer a big problem. Read about why final is so important here. Amazing post.
But to your problem:
To make mocking work right now use this: https://phpfashion.com/how-to-mock-final-classes
Instead of removing final, the better call is to decouple an interface - you can send PR for that, since it might be useful in more cases.
Instead of using mocks, use anonymous classes - https://www.tomasvotruba.cz/blog/2018/06/11/how-to-turn-mocks-from-nightmare-to-solid-kiss-tests/#solid-code-as-a-side-effect, they're clear, easy to refactor and everyone understand them - it's plain PHP
Why not pass the GitWrapper by constructor instead of set/get magic? It would make design more clear and you'd get rid of manual new GitWrapper
inside the code - you should never do that, there is DI.
final class UpdateMasterCommand extends Command
{
/**
* @var GitWrapper
*/
private $gitWrapper;
- public function getGitWrapper()
- {
- if (null === $this->gitWrapper) {
- $this->setGitWrapper(new GitWrapper());
- }
-
- return $this->gitWrapper;
- }
-
- public function setGitWrapper($gitWrapper)
- {
- $this->gitWrapper = $gitWrapper;
- }
+
+ public function __construct(GitWrapper $gitWrapper)
+ {
+ $this->gitWrapper = $gitWrapper;
+ }
}
Hey @TomasVotruba, the final keyword bites me when testing, too. The git wrapper lib is a low level library, when it comes to functional tests, one really wants to mock some things away, for example $workingCopy->push().
I'm using constructor DI for the GitWrapper class in my service. This does not help here since I can't inject a mocked wrapper (and workingCopty) as long as they have final keyword and no interface.
The project 'dg/bypass-finals' is only a hack for devs who are in desperate need to get rid of the final keyword from a library class without interfaces. Tokenizing the php stream and rewriting on the fly really is not a cool thing. Not even for tests. I don't consider that a solution.
I ask to do one of the following:
Hi, reverting final
is a no-go. See my comment above.
I'm open to solution 2.
Adding final without adding interfaces was breaking and violated semver.
With the rather long main classes, you would end up with longish interfaces that simply contain the exact same method signatures, forcing you to keep those forever. This does not make much sense either, I'll not create pull requests for that.
At the moment I rather tend to fork and revert the final patch or to switch away and use a shell script.
Adding final without adding interfaces was breaking and violated semver.
That's not true. It was released as 2.0: https://github.com/cpliakas/git-wrapper/pull/137
At the moment I rather tend to fork and revert the final patch or to switch away and use a shell script.
That's sad that you prefer that over adding a PR with interface. But if that's what you prefer, give it a go.
You really prefer interfaces for things like GitWrapper that contain 50 method signatures?
I'd use the bypass final, since it's another package that forces bad design.
Imagine that framework you use would require static methods only. Would you send PRs to all packages that you use to use such design?
Closing as options are answered and it's 3rd party responsibility
Having all the classes marked as
finial
makes it difficult for creating test mocks in applications.Example:
UpdateMasterCommand.php:
UpdateMasterCommandTest.php:
When running PHPunit, the following exception is thrown:
This is because the type of
UpdateMasterCommandTest::$gitWorkingCopy
is a proxied mock and cannot descendGitWrapper\GitWorkingCopy
.The second side effect is that I cannot set
UpdateMasterCommand::setGitWrapper
to accept only GitWrapper instances due to the fact thatUpdateMasterCommandTest::$gitWrapper
is also a proxied object