Closed (fixed)
Project:
PDF generator API
Version:
2.x-dev
Component:
Code
Priority:
Major
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
15 Mar 2023 at 14:44 UTC
Updated:
18 May 2023 at 12:59 UTC
Jump to comment: Most recent
Comments
Comment #2
markdorisonComment #3
markdorisonComment #4
markdorisonComment #6
markdorisonComment #7
nigelcunningham commentedThe fork looks to be well maintained; let's use that for now.
Comment #8
markdorisonUpdated MR to use the fork.
Comment #11
nigelcunningham commentedSorry for my slowness. I'll try to give this a test in the morning (ADST).
Comment #12
markdorisonNo 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.
Comment #15
markdorisonComment #18
markdorison@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.
Comment #19
adamzimmermann commentedThe code changes look correct to me based upon the issue description.
Comment #21
markdorisonAfter 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: devset 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.
Comment #23
markdorisonComment #24
markdorisonAdded an option to the IS to:
@Nigel Do you have feelings about this option?
Comment #26
markdorisonI 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.
Comment #27
markdorisonComment #28
nigelcunningham commentedMorning 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 :)
Comment #31
markdorisonUpdated 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.
Comment #33
nigelcunningham commentedI'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.
Comment #34
nigelcunningham commentedComment #35
markdorison@Nigel Do you think we need to provide any notification or update to users who may have previously been using this library?
Comment #36
nigelcunningham commentedGood 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.
Comment #37
nigelcunningham commentedI'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.
Comment #38
markdorisonThere has been movement on the mpdf PR, so I am hoping it will be committed soon.
Comment #40
markdorisonThe mpdf changes have been committed. Opened a new MR (12) to include them here.
Comment #42
nigelcunningham commentedMerged, thanks!
Comment #43
markdorisonCut 2.3.1 release with this included.