oat-sa / lib-lti1p3-core

PHP library for LTI 1.3 Core implementations as platforms and / or as tools.
GNU General Public License v2.0
33 stars 16 forks source link

fix: replace abandoned php-http/message-factory with psr/http-factory #165

Closed kochen closed 11 months ago

kochen commented 1 year ago

fixes #164

kochen commented 11 months ago

@wazelin thanks for taking the time to look at this!

The change I proposed does not require any additional changes (and passes all tests).

I suggest we first deal with the reported vulnerability and then look at the rest. Could you approve/merge this #169 first?

wazelin commented 11 months ago

Could you approve/merge this #169 first?

169 is fine indeed. Thank you!

That new Sonarqube integration is misbehaving a little, but that's not related to the changes introduced.

kochen commented 11 months ago

@wazelin I refactored the code to use Nyholm\Psr7\Response (and removed the abandoned php-http/message-factory).

There are 2 commits here, because I'm using the "bumped" versions from #166

kochen commented 11 months ago

Sure, though I would still suggest tackling these tasks independently and also merging them one by one.

kochen commented 11 months ago

@wazelin the libraries cannot be updates, because they rely on core v6.9 which uses the ResponseFactory.

Could you maybe release v7-beta/RC so the code adjustments could be done?

wazelin commented 11 months ago

@wazelin the libraries cannot be updates, because they rely on core v6.9 which uses the ResponseFactory.

Could you maybe release v7-beta/RC so the code adjustments could be done?

I'm not quite sure I understand. Removing ResponseFactory usage doesn't really require having that same factory removed from the core library, does it? I mean, to me it seems that places like this one here should be refactored to return an instance of Nyholm\Psr7\Response explicitly.

kochen commented 11 months ago

That's true for all the src code, but tests like this (and many others), still expect the factory being passed.

So I could fix the tests for v6.9, but once v7 is released the tests would fail.

wazelin commented 11 months ago

I've opened alternative PRs for all of the lti1p3 libraries and the Symfony bundle, @kochen. Let me know what you think.

kochen commented 11 months ago

looks great!

I'm following... ;)