Skip to content
This repository was archived by the owner on Jun 27, 2018. It is now read-only.

Use dependency injection or similar for HTTPMessage class #8

Open
xow opened this issue Sep 30, 2016 · 16 comments
Open

Use dependency injection or similar for HTTPMessage class #8

xow opened this issue Sep 30, 2016 · 16 comments

Comments

@xow
Copy link
Contributor

xow commented Sep 30, 2016

In a lot of cases software using this library will want to control the messages being sent from this library in a specific way. An easy way of achieving that (following DataConnector) is by using DI allowing us to override the HTTPMessage class and add logic or use different curl libraries.

@jeremeamia
Copy link

@xow @spvickers I have an idea on how to support this in a backward-compatible way. Feedback?

@xow
Copy link
Contributor Author

xow commented Oct 25, 2016

@jeremeamia thanks! You might want to create it as a pull request so we can easily view it, comment on it and (in Stephen's case) merge it.

@jeremeamia
Copy link

Yep, planning on it. Just wanted to see if it was something that would satisfy your needs first. In a similar boat as you. I'm exploring the use of this library to replace some of our custom LTI code, but would like to use Guzzle as the client.

@xow
Copy link
Contributor Author

xow commented Oct 25, 2016

Definitely would do what we need and solve this issue! Since this affected both of us, sounds like the solution will be useful in a lot of different cases.

@franzliedke
Copy link

Hi folks, I've been working on cleaning up this library for a while now over at https://github.com/franzliedke/lti. Using Guzzle is part of that.

Sorry for the shameless plug, but I'd surely appreciate some collaboration. :)

@spvickers
Copy link
Contributor

Sorry late to the conversation. I do not like the HTTPMessage particularly and had been wondering about using another module instead, for example, guzzlehttp/guzzle. But is your suggestion to enable the package to have users insert their own preferred library for this? If so, is there a standard interface for such libraries which we could use to enable this to be possible?

@xow
Copy link
Contributor Author

xow commented Oct 26, 2016

@franzliedke That's a lot of changes! I haven't looked in detail, but is there any reason forking it? It would be great if improvements could be contributed upstream so we can all benefit.

@spvickers I don't mind just having functions for what the library does and we can manually use whatever library we need. But true if there's a standard interface that would be useful to follow.

@spvickers
Copy link
Contributor

@franzliedke your library you reference at https://github.com/franzliedke/lti appears to be based on the ceLTIc project library. This repository represents a siginificant rewrite of that library with the inclusion of support for LTI 2. Your comments on what needs to change in this one to meet your needs would be appreciated.

@spvickers
Copy link
Contributor

@xow so if we agreed an interface you could write your own class using your own choice of HTTP library?

@xow
Copy link
Contributor Author

xow commented Oct 26, 2016

@spvickers I will, however, mine will be just moodle's curl wrapper so it won't do anyone else much good unfortunately

@franzliedke
Copy link

@xow @spvickers When I started this rewrite (more than a year ago), I believe this project did not exist yet and I was looking for a Composer library.

As for decoupling HTTP sending from concrete implementations, that's what PSR-7 is for, and that's what I built into my fork.

@spvickers
Copy link
Contributor

@franzliedke I appreciate your project pre-dates the release of this library, but I am concerned that your shameless plug for others to collaborate in your repository could be a backward step unless you have also added in LTI 2 support. IMS welcomes contributions and suggestions which make this library better able to support the needs of users and decoupling HTTP is certainly high up on my list. I had already started looking at guzzlehttp/guzzle so am pleased to see this is one you have found useful.

@franzliedke
Copy link

@spvickers I'm beginning to think that I only want to implement the low-level basics for sending / receiving LTI messages. I will leave the rest (implementing the logic for a tool provider) up to you - maybe you can even build on top of my library when it's done.

@franzliedke
Copy link

This will reduce the complexity on both sides.

@jeremeamia
Copy link

@xow @spvickers Have a look at #12

@spvickers
Copy link
Contributor

Thanks for the pull request; we are looking at doing something in this area and will review your proposal.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants