Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Referencer/Dereferencer logger error #4217

Merged
merged 10 commits into from
Jul 17, 2024
Merged

Referencer/Dereferencer logger error #4217

merged 10 commits into from
Jul 17, 2024

Conversation

paul-m
Copy link
Contributor

@paul-m paul-m commented Jul 8, 2024

Fixes #4207

Originally, this was reported against Referencer and Dereferencer, but it's not just those.

#4140 broke a bunch of logging functionality, so we fix it here.

Explicitly adds colinodell/psr-testlogger to our dev dependencies, but it's already a dependency of drupal/core-dev: https://git.drupalcode.org/project/drupal/-/blob/10.1.x/composer.json?ref_type=heads#L21

All the classes that use logging incorrectly:

  • modules/datastore/src/Storage/DatabaseTable.php
  • modules/metastore/src/MetastoreService.php
  • modules/metastore/src/Reference/Dereferencer.php
  • modules/metastore/src/Reference/Referencer.php
  • modules/metastore/src/Storage/Data.php
  • modules/metastore/src/Storage/ResourceMapperDatabaseTable.php

All these classes now call their logger as if it were a \Psr\Log\LoggerInterface (because it is).

All these classes now have tests which touch the logging call, except ResourceMapperDatabaseTable, which is deprecated (but still fixed).

Another notable exception is Referencer::referenceSingle(), where the logging code seems to be unreachable by test. One wonders if it counts as dead code.

QA steps include:

  • Looking for obvious errors.
  • Verifying that these lines of code are covered by tests in a coverage report (see the CircleCI Artifacts tab, for the Install Target build). The exception here is ResourceMapperDatabaseTable, which we didn't add tests for because it's deprecated.

@swirtSJW
Copy link
Contributor

I tried applying the patch from this to 2.18.3 and would not apply. I'll try a more up-to-date install.

*/
private function htmlPurifier(string $input) {
// Initialize HTML Purifier cache config settings array.
$config = [];

// Determine path to tmp directory.
// @todo Inject this service.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean this as a reminder to yourself for this ticket or for future selves?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our future selves. 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always have this same dilemma myself. When I am in the middle of building something I leave myself a trail of @todos . I have always thought there should be two levels. @todo and @now-dammit

Copy link
Contributor

@swirtSJW swirtSJW left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was able to test this on PDC.
Visiting the revisions page for a dataset no longer shows any warnings
image

Can confirm that the logging is working becuase now it is logging correctly
image

Code looks good. Nice work adding new test coverage.

@janette janette merged commit 228bdb4 into 2.x Jul 17, 2024
10 checks passed
@janette janette deleted the dereferencer-logger branch July 17, 2024 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Warning: Undefined array key "value_referencer" in Drupal\Core\Logger\LoggerChannel->log()
3 participants