Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
There are two DiffFormatter classes. One is in namespace \Drupal\Component\Diff, and the other is in \Drupal\Core\Diff.
When these two were separated out, there was one thing that wasn't done quite right, apparently, and it's causing exceptions in a test that is using the Component class to format a diff.
So, in my code (in a contrib module), I'm calling the component class's DiffFormatter::format() method.
It has these two lines that say:
if (!empty($xi)) {
$this->line_stats['counter']['x'] += $xi;
}
if (!empty($yi)) {
$this->line_stats['counter']['y'] += $yi;
}
near the bottom.
But $this->line_stats is not defined, and so I'm getting 4 exceptions in my test that say:
Undefined property: Drupal\Component\Diff\DiffFormatter::$line_stats
Undefined index: counter
Undefined index: x
Undefined index: y
The problem is that $this->line_stats is only defined in the Core class, like this:
/**
* The line stats.
*
* @var array
*/
protected $line_stats = array(
'counter' => array('x' => 0, 'y' => 0),
'offset' => array('x' => 0, 'y' => 0),
);
Proposed resolution
Either (a) move the protected $line_stats declaration into the Component class, or (b) remove those two lines of code from the Component class.
Also apparently the Component class is not being tested in Core, or this problem would have probably occurred in the test, so also a test needs to be written.
Remaining tasks
Figure out what to do and do it, and write a test for the Component class.
User interface changes
None.
API changes
Not really, except the Component class might actually work.
Data model changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | 2824410-fix-line-stats-test-only-FAIL.patch | 1.66 KB | jhodgdon |
| #5 | 2824410-fix-line-stats-with-test.patch | 2.9 KB | jhodgdon |
Comments
Comment #2
jhodgdonHere is a patch that fixes my tests (no exceptions now). Doesn't add a test for the Core stuff...
Comment #3
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedLooking at the
DiffFormatterclass from Component, it does use this protected variable, so I think option a) from the IS (and the patch from #2) is the way to go :)Comment #4
jhodgdonAgreed. The other option would be to remove those two lines from the Component format() method, and override it in the Core format() method, because that's the only use of it in the Component (setting the values, never reads them), and the Core class is what actually I think does something with them.
Anyway it does need a test. I have a failing test on #2394603: Add tests for Drush commands but that's not going to help much... Easy to make a pure core test though. I'll see what I can do.
Comment #5
jhodgdonOK, here's a patch with a simple test, and a test-only patch. Tests passes locally with the patched diffformatter classes... let's see what the bot thinks (and hopefully amateescu).
I didn't make an interdiff. The only difference is the added test.
Comment #7
jhodgdonThe test-only patch is failing as expected, with:
Undefined property: Drupal\Component\Diff\DiffFormatter::$line_stats
So I think this is ready to go.
@amateescu, care to review this fairly simple test and patch? The test could probably be improved, but at least it provides a basic test of the DiffFormatter component (and the Diff component as a by-product).
Comment #8
dawehnerNice, named tests
Comment #9
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedLooks good to me as well :)
Comment #10
alexpottNice find and great to see a test :)
Committed and pushed 3408c30 to 8.3.x and ad68e1b to 8.2.x. Thanks!
Minor code style fixes on commit.
There's been some debate about this is other issues with people seemingly falling on the use
Drupal\Tests\UnitTestCaseside because it's "consistent". Given that this test is way away from the container and using config etc. \PHPUnit_Framework_TestCase is absolutely fine. All the other tests inDrupal\Tests\Component\Diffare using it and I think all of the component tests should be because components are supposed to be separate from the application. Removed unused use.