Add `trait` for 'standard' `Commentable` implementation

[JakeQZ]

The various classes that implement Commentable all seem to have exactly the same implementation.

This duplication could be avoided by using a trait for a 'standard' implementation.

[oliverklee]

When we implement the trait, we don't need to add it to the @covers annotations of the corresponding testcases as PHPUnit includes them automatically starting with version 7.3.

[JakeQZ]

When we implement the trait, we don't need to add it to the @covers annotations of the corresponding testcases as PHPUnit includes them automatically starting with version 7.3.

Presumably we would have a dedicated TestCase for the trait, and a concrete implementation for testing it alone. And for clarity the dedicated TestCase could have a @covers annotation.

[oliverklee]

Yes, that's the way to go. (We'll need to check whether we can @covers a trait directly, though. PHPUnit 12 has the CoversTrait attribute, which we can't use in PHPUnit 8.5 yet.)

I also found this article (which is a bit on the older side, though): https://doeken.org/blog/testing-traits-in-phpunit

[oliverklee]

Two more thoughts on this:

• There is no way to test that a class actually uses a trait. So while we might have dedicated tests for a trait, this does not replace having the same tests in place for the classes that implement a trait.
• I'm a big fan of adding traits only if there also is a corresponding interface (or to create the corresponding interface), and to add a comment in the trait that this is the default implementation of said interface, and vice versa.

[JakeQZ]

* I'm a big fan of adding comments to add traits only if there also is a corresponding interface (or to create the corresponding interface), and to add a comment in the trait that this is the default implementation of said interface, and vice versa.

Do you mean "... fan of adding traits only if there ..."?

I also found this article (which is a bit on the older side, though): https://doeken.org/blog/testing-traits-in-phpunit

The first technique has the drawbacks mentioned. The second technique uses an anonymous class, and would work, though the article says:

If only there was a way to reduce the anonymous class to a single line... a helper method perhaps?

setUp should take care of that issue, I think.

The final three techniques all use PHPUnit methods which have been deprecated since v10.1, and for which there are no alternatives. getObjectForTrait looked like it would be perfect here.

From https://github.com/sebastianbergmann/phpunit/issues/5244:

Having to use getObjectForTrait() to test something is almost always a code smell: something is not quite right with the software design of the system-under-test.

I don't see (or smell) the 'smell' here.

Anyway, it looks like the only options are an anonymous class defined in setUp(), or a separate named class. I think I prefer the former, because everything for the TestCase will be encapsulated in the TestCase class, whereas with the latter, the reader has to look at two separate source files to see how the tests work. WDYT?

[JakeQZ]

anonymous class defined in setUp()

This can implement Commentable so it has a defined type (rather than just object).

[oliverklee]

Do you mean "... fan of adding traits only if there ..."?

Oh, yes, of course. Thanks for noticing and asking!

[oliverklee]

I'd be fine with either getObjectForTrait or a fixture class in tests that uses the trait. (I'd slightly prefer the latter.)

Edit: And I'd prefer a "real" fixture class over an anonymous class.

[JakeQZ]

I'd be fine with either getObjectForTrait or a fixture class in tests that uses the trait. (I'd slightly prefer the latter.)

getObjectForTrait is not on the menu, since we'd have to change it when we want to use PHPUnit 11.

Edit: And I'd prefer a "real" fixture class over an anonymous class.

This would also be consistent style-wise with the classes in CSSList\Fixtures (which could have been implemented using anonymous classes).