Problem/Motivation

\Drupal\media\Plugin\media\Source\OEmbed and \Drupal\media\Controller\OEmbedIframeController expect the \Drupal\Core\Logger\LoggerChannelInterface class injected into their constructors. This is incompatible with the monolog module.

Related issues for contrib:
#2884802: Incompatible with monolog
#2960531: Incompatible with monolog module
#2868851: Search api and monolog module don't work together.
#2909673: Doesn't work with Monolog module

Proposed resolution

Change signature to use \Psr\Log\LoggerInterface instead.

Kudos to @rikki_iki for finding this :)

CommentFileSizeAuthor
#2 3000424-oembed-monolog-2.patch5.7 KBacbramley
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

acbramley created an issue. See original summary.

acbramley’s picture

acbramley CreditAttribution: acbramley at PreviousNext commented
Status: Active » Needs review
FileSize
3000424-oembed-monolog-2.patch5.7 KB
acbramley’s picture

acbramley CreditAttribution: acbramley at PreviousNext commented
Title: OEmbed Media Source and related controller/formatter use LoggerChannelInterface in their constructors which is not compatible with the monolog module » OEmbed Media Source and related controller use LoggerChannelInterface in their constructors which is not compatible with the monolog module
acbramley’s picture

acbramley CreditAttribution: acbramley at PreviousNext commented
Issue summary: View changes

Sam152 credited rikki_iki.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented

Looks good to me, the interface of LoggerChannelInterface says:

 * This interface defines the full behavior of the central Drupal logger
 * facility. However, when writing code that does logging, use the generic
 * \Psr\Log\LoggerInterface for typehinting instead (you shouldn't need the
 * methods here).

So I can't see any issues with changing this. Will see what the testbot says. Not exactly sure if testing this makes sense, I suppose we could use a different logger in the oembed tests, but haven't seen any examples of that done elsewhere.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

I am wondering whether something like this could be automatically checked.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Otherwise, totally +1

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

I think best thing to do will be to add a follow-up task to mark the LoggerChannelInterface @internal. So at least IDEs will help us. This task should also replace other incorrect typehints.

Also how does this break the monolog module? I don't think this is true since #2749859: Logger channel factory must return instances of LoggerChannelInterface.

But there is no harm in obeying the instructions not to use it that exist on the interface. As an API widening (not change) I don't think this needs tests.

Committed and pushed e6a096cf45 to 8.7.x and faf551f882 to 8.6.x. Thanks!

  • alexpott committed e6a096c on 8.7.x 
    Issue #3000424 by acbramley, rikki_iki: OEmbed Media Source and related...

  • alexpott committed faf551f on 8.6.x 
    Issue #3000424 by acbramley, rikki_iki: OEmbed Media Source and related...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.