Silverstripe-framework: RFC: Remove RestfulService in SS4

[sminnee]

RestfulService was written at a time when the state of the art for connecting to an API was direct Curl calls. It was also written at a time when SilverStripe had a more "kitchen sink" mentality.

These days, a developer is likely to have a much better time using something like Guzzle.

Dan has proposed that we drop it, which I agree with.

Specifically:

• deprecate RestfulService in the 3 branch
• update Oembed not to rely on it (a simple direct use of curl is probably fine for now)
• delete RestfulService from the master branch
• close all PRs and issues related to RestfulService

An open question: should we provide something compatible with the RestfulService API that acts as a layer over guzzle, most likely in a module? It doesn't seem worth maintaining so I'm inclined to say "no", but it would make upgrading easier.

[sminnee]

@dhensby, this one is for you

[micmania1]

Set it on fire!

[wilr]

:+1: remove. Implementing via Guzzle is easy enough. I would think it didn't need to be in core. If someone needs a SilverStripe wrapper around Guzzle then a module could be raised but let the need drive the demand for it.

[dhensby]

I've thought about providing a polyfill to wrap around guzzle, but I don't think it's worth our time, RestfulService is just not good enough to make it worth the time - and i have other things I'd prefer to do instead :)

[bummzack]

I preferred RestfulService over raw Curl calls (ease of use), and I have some projects where I made use of RestfulService.

Calling some REST API from your SilverStripe code is quite common and I preferred RestfulService, because it didn't add additional dependencies and pretty much did what I wanted in a one-liner.

Using something like guzzle from 4.x onward is fine for me, so I'm also in favor of removing RestfulService. I guess a wrapper won't be necessary, guzzle seems simple enough.

[kinglozzer]

I don’t see any need for maintaining a restfulservice-like wrapper around Guzzle. We may as well just move the class into a “module” for people who want a marginally easier upgrade path and just never enhance it. Not that I’m advocating that, I think we can just drop it.

[dhensby]

I'd kill Oembed too ;) (there are thirdparty libs that can do it better!)

[tractorcow]

RestfulService is terrible terrible code. Please remove it.

[sunnysideup]

I would love to see a recipe somewhere on how to set up API and I think a lot of people would benefit from that. Having a solid API with a site makes it a lot more valuable.

[sminnee]

Yeah I think a "how to move from RestfulService to Guzzle" tutorial would
be in order.

[sminnee]

I'd kill Oembed too.

Agreed, this can probably be refactored to depend on a 3rd party library. We'd want to retain the functionality, though. ;-)

[wernerkrauss]

Well a wrapper would be great for quick updates of a site.

[dhensby]

A wrapper would be nice for quick upgrades, but I don't see a big advantage in maintaining it over encouraging people to use a third party lib instead.

[sminnee]

I think a wrapper library isn't going to be maintained by the core team, but of a community member wanted to put this together, that's fine?

[dhensby]

Sure. I'd probably just suggest someone copy pastes the class into their mysite rather than have a possibly unmaintainded module

[patricknelson]

Out with RestfulService. How incredibly redundant.

We do, however, very much use oEmbed functionality which is integrated into the editor. Surely this wouldn't be on the chopping block? It's not even the topic here anyway but hopefully this isn't the case. The implementation can be overhauled, just please keep the functionality.

Edit: Maybe misread above. As long as the CMS retains native support for oEmbed, I'm happy. Definitely a good idea not to reinvent that wheel and let a third party maintain it, sure. Nuke the SilverStripe class...

[patricknelson]

Wat? https://github.com/silverstripe/silverstripe-framework/blob/ce560e7b76ee91369bd0fbd92205ad086f93305a/api/RestfulService.php#L554

Don't think I've seen a single line if statement without braces used for such a large block of code in a very long time.

[svandragt]

We use RestfulService in our main silverstripe sites to seperate the data layer api from the cms content (another silverstripe site). I've never heard of Guzzle but if it slots in easy enough and RestfulServer is as bad as you say it is then drop it for v4 I suppose. I guess we might not upgrade to version 4 any time soon.

I would support a Guzzle SilverStripe module though connecting to end points seems to be a core function of a modern CMS these days.

[kinglozzer]

I would love to see a recipe somewhere on how to set up API and I think a lot of people would benefit from that. Having a solid API with a site makes it a lot more valuable.

I've never heard of Guzzle but if it slots in easy enough and RestfulServer is as bad as you say it is then drop it for v4 I suppose

Just to clarify, as the two names are sometimes confused: we’re discussing dropping RestfulService (a curl-based class used for consuming data), not RestfulServer (a separate module to provide an API that exposes data).

[sminnee]

We've had 6 core committers give a 👍 , can @tractorcow @hafriedlander @unclecheese @jonom @halkyon @stojg vote on the original ticket?

[stojg]

I vote for removal. It should be deprecated, there are much better and more supported alternatives that can be found on packagist.

[sminnee]

Alright we have a quorum.

[sminnee]

@Firesphere has started implementation of this. One thing that it has raised is whether we should move to an external oEmbed implementation, which I've raised as a ticket here: #5487

[Firesphere]

If wished, I'll see if I can find some time to write a basic "how to use Guzzle with SilverStripe" doc.
Also, after removal, I've created a barebone RestfulService module, with deprecation notice here:
https://github.com/Firesphere/silverstripe-restfulservice
I really have _not_ paid attention to making it a good module. I just threw it in there and marked it deprecated, just in case someone migrates and uses it.

[dhensby]

Now removed from master :) thanks @Firesphere

[Firesphere]

Since members of our community raised concerns on what the effect of removing RestfulService would be on their development/upgrade process and/or how to use Guzzle instead, I wrote a little howto.
https://www.silverstripe.org/blog/removing-the-reinvented-wheel/

[dhensby]

@Firesphere thanks for that blog. There is one thing missing, which is mention of caching. RestfulService has an automatic cache layer which helps make requests through it a bit more scalable and is going to be a notable point of performance degradation when moving to vanilla guzzle as you've suggested. I'd at least point this out and possibly provide added information about caching guzzle requests.

There is a caching middleware that even has a "greedy cache" (which is basically what RestfulService did - ignored HTTP cache headers and just stored the result) - https://github.com/Kevinrob/guzzle-cache-middleware#greedy-caching, which you could link to...