Problem/Motivation

Proposed resolution

  1. mpdf briefly included psr/log 3.0 support in version 8.0.16 before reverting it in 8.0.17. We could pin the requirement to 8.0.16.
  2. There is a fork that includes psr/log 3.0 support. Switching to the fork creates an issue since we would be requiring a branch of the dependency which means it is a "dev" release. This could (would?) block users of this module from upgrading unless they have minimum-stability: dev set in their project's root composer.json A tag has been created on the fork so we could pull it in with that tag
  3. Per a suggestion greg.1.anderson: Create a new Composer package implementing a trait that would support using either psr/log v2 or v3. Attempt to get a PR accepted by mpdf using this package allowing both psr/log v2 and v3 to be supported simultaneously. This approach is now in a PR under review and if accepted could be merged in May.
  4. Remove support for mpdf/mpdf until support for psr/log v3 is added. We could release this change as a new major version since we would be removing support for mpdf.

Issue fork pdf_api-3348174

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

markdorison created an issue. See original summary.

markdorison’s picture

Issue summary: View changes
markdorison’s picture

Issue summary: View changes
markdorison’s picture

Title: dompdf dependency of psr/log blocking Drupal 10 update » mpdf dependency of psr/log blocking Drupal 10 update
Issue summary: View changes

markdorison’s picture

Status: Active » Needs review
nigelcunningham’s picture

The fork looks to be well maintained; let's use that for now.

markdorison’s picture

Updated MR to use the fork.

nigelcunningham’s picture

Sorry for my slowness. I'll try to give this a test in the morning (ADST).

markdorison’s picture

Status: Needs review » Needs work

No worries Nigel. I am trying to resolve the Tugboat build issue that is cropping up here that is occurring due to the fork. Slack discussion.

markdorison’s picture

Issue summary: View changes
Status: Needs work » Needs review

markdorison’s picture

@Nigel, This is ready for your review. I am a little concerned about projects potentially needing to specify "minimum-stability: dev" in their own composer.json file for Composer to pull in this release. Testing confidently is tricky, but once this is committed, it should be more straightforward.

adamzimmermann’s picture

Status: Needs review » Reviewed & tested by the community

The code changes look correct to me based upon the issue description.

  • markdorison committed 3f2d5e3d on 2.x
    Issue #3348174 by markdorison: mpdf dependency of psr/log blocking...
markdorison’s picture

Status: Reviewed & tested by the community » Needs work

After some testing and this super-informative thread in Drupal Slack, I am concerned that requiring the fork on a dev branch could block users of this module from upgrading unless they have minimum-stability: dev set in their project's root composer.json. Greg Anderson did suggest a potential workaround in the thread. I will update the issue summary with our current options.

In the meantime, I am going to revert 3f2d5e3d.

  • markdorison committed 63860fe1 on 2.x
    Revert "Issue #3348174 by markdorison: mpdf dependency of psr/log...
markdorison’s picture

Issue summary: View changes
markdorison’s picture

Issue summary: View changes

Added an option to the IS to:

Remove support for mpdf/mpdf until support for psr/log v3 is added.

@Nigel Do you have feelings about this option?

markdorison’s picture

Issue summary: View changes
Status: Needs work » Needs review

I have created an MR showing the option of removing mpdf support. If we go this route, I think we would want to commit it to a 3.x branch and release it as a 3.0 since we are dropping support (for now) for this option.

markdorison’s picture

Issue summary: View changes
nigelcunningham’s picture

Morning Mark.

I'd like to get an idea of how many users are making use of the library, if that's possible (and what their opinions are).

I might also have another alternative - I've already forked another project that needed the same support and could do the same for mPDF as a short term solution. If we have control of the fork (and of course we will), we should be able to deal with versioning / stability level issues.

Of course the other problems is my finding time to do all these things, but the big project I've been working on for about a year and a half is getting to completion, so I might have a window coming :)

markdorison’s picture

Issue summary: View changes
Status: Needs review » Needs work

Updated issue summary with the latest developments. I am currently waiting to see if this mpdf PR gets accepted which would add psr/log v3 support.

nigelcunningham’s picture

I've merged MR11. On github, mPDF has far fewer stars than the other options, so I'm assuming that means fewer users too.

"In May after I've returned from travels" (the referenced thread in #31) might never happen. Let's just get on with life. If it does happen, we can always re-add mPDF support.

nigelcunningham’s picture

Status: Needs work » Fixed
markdorison’s picture

@Nigel Do you think we need to provide any notification or update to users who may have previously been using this library?

nigelcunningham’s picture

Good point.

Perhaps we should update the project page.

I'll make a note to myself to do that when I start work in an hour or so.

nigelcunningham’s picture

I've added the following to the project front page:

In the current development branch, we have removed support for mPDF, due to mPDF not supporting psr logger changes that are required for Drupal 10 support. This removal will be reverted once https://github.com/mpdf/mpdf/pull/1857 is resolved in a way that allows us to support mPDF and Drupal 10.

markdorison’s picture

There has been movement on the mpdf PR, so I am hoping it will be committed soon.

markdorison’s picture

Status: Fixed » Needs review

The mpdf changes have been committed. Opened a new MR (12) to include them here.

nigelcunningham’s picture

Merged, thanks!

markdorison’s picture

Status: Needs review » Fixed

Cut 2.3.1 release with this included.

Status: Fixed » Closed (fixed)

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