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.

Sign up for GitHub

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

Jump to bottom

Youtube service #77

Merged
merged 4 commits into from
Apr 18, 2015
Merged

Youtube service #77

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fa856f7
Youtube service
WyriHaximus Nov 22, 2014
24f6cc0
Moved the cache TTL into a class constant
WyriHaximus Nov 24, 2014
b9da488
Using dev-master for now
WyriHaximus Apr 18, 2015
1d4a174
Factory method as suggested by @pascaldevink and @rdohms
WyriHaximus Apr 18, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/config/parameters.yml.dist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,5 @@ parameters:
meetup_key:
akismet_key:
group_urlname:
youtube_key:
youtube_playlist:
3 changes: 2 additions & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@
"guzzle/guzzle": "~3.7",
"ornicar/akismet-bundle": "*@dev",
"ornicar/gravatar-bundle": "dev-master@dev",
"antimattr/google-bundle": "dev-master@dev"
"antimattr/google-bundle": "dev-master@dev",
Copy link
Member

Choose a reason for hiding this comment

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

Apparantly, animattr tagged a couple of versions in the mean time 😀 https://packagist.org/packages/antimattr/google-bundle, can we use a stable version?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ow neat, I'll look into that 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

After looking at the package name it's a different package then what is used for youtube. Maybe it's out of scope for this PR?

"madcoda/php-youtube-api": "~1.0"
},
"require-dev": {
"phpspec/phpspec": "2.0.*@dev",
Expand Down
59 changes: 51 additions & 8 deletions composer.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 5 additions & 1 deletion src/AmsterdamPHP/Bundle/SiteBundle/Controller/DefaultController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,16 @@ public function indexAction()
$blogService = $this->get('amsterdamphp_site.integration.blog');
$posts = $blogService->getLatestBlogPosts();

$youtubeService = $this->get('amsterdamphp_site.integration.youtube');
Copy link
Member

Choose a reason for hiding this comment

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

No feedback on your code, but we should definitely introduce controllers as services for better dependency injection. That would've also been a nice hackathon ticket 😝

Copy link
Member

Choose a reason for hiding this comment

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

Controllers as services are overrated to be honest. Too much overhead for not enough benefit.

$videos = $youtubeService->getLatestVideos();

return [
'header_photo' => $headerPhoto,
'next_event' => array_shift($nextEvents),
'past_events' => array_splice($pastEvents, 0, 2),
'blog_posts' => $posts,
'sponsors' => $sponsors
'sponsors' => $sponsors,
'videos' => $videos,
];
}
}
7 changes: 7 additions & 0 deletions src/AmsterdamPHP/Bundle/SiteBundle/Resources/config/services.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,13 @@ services:
arguments:
cache: @snc_redis.cache

amsterdamphp_site.integration.youtube:
class: AmsterdamPHP\Bundle\SiteBundle\Service\YoutubeService
arguments:
cache: @snc_redis.cache
key: %youtube_key%
playlist: %youtube_playlist%

amsterdamphp_site.email.contact:
class: AmsterdamPHP\Bundle\SiteBundle\Service\ContactService
arguments:
Expand Down
57 changes: 57 additions & 0 deletions src/AmsterdamPHP/Bundle/SiteBundle/Service/YoutubeService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
<?php

namespace AmsterdamPHP\Bundle\SiteBundle\Service;

use Madcoda\Youtube;
use Predis\Client as Cache;

class YoutubeService
{
/**
* @var Client
*/
protected $client;

/**
* @var Cache
*/
protected $cache;

/**
* @var string
*/
protected $playlist;

/**
* Constructor
*/
public function __construct(Cache $cache, $key, $playlist)
{
$this->client = new Youtube(array(
Copy link
Member

Choose a reason for hiding this comment

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

Why would you create an instance of this dependency here and not in the service container? It feels kinda double now.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, I'll look into it 👍

Copy link
Member

Choose a reason for hiding this comment

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

agreed, use the factory method functionality and you should be in the clear.

'key' => $key,
));
$this->cache = $cache;
$this->playlist = $playlist;
}

/**
* Returns the latest blog posts
*
* @return mixed
*/
public function getLatestVideos()
{
$cacheKey = 'youtube.talks.latest';
$videoList = $this->cache->get($cacheKey);

if ($videoList === null){
$videoList = base64_encode(serialize($this->client->getPlaylistItemsByPlaylistId($this->playlist)));
$this->cache->set($cacheKey, $videoList);
$this->cache->expireat($cacheKey, strtotime('+5 hours'));
Copy link
Member

Choose a reason for hiding this comment

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

I think the cache can be set to something a bit further away in time. It only happens once a month at most that we post a video, so it doesn't have to be on the website immediately.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, so 24 hours or would you prefer going with a few days?

Copy link
Member

Choose a reason for hiding this comment

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

24 hours would be better, yes

Copy link
Member

Choose a reason for hiding this comment

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

Also: make it a class constant for easier tweaking and more context.

}

$videoList = unserialize(base64_decode($videoList));

return $videoList;
}
}