Skip to content

Commit 228bdb4

Browse files
paul-mpaul-m
authored
Fix Referencer/Dereferencer logger error (#4217)
1 parent 6d55543 commit 228bdb4

File tree

13 files changed

+349
-67
lines changed

13 files changed

+349
-67
lines changed

‎composer.json

+1
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
"symfony/polyfill-php80": "^1.27"
3939
},
4040
"require-dev": {
41+
"colinodell/psr-testlogger": "^1.2",
4142
"drush/drush": "^12@stable",
4243
"getdkan/mock-chain": "^1.3.7",
4344
"osteel/openapi-httpfoundation-testing": "<1.0",

‎modules/datastore/src/Storage/DatabaseTable.php

+8-10
Original file line numberDiff line numberDiff line change
@@ -101,19 +101,17 @@ public function getTableName() {
101101
protected function prepareData(string $data, string $id = NULL): array {
102102
$decoded = json_decode($data);
103103
if ($decoded === NULL) {
104-
$this->logger->log(
105-
'datastore_import',
106-
'Error decoding id:@id, data: @data.',
107-
['@id' => $id, '@data' => $data]
108-
);
104+
$this->logger->error('Error decoding id:@id, data: @data.', [
105+
'@id' => $id,
106+
'@data' => $data,
107+
]);
109108
throw new \Exception('Import for ' . $id . ' error when decoding ' . $data);
110109
}
111110
elseif (!is_array($decoded)) {
112-
$this->logger->log(
113-
'datastore_import',
114-
'Array expected while decoding id:@id, data: @data.',
115-
['@id' => $id, '@data' => $data]
116-
);
111+
$this->logger->error('Array expected while decoding id:@id, data: @data.', [
112+
'@id' => $id,
113+
'@data' => $data,
114+
]);
117115
throw new \Exception('Import for ' . $id . ' returned an error when preparing table header: ' . $data);
118116
}
119117
return $decoded;

‎modules/datastore/tests/src/Kernel/Storage/DatabaseTableTest.php

+58
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,15 @@
22

33
namespace Drupal\Tests\datastore\Kernel\Storage;
44

5+
use ColinODell\PsrTestLogger\TestLogger;
56
use Drupal\common\DataResource;
67
use Drupal\common\Storage\ImportedItemInterface;
8+
use Drupal\datastore\DatastoreResource;
79
use Drupal\datastore\Plugin\QueueWorker\ImportJob;
810
use Drupal\datastore\Service\Factory\ImportServiceFactory;
911
use Drupal\datastore\Storage\DatabaseTable;
1012
use Drupal\KernelTests\KernelTestBase;
13+
use Drupal\Tests\common\Unit\Connection;
1114
use Procrastinator\Result;
1215

1316
/**
@@ -61,4 +64,59 @@ public function testIsNotImportedItemInterface() {
6164
$this->assertEquals(2, $import_job->getStorage()->count());
6265
}
6366

67+
public function providePrepareData() {
68+
return [
69+
// Bad JSON results in a NULL on decode.
70+
[
71+
'Error decoding id:@id, data: @data.',
72+
'Import for error when decoding "badjson""',
73+
'"badjson""',
74+
],
75+
// The decoded JSON is supposed to be an array.
76+
[
77+
'Array expected while decoding id:@id, data: @data.',
78+
'Import for returned an error when preparing table header: {"this_is": "an_object"}',
79+
'{"this_is": "an_object"}',
80+
],
81+
];
82+
}
83+
84+
/**
85+
* @covers ::prepareData
86+
* @dataProvider providePrepareData
87+
*/
88+
public function testPrepareDataLogging($expected_log, $expected_exception, $data) {
89+
// Get a logger that we can assert against.
90+
$logger = new TestLogger();
91+
92+
// We have to mock tableExist() because otherwise it tries to talk to the
93+
// database, which we don't actually have right now.
94+
$database_table = $this->getMockBuilder(DatabaseTable::class)
95+
->onlyMethods(['tableExist'])
96+
->setConstructorArgs([
97+
$this->createStub(Connection::class),
98+
$this->createStub(DatastoreResource::class),
99+
$logger,
100+
])
101+
->getMock();
102+
$database_table->expects($this->any())
103+
->method('tableExist')
104+
->willReturn(FALSE);
105+
106+
$ref_prepare_data = new \ReflectionMethod($database_table, 'prepareData');
107+
$ref_prepare_data->setAccessible(TRUE);
108+
109+
// We can't use expectException() because we also want to look at the log.
110+
try {
111+
$ref_prepare_data->invokeArgs($database_table, [$data]);
112+
$this->assertTrue(FALSE, 'We expected this call to throw an exception.');
113+
}
114+
catch (\Exception $e) {
115+
$this->assertSame($e->getMessage(), $expected_exception);
116+
$this->assertTrue(
117+
$logger->hasErrorThatContains($expected_log)
118+
);
119+
}
120+
}
121+
64122
}

‎modules/metastore/src/MetastoreService.php

+4-3
Original file line numberDiff line numberDiff line change
@@ -215,9 +215,10 @@ function ($jsonString) use ($schema_id) {
215215
});
216216
}
217217
catch (\Exception) {
218-
$this->logger->log('metastore', 'A JSON string failed validation.',
219-
['@schema_id' => $schema_id, '@json' => $jsonString]
220-
);
218+
$this->logger->error('A JSON string failed validation.', [
219+
'@schema_id' => $schema_id,
220+
'@json' => $jsonString,
221+
]);
221222
return NULL;
222223
}
223224
}, $jsonStringsArray);

‎modules/metastore/src/Reference/Dereferencer.php

+8-13
Original file line numberDiff line numberDiff line change
@@ -103,11 +103,10 @@ private function dereferencePropertyUuid(string $property_id, $uuid) {
103103
return $this->dereferenceSingle($property_id, $uuid);
104104
}
105105
else {
106-
$this->logger->log('value_referencer', 'Unexpected data type when dereferencing property_id: @property_id with uuid: @uuid',
107-
[
108-
'@property_id' => $property_id,
109-
'@uuid' => var_export($uuid, TRUE),
110-
]);
106+
$this->logger->error('Unexpected data type when dereferencing property_id: @property_id with uuid: @uuid', [
107+
'@property_id' => $property_id,
108+
'@uuid' => var_export($uuid, TRUE),
109+
]);
111110
return NULL;
112111
}
113112
}
@@ -168,14 +167,10 @@ private function dereferenceSingle(string $property_id, string $uuid) {
168167

169168
// If a property node was not found, it most likely means it was deleted
170169
// while still being referenced.
171-
$this->logger->log(
172-
'value_referencer',
173-
'Property @property_id reference @uuid not found',
174-
[
175-
'@property_id' => $property_id,
176-
'@uuid' => var_export($uuid, TRUE),
177-
]
178-
);
170+
$this->logger->error('Property @property_id reference @uuid not found', [
171+
'@property_id' => $property_id,
172+
'@uuid' => var_export($uuid, TRUE),
173+
]);
179174

180175
return [NULL, NULL];
181176
}

‎modules/metastore/src/Reference/Referencer.php

+7-10
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,6 @@ private function referenceMultiple(string $property_id, array $values) : array {
175175
* The Uuid reference, or NULL on failure.
176176
*/
177177
private function referenceSingle(string $property_id, $value) {
178-
179178
if ($property_id == 'distribution') {
180179
$value = $this->distributionHandling($value);
181180
}
@@ -188,14 +187,12 @@ private function referenceSingle(string $property_id, $value) {
188187
return $uuid;
189188
}
190189
else {
191-
$this->logger->log(
192-
'value_referencer',
193-
'Neither found an existing nor could create a new reference for property_id: @property_id with value: @value',
194-
[
195-
'@property_id' => $property_id,
196-
'@value' => var_export($value, TRUE),
197-
]
198-
);
190+
// @todo Based on the fact that we can't make a test hit these lines, it
191+
// seems that they are dead code.
192+
$this->logger->error('Neither found an existing nor could create a new reference for property_id: @property_id with value: @value', [
193+
'@property_id' => $property_id,
194+
'@value' => var_export($value, TRUE),
195+
]);
199196
return NULL;
200197
}
201198
}
@@ -349,7 +346,7 @@ private function getLocalMimeType(string $downloadUrl): ?string {
349346
// If we couldn't find a mime type, log an error notifying the user.
350347
if (is_null($mime_type)) {
351348
$filename = basename($downloadUrl);
352-
$this->logger->log('value_referencer', 'Unable to determine mime type of file with name "@name".', [
349+
$this->logger->error('Unable to determine mime type of file with name "@name".', [
353350
'@name' => $filename,
354351
]);
355352
}

‎modules/metastore/src/Storage/Data.php

+2-3
Original file line numberDiff line numberDiff line change
@@ -456,21 +456,20 @@ private function filterHtml(mixed $input, string $parent = 'dataset') {
456456
*
457457
* @return string
458458
* Filtered string.
459-
*
460-
* @codeCoverageIgnore
461459
*/
462460
private function htmlPurifier(string $input) {
463461
// Initialize HTML Purifier cache config settings array.
464462
$config = [];
465463

466464
// Determine path to tmp directory.
465+
// @todo Inject this service.
467466
$tmp_path = \Drupal::service('file_system')->getTempDirectory();
468467
// Specify custom location in tmp directory for storing HTML Purifier cache.
469468
$cache_dir = rtrim($tmp_path, '/') . '/html_purifier_cache';
470469

471470
// Ensure the tmp cache directory exists.
472471
if (!is_dir($cache_dir) && !mkdir($cache_dir)) {
473-
$this->logger->log('metastore', 'Failed to create cache directory for HTML purifier');
472+
$this->logger->error('Failed to create cache directory for HTML purifier');
474473
}
475474
else {
476475
$config['Cache.SerializerPath'] = $cache_dir;

‎modules/metastore/src/Storage/ResourceMapperDatabaseTable.php

+10-15
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44

55
use Drupal\Core\Database\Connection;
66
use Drupal\common\Storage\AbstractDatabaseTable;
7-
use Psr\Log\LogLevel;
87
use Psr\Log\LoggerInterface;
98

109
/**
@@ -105,22 +104,18 @@ protected function prepareData(string $data, string $id = NULL): array {
105104
}
106105

107106
if ($decoded === NULL) {
108-
$this->logger->log(
109-
'dkan_metastore_filemapper',
110-
"Error decoding id:@id, data: @data.",
111-
['@id' => $id, '@data' => $data],
112-
LogLevel::ERROR
113-
);
114-
throw new \Exception("Import for {$id} error when decoding {$data}");
107+
$this->logger->error('Error decoding id:@id, data: @data.', [
108+
'@id' => $id,
109+
'@data' => $data,
110+
]);
111+
throw new \Exception('Import for ' . $id . ' error when decoding ' . $data);
115112
}
116113
elseif (!is_object($decoded)) {
117-
$this->logger->log(
118-
'dkan_metastore_filemapper',
119-
"Object expected while decoding id:@id, data: @data.",
120-
['@id' => $id, '@data' => $data],
121-
LogLevel::ERROR
122-
);
123-
throw new \Exception("Import for {$id} returned an error when preparing table header: {$data}");
114+
$this->logger->error('Object expected while decoding id:@id, data: @data.', [
115+
'@id' => $id,
116+
'@data' => $data,
117+
]);
118+
throw new \Exception('Import for ' . $id . ' returned an error when preparing table header: ' . $data);
124119
}
125120
return (array) $decoded;
126121
}

‎modules/metastore/tests/src/Functional/Storage/DataTest.php

+121
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
<?php
2+
3+
namespace Drupal\Tests\metastore\Functional\Storage;
4+
5+
use ColinODell\PsrTestLogger\TestLogger;
6+
use Drupal\Core\Config\ConfigFactoryInterface;
7+
use Drupal\Core\Entity\EntityTypeManagerInterface;
8+
use Drupal\Core\File\FileSystem;
9+
use Drupal\Tests\BrowserTestBase;
10+
use Drupal\metastore\Storage\Data;
11+
use Psr\Log\LoggerInterface;
12+
use org\bovigo\vfs\vfsStream;
13+
14+
/**
15+
* Test the Data class.
16+
*
17+
* Because of the tight coupling and unification of concerns, this is a
18+
* BrowserTestBase test. A Kernel test would be too complex and fragile.
19+
*
20+
* @covers \Drupal\metastore\Storage\Data
21+
* @coversDefaultClass \Drupal\metastore\Storage\Data
22+
*
23+
* @group dkan
24+
* @group metastore
25+
* @group functional
26+
* @group btb
27+
*/
28+
class DataTest extends BrowserTestBase {
29+
30+
protected $defaultTheme = 'stark';
31+
32+
protected static $modules = [
33+
'metastore',
34+
'node',
35+
];
36+
37+
/**
38+
* Test the logging of htmlPurifier().
39+
*
40+
* @covers ::htmlPurifier
41+
*/
42+
public function testHtmlPurifierLogging() {
43+
// Set up a read-only temp directory.
44+
$temp = vfsStream::setup('temp');
45+
$temp_dir = vfsStream::newDirectory('mytemp', 0000)
46+
->at($temp);
47+
48+
// Tell the file system service to use this temp directory, which will
49+
// prevent the HTML purifier from writing its cache directory, and it
50+
// should log this fact.
51+
$fs = $this->getMockBuilder(FileSystem::class)
52+
->disableOriginalConstructor()
53+
->onlyMethods(['getTempDirectory'])
54+
->getMock();
55+
$fs->expects($this->any())
56+
->method('getTempDirectory')
57+
->willReturn($temp_dir->url());
58+
59+
// Let's keep the old filesystem object so we can set it back.
60+
$old_fs = $this->container->get('file_system');
61+
$this->container->set('file_system', $fs);
62+
63+
// Create a Data object with a testable logger.
64+
$logger = new TestLogger();
65+
$data = new StubData(
66+
'dataset',
67+
$this->container->get('entity_type.manager'),
68+
$this->container->get('config.factory'),
69+
$logger
70+
);
71+
72+
$uuid = '05aea36e-9e24-452e-9cf9-9727ab90c198';
73+
74+
// Since filterHtml() and htmlPurifier() are private, we call in from
75+
// store() with crafted data.
76+
$identifier = $data->store(json_encode((object) [
77+
'identifier' => $uuid,
78+
'title' => 'title',
79+
'data' => (object) [
80+
'description' => 'purify me',
81+
],
82+
]));
83+
84+
// We stored a node.
85+
$this->assertSame($uuid, $identifier);
86+
// We logged that the cache directory was not created.
87+
$this->assertTrue(
88+
$logger->hasErrorThatContains('Failed to create cache directory for HTML purifier')
89+
);
90+
91+
// Test can't clean up if we don't set back the old file system service.
92+
$this->container->set('file_system', $old_fs);
93+
}
94+
95+
}
96+
97+
/**
98+
* Stub out the abstract Data class with just enough to make it run.
99+
*
100+
* Values are copied from NodeData.
101+
*/
102+
class StubData extends Data {
103+
104+
public function __construct(string $schemaId, EntityTypeManagerInterface $entityTypeManager, ConfigFactoryInterface $config_factory, LoggerInterface $loggerChannel) {
105+
$this->entityType = 'node';
106+
$this->bundle = 'data';
107+
$this->bundleKey = 'type';
108+
$this->labelKey = 'title';
109+
$this->schemaIdField = 'field_data_type';
110+
$this->metadataField = 'field_json_metadata';
111+
parent::__construct($schemaId, $entityTypeManager, $config_factory, $loggerChannel);
112+
}
113+
114+
public function retrieveContains(string $string, bool $caseSensitive): array {
115+
return [];
116+
}
117+
118+
public function retrieveByHash($hash, $schemaId) {
119+
}
120+
121+
}

0 commit comments

Comments
 (0)