Use SVG to generate thumbnails for text-only tweets

This looks amazing indeed! Just two nitpiks.

Some review:

1. +  if ($variables['avatar']) {
   +    $data = file_get_contents($variables['avatar']);
   

   Would this make an external request every time the preprocess function is called? I'm guessing that once the SVG is generated this will never be called again, so that might be OK. Just checking!

2. +      $variables['avatar'] = 'data:image/' . strtolower($extension) . ';base64,' . base64_encode($data);
   

   Due to security paranoia, it might be good to hard-core the image extension if we know what to expect from Twitter. For example, if the extension of the avatar is SVG, that SVG could contact executable code.

I also mentioned at NYCCamp that there is a fair use concern with making copies of Tweets. You can read Twitter's article on this here: https://support.twitter.com/articles/20171959. The TL;DR (IANAL) of the liability is that if I make a Tweet and you make a copy of it on your site that violates Fair Use, I can file a complaint against Twitter and might get your API access revoked. This sounds scary, but if these thumbnails are only displayed in the backend it's unlikely that you are going to get complaints filed against you (unless your content editors created a Tweet you've embedded). Even if a complaint does get filed against your account, if you're only using this in an admin interface (i.e. the thumbnails are not publicly accessible), you are probably fine.

... but if we want to err on the side of paranoia, we could probably validate the extension before the SVG is generated ...

Yep, that would work for me. The specific exploits I can think of are 1) Embedding SVGs with untrusted data 2) Malformed extensions that could break out of the tag. I guess that we should trust Twitter to validate extensions, but the more checks we can do, the better!

... I suggest that we add a checkbox to the Twitter media type plugin's configuration that specifically enables the auto-generation of thumbnails ...

That, or any explicit documentation should be fine. I'm not sure how to properly warn users when enabling a module, so a check box makes sense since they have to interact with it.

A bit tricky to test. Here are few ideas that maybe can help.

1. create few fixtures files with exported result that you get from fetchTweet() execution
2. then you can mock service to return saved fixture (depending of tweet id, so that you can test with few different tweets)
3. there is also option to create test module -> and in that module override your service to do mocking as explained above -> then you can go with functional tests, I guess, instead of unit

Finally managed to look at this. Looks great. I have a few comments, but we should be almost there. Great work!

1. +++ b/src/Plugin/MediaEntity/Type/Twitter.php
   @@ -138,21 +162,23 @@ class Twitter extends MediaTypeBase {
   +              $image_url = $this->getField($media, 'image');
   +              return file_unmanaged_save_data($image_url, $local_uri, FILE_EXISTS_REPLACE);
   

   $image_url is not the actual data... Yes, if this is broken then it is an existing bug, but would be nice to fix it anyway.

2. +++ b/src/TweetFetcher.php
   @@ -0,0 +1,91 @@
   +
   +class TweetFetcher implements TweetFetcherInterface {
   

   Class docblock missing.

3. +++ b/src/TweetFetcher.php
   @@ -0,0 +1,91 @@
   +    // Assume credentials have been set and query Twitter's API.
   +    $response = $this->twitter
   

   $this->twitter may not be initialized at this point.

4. +++ b/src/TweetFetcher.php
   @@ -0,0 +1,91 @@
   +    // TODO: Check for errors in the response and throw errors if there are any.
   

   Any ideas for checks? If they are straightforward could we add then now?

5. +++ b/src/TweetFetcher.php
   @@ -0,0 +1,91 @@
   +    // If we have a cache, store the response for future use.
   +    if ($this->cache) {
   +      $this->cache->set($id, $response);
   +    }
   

   Set a lifetime?

6. +++ b/src/TweetFetcherInterface.php
   @@ -0,0 +1,47 @@
   +   * @throws \Exception
   

   Would be nice to have custom exception class.

7. +++ b/tests/src/Kernel/ThumbnailTest.php
   @@ -0,0 +1,177 @@
   +/**
   + * @group media_entity_twitter
   + */
   +class ThumbnailTest extends KernelTestBase {
   

   Description missing.

8. +++ b/tests/src/Kernel/ThumbnailTest.php
   @@ -0,0 +1,177 @@
   +    $uri = 'public://twitter-thumbnails/12345.ico';
   +    touch($uri);
   

   It seems that this assumes entity id. Is it safe to do that (usually not)?

9. +++ b/src/Plugin/MediaEntity/Type/Twitter.php
   @@ -280,6 +322,40 @@ class Twitter extends MediaTypeBase {
   +    // Ensure the destination directory is writable. (TODO: Throw an exception
   +    // or otherwise take action if it isn't.)
   

   Let's do that :)

Fixed a nitpik. Should be ready to go.