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

Support http-message 2.0 as well as 1.0. #1907

Merged
merged 1 commit into from Sep 1, 2023

Conversation

NigelCunningham
Copy link
Contributor

The 2.0 release only differs in adding return type hints and removing the strict types declarations. Either version should be usable with mpdf.

This update is also needed for projects that use pdf, where other dependencies already require http-message 2. We are seeking this need in the Drupal printable / pdf_api use of mpdf; the lack of http-message 2 support is preventing an update to Drupal 10.1 (https://www.drupal.org/project/printable/issues/3374990).

@finwe
Copy link
Member

finwe commented Jul 18, 2023

See this comment, make this a draft and propose only when [you are] certain [that] composer ci on all supported PHP versions passes.

@NigelCunningham
Copy link
Contributor Author

Yeah, that's what I started on - I initially thought the solution was going to be simpler since this one isn't overriding constructors; apologies for the premature PR. On the plus side, it hopefully provides visibility that something is happening and an opportunity for testers to jump in.

May I ask what might seem like a silly question from your perspective: why are you supporting such old versions of PHP?

@finwe
Copy link
Member

finwe commented Jul 18, 2023

There are people still using them.

@NigelCunningham
Copy link
Contributor Author

Ok, and I can see there's no point going further with that question :)

@finwe
Copy link
Member

finwe commented Jul 18, 2023

Maybe not even with this PR, over the last months, I've become much closer to releasing the next major version with older PHP versions dropped.

@NigelCunningham
Copy link
Contributor Author

Should I bother continuing then? I have plenty of other things I could spend the time on.

@finwe
Copy link
Member

finwe commented Jul 18, 2023

Depends on if and how fast you need this, I'd give the drop a few months (which is quite fast given how the library is evolving)

@NigelCunningham
Copy link
Contributor Author

Ok. We have people wanting to use our Drupal module with D10 today, so I'll keep going.

@finwe
Copy link
Member

finwe commented Jul 18, 2023

I'll create the repository for the module and give you access, so you can work under mpdf namespace directly.

@NigelCunningham
Copy link
Contributor Author

Oh, that would be great; thanks.

@NigelCunningham NigelCunningham force-pushed the http_message_2_support branch 2 times, most recently from 2804908 to abb6203 Compare July 19, 2023 11:16
@finwe
Copy link
Member

finwe commented Jul 19, 2023

@NigelCunningham Can you please mark this as a draft until there is actually anything to review? I'm getting notified for every commit.

@NigelCunningham
Copy link
Contributor Author

Ok, I've updated this PR and pushed to the http-message-aware-trait repo, having tested on Ubuntu with PHP 5.6, 7.0, 7.1, 7.2, 7.3, 7.4, 8.0, 8.1, 8.2.

It all works locally and I've also tested using the repository on github.

Copy link
Member

@finwe finwe left a comment

Choose a reason for hiding this comment

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

Looks on a good path, just maybe, seeing that all the content of the classes is removed, wouldn't it be better to move whole classes to the package insteadof using a trait in an empty class? of course the package would then have to be renamed.

composer.json Outdated Show resolved Hide resolved
src/Http/Request.php Outdated Show resolved Hide resolved
@NigelCunningham NigelCunningham marked this pull request as draft August 31, 2023 22:34
@NigelCunningham NigelCunningham force-pushed the http_message_2_support branch 2 times, most recently from d18273f to 950270b Compare September 1, 2023 02:07
Signed-off-by: Nigel Cunningham <nigel@technocrat.com.au>
@NigelCunningham
Copy link
Contributor Author

NigelCunningham commented Sep 1, 2023

I believe all issues have been resolved now. I've renamed the repo contents to mpdf/psr-http-message-shim. If you're able to rename the repo as well, everything should just work.

I've been testing with the following addition to the mpdf/mpdf composer.json:

diff --git a/composer.json b/composer.json
index 198b2c9..efc17ed 100644
--- a/composer.json
+++ b/composer.json
@@ -67,5 +67,11 @@
        },
        "config": {
                "sort-packages": true
+       },
+       "repositories": {
+               "mpdf/psr-http-message-shim": {
+                       "type": "vcs",
+                       "url": "https://github.com/mpdf/http-message-aware-trait.git"
+               }
        }
 }

... and have successfully checked tests pass with PHP 5.6, 7.0 through 7.4 inclusive and 8.0 and 8.1.

@NigelCunningham NigelCunningham marked this pull request as ready for review September 1, 2023 06:30
@finwe finwe merged commit 45ea7fb into mpdf:development Sep 1, 2023
19 checks passed
@finwe
Copy link
Member

finwe commented Sep 1, 2023

Great job, thanks!

@NigelCunningham
Copy link
Contributor Author

You're welcome!

@NigelCunningham
Copy link
Contributor Author

Could I just query as to when you're planning another release? I can make the Drupal module that depends on this use the development branch for now, but it would be better to avoid that if I can :)

@finwe
Copy link
Member

finwe commented Sep 1, 2023

Shortly. I just need to resolve one seemingly minor issue elsewhere to release 8.2.0

@NigelCunningham
Copy link
Contributor Author

Great; thanks!

@finwe
Copy link
Member

finwe commented Sep 1, 2023

Released.

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.

None yet

2 participants