From 229463547d2eb1d6999dc7bce60d566717711d94 Mon Sep 17 00:00:00 2001 From: Dylan Wagstaff Date: Mon, 11 Dec 2017 17:20:00 +1300 Subject: [PATCH] Edit updated things until the tests pass --- _config/versionfeed.yml | 24 +++++---- docs/en/developer.md | 2 +- src/Filters/CachedContentFilter.php | 2 +- src/Filters/ContentFilter.php | 8 +-- src/Filters/RateLimitFilter.php | 14 +++--- src/VersionFeed.php | 20 ++------ src/VersionFeedController.php | 2 +- tests/VersionFeedFunctionalTest.php | 77 +++++++++++++++-------------- tests/VersionFeedTest.php | 18 ++++--- 9 files changed, 84 insertions(+), 83 deletions(-) diff --git a/_config/versionfeed.yml b/_config/versionfeed.yml index d775357..66932c4 100644 --- a/_config/versionfeed.yml +++ b/_config/versionfeed.yml @@ -1,21 +1,25 @@ --- Name: versionedfeedconfig --- -Injector: - RateLimitFilter: \VersionFeed\Filters\RateLimitFilter +SilverStripe\Core\Injector\Injector: + RateLimitFilter: SilverStripe\VersionFeed\Filters\RateLimitFilter ContentFilter: - class: \VersionFeed\Filters\CachedContentFilter + class: SilverStripe\VersionFeed\Filters\CachedContentFilter constructor: - %$RateLimitFilter -SiteTree: + Psr\SimpleCache\CacheInterface.VersionFeedController: + factory: SilverStripe\Core\Cache\CacheFactory + constructor: + namespace: 'VersionFeedController' +SilverStripe\CMS\Model\SiteTree: extensions: - - VersionFeed -SiteConfig: + - SilverStripe\VersionFeed\VersionFeed +SilverStripe\SiteConfig\SiteConfig: extensions: - - VersionFeedSiteConfig -ContentController: + - SilverStripe\VersionFeed\VersionFeedSiteConfig +SilverStripe\CMS\Controllers\ContentController: extensions: - - VersionFeed_Controller -VersionFeed_Controller: + - SilverStripe\VersionFeed\VersionFeedController +SilverStripe\VersionFeed\VersionFeedController: dependencies: ContentFilter: %$ContentFilter diff --git a/docs/en/developer.md b/docs/en/developer.md index 645b41b..0203089 100644 --- a/docs/en/developer.md +++ b/docs/en/developer.md @@ -42,7 +42,7 @@ Two filters are applied on top of one another: server. This filter will only be applied if the `CachedContentFilter` does not have any cached record for a request. -Either one of these can be replaced, added to, or removed, by adjusting the `VersionFeed_Controller.dependencies` +Either one of these can be replaced, added to, or removed, by adjusting the `SilverStripe\VersionFeed\VersionFeedController.dependencies` config to point to a replacement (or no) filter. For smaller servers where it's reasonable to apply a strict approach to rate limiting the default diff --git a/src/Filters/CachedContentFilter.php b/src/Filters/CachedContentFilter.php index acfb21f..dc9d0af 100644 --- a/src/Filters/CachedContentFilter.php +++ b/src/Filters/CachedContentFilter.php @@ -34,7 +34,7 @@ class CachedContentFilter extends ContentFilter { // Fallback to generate result $result = parent::getContent($key, $callback); $lifetime = Config::inst()->get(ContentFilter::class, 'cache_lifetime') ?: null; - $cache->set($result, $key, $lifetime); + $cache->set($key, $result, $lifetime); return $result; } } diff --git a/src/Filters/ContentFilter.php b/src/Filters/ContentFilter.php index e43edda..e2339bd 100644 --- a/src/Filters/ContentFilter.php +++ b/src/Filters/ContentFilter.php @@ -2,10 +2,8 @@ namespace SilverStripe\VersionFeed\Filters; -use SS_Cache; - use SilverStripe\VersionFeed\VersionFeedController; -use SilverStripe\Core\Config\Config; +use SilverStripe\Core\Config\Configurable; use Psr\SimpleCache\CacheInterface; use SilverStripe\Core\Injector\Injector; @@ -16,6 +14,8 @@ use SilverStripe\Core\Injector\Injector; * of its execution. */ abstract class ContentFilter { + + use configurable; /** * Nested content filter @@ -43,7 +43,7 @@ abstract class ContentFilter { */ protected function getCache() { return Injector::inst()->get( - CacheInterface::class . '.' . VersionFeedController::class + CacheInterface::class . '.VersionFeedController' ); } diff --git a/src/Filters/RateLimitFilter.php b/src/Filters/RateLimitFilter.php index c03be35..8b2c536 100644 --- a/src/Filters/RateLimitFilter.php +++ b/src/Filters/RateLimitFilter.php @@ -70,12 +70,12 @@ class RateLimitFilter extends ContentFilter { $key = self::CACHE_PREFIX; // Add global identifier - if(Config::inst()->get(get_class(), 'lock_bypage')) { + if($this->config()->get('lock_bypage')) { $key .= '_' . md5($itemkey); } // Add user-specific identifier - if(Config::inst()->get(get_class(), 'lock_byuserip') && Controller::has_curr()) { + if($this->config()->get('lock_byuserip') && Controller::has_curr()) { $ip = Controller::curr()->getRequest()->getIP(); $key .= '_' . md5($ip); } @@ -86,7 +86,7 @@ class RateLimitFilter extends ContentFilter { public function getContent($key, $callback) { // Bypass rate limiting if flushing, or timeout isn't set - $timeout = Config::inst()->get(get_class(), 'lock_timeout'); + $timeout = $this->config()->get('lock_timeout'); if(isset($_GET['flush']) || !$timeout) { return parent::getContent($key, $callback); } @@ -106,18 +106,18 @@ class RateLimitFilter extends ContentFilter { $lifetime = Config::inst()->get(ContentFilter::class, 'cache_lifetime') ?: null; // Apply rate limit - $cache->set(time() + $timeout, $limitKey, $lifetime); + $cache->set($limitKey, time() + $timeout, $lifetime); // Generate results $result = parent::getContent($key, $callback); // Reset rate limit with optional cooldown - if($cooldown = Config::inst()->get(get_class(), 'lock_cooldown')) { + if($cooldown = $this->config()->get('lock_cooldown')) { // Set cooldown on successful query execution - $cache->set(time() + $cooldown, $limitKey, $lifetime); + $cache->set($limitKey, time() + $cooldown, $lifetime); } else { // Without cooldown simply disable lock - $cache->remove($limitKey); + $cache->delete($limitKey); } return $result; } diff --git a/src/VersionFeed.php b/src/VersionFeed.php index 3c71eb1..2ad0c46 100644 --- a/src/VersionFeed.php +++ b/src/VersionFeed.php @@ -2,16 +2,8 @@ namespace SilverStripe\VersionFeed; - - -use Diff; -use HTMLText; - - - - - - +use SilverStripe\ORM\FieldType\DBField; +use SilverStripe\View\Parsers\Diff; use SilverStripe\ORM\ArrayList; use SilverStripe\Forms\FieldList; use SilverStripe\Core\Config\Config; @@ -21,8 +13,6 @@ use SilverStripe\Forms\LiteralField; use SilverStripe\SiteConfig\SiteConfig; use SilverStripe\CMS\Model\SiteTreeExtension; - - class VersionFeed extends SiteTreeExtension { private static $db = array( @@ -103,7 +93,7 @@ class VersionFeed extends SiteTreeExtension { if ($version->Title != $previous->Title) { $diffTitle = Diff::compareHTML($version->Title, $previous->Title); - $version->DiffTitle = new HTMLText(); + $version->DiffTitle = DBField::create_field('HTMLText', null); $version->DiffTitle->setValue( sprintf( '
%s ' . $diffTitle . '
', @@ -116,7 +106,7 @@ class VersionFeed extends SiteTreeExtension { if ($version->Content != $previous->Content) { $diffContent = Diff::compareHTML($version->Content, $previous->Content); - $version->DiffContent = new HTMLText(); + $version->DiffContent = DBField::create_field('HTMLText', null); $version->DiffContent->setValue('
'.$diffContent.'
'); $changed = true; } @@ -140,7 +130,7 @@ class VersionFeed extends SiteTreeExtension { // a diff on the initial version we will just get that version, verbatim. if ($previous && $versions->count()<$qLimit) { $first = clone($previous); - $first->DiffContent = new HTMLText(); + $first->DiffContent = DBField::create_field('HTMLText', null); $first->DiffContent->setValue('
' . $first->Content . '
'); // Copy the link so it can be cached. $first->GeneratedLink = $first->AbsoluteLink(); diff --git a/src/VersionFeedController.php b/src/VersionFeedController.php index 6b6a0b4..55c92f8 100644 --- a/src/VersionFeedController.php +++ b/src/VersionFeedController.php @@ -16,7 +16,7 @@ use SilverStripe\View\Requirements; use SilverStripe\Core\Extension; use SilverStripe\VersionFeed\Filters\ContentFilter; -class VersionFeed_Controller extends Extension { +class VersionFeedController extends Extension { private static $allowed_actions = array( 'changes', diff --git a/tests/VersionFeedFunctionalTest.php b/tests/VersionFeedFunctionalTest.php index e9a7344..bbbeed3 100644 --- a/tests/VersionFeedFunctionalTest.php +++ b/tests/VersionFeedFunctionalTest.php @@ -14,45 +14,50 @@ use SilverStripe\CMS\Model\SiteTree; use SilverStripe\Versioned\Versioned; use SilverStripe\SiteConfig\SiteConfig; use SilverStripe\Dev\FunctionalTest; +use SilverStripe\Control\Director; +use Psr\SimpleCache\CacheInterface; class VersionFeedFunctionalTest extends FunctionalTest { - protected $usesDatabase = true; + protected $usesDatabase = true; + + protected $baseURI = 'http://www.fakesite.test'; - protected $required_extensions = array( + protected static $required_extensions = array( 'Page' => array(VersionFeed::class), - 'Page_Controller' => array(VersionFeedController::class), + 'PageController' => array(VersionFeedController::class), ); protected $userIP; - public function setUp() { + protected function setUp() { + Director::config()->set('alternate_base_url', $this->baseURI); + parent::setUp(); $cache = Injector::inst()->get( - CacheInterface::class . '.' . VersionFeedController::class + CacheInterface::class . '.VersionFeedController' ); $cache->clear(); - $this->userIP = isset($_SERVER['HTTP_CLIENT_IP']) ? $_SERVER['HTTP_CLIENT_IP'] : null; + $this->userIP = isset($_SERVER['REMOTE_ADDR']) ? $_SERVER['REMOTE_ADDR'] : null; - Config::nest(); // Enable history by default - Config::inst()->update(VersionFeed::class, 'changes_enabled', true); - Config::inst()->update(VersionFeed::class, 'allchanges_enabled', true); + Config::modify()->set(VersionFeed::class, 'changes_enabled', true); + Config::modify()->set(VersionFeed::class, 'allchanges_enabled', true); // Disable caching and locking by default - Config::inst()->update(CachedContentFilter::class, 'cache_enabled', false); - Config::inst()->update(RateLimitFilter::class, 'lock_timeout', 0); - Config::inst()->update(RateLimitFilter::class, 'lock_bypage', false); - Config::inst()->update(RateLimitFilter::class, 'lock_byuserip', false); - Config::inst()->update(RateLimitFilter::class, 'lock_cooldown', false); + Config::modify()->set(CachedContentFilter::class, 'cache_enabled', false); + Config::modify()->set(RateLimitFilter::class, 'lock_timeout', 0); + Config::modify()->set(RateLimitFilter::class, 'lock_bypage', false); + Config::modify()->set(RateLimitFilter::class, 'lock_byuserip', false); + Config::modify()->set(RateLimitFilter::class, 'lock_cooldown', false); } public function tearDown() { - Config::unnest(); - - $_SERVER['HTTP_CLIENT_IP'] = $this->userIP; + Director::config()->set('alternate_base_url', null); + + $_SERVER['REMOTE_ADDR'] = $this->userIP; parent::tearDown(); } @@ -83,18 +88,18 @@ class VersionFeedFunctionalTest extends FunctionalTest { public function testRateLimiting() { // Re-enable locking just for this test - Config::inst()->update(RateLimitFilter::class, 'lock_timeout', 20); - Config::inst()->update(CachedContentFilter::class, 'cache_enabled', true); + Config::modify()->set(RateLimitFilter::class, 'lock_timeout', 20); + Config::modify()->set(CachedContentFilter::class, 'cache_enabled', true); $page1 = $this->createPageWithChanges(array('PublicHistory' => true, 'Title' => 'Page1')); $page2 = $this->createPageWithChanges(array('PublicHistory' => true, 'Title' => 'Page2')); // Artifically set cache lock - Config::inst()->update(RateLimitFilter::class, 'lock_byuserip', false); + Config::modify()->set(RateLimitFilter::class, 'lock_byuserip', false); $cache = Injector::inst()->get( - CacheInterface::class . '.' . VersionFeedController::class + CacheInterface::class . '.VersionFeedController' ); - $cache->set(time() + 10, \VersionFeed\Filters\RateLimitFilter::CACHE_PREFIX); + $cache->set(RateLimitFilter::CACHE_PREFIX, time() + 10); // Test normal hit $response = $this->get($page1->RelativeLink('changes')); @@ -105,39 +110,39 @@ class VersionFeedFunctionalTest extends FunctionalTest { $this->assertGreaterThan(0, $response->getHeader('Retry-After')); // Test page specific lock - Config::inst()->update(RateLimitFilter::class, 'lock_bypage', true); + Config::modify()->set(RateLimitFilter::class, 'lock_bypage', true); $key = implode('_', array( 'changes', $page1->ID, Versioned::get_versionnumber_by_stage(SiteTree::class, 'Live', $page1->ID, false) )); - $key = \VersionFeed\Filters\RateLimitFilter::CACHE_PREFIX . '_' . md5($key); - $cache->set(time() + 10, $key); + $key = RateLimitFilter::CACHE_PREFIX . '_' . md5($key); + $cache->set($key, time() + 10); $response = $this->get($page1->RelativeLink('changes')); $this->assertEquals(429, $response->getStatusCode()); $this->assertGreaterThan(0, $response->getHeader('Retry-After')); $response = $this->get($page2->RelativeLink('changes')); $this->assertEquals(200, $response->getStatusCode()); - Config::inst()->update(RateLimitFilter::class, 'lock_bypage', false); + Config::modify()->set(RateLimitFilter::class, 'lock_bypage', false); // Test rate limit hit by IP - Config::inst()->update(RateLimitFilter::class, 'lock_byuserip', true); - $_SERVER['HTTP_CLIENT_IP'] = '127.0.0.1'; - $cache->set(time() + 10, \VersionFeed\Filters\RateLimitFilter::CACHE_PREFIX . '_' . md5('127.0.0.1')); + Config::modify()->set(RateLimitFilter::class, 'lock_byuserip', true); + $_SERVER['REMOTE_ADDR'] = '127.0.0.1'; + $cache->set(RateLimitFilter::CACHE_PREFIX . '_' . md5('127.0.0.1'), time() + 10); $response = $this->get($page1->RelativeLink('changes')); $this->assertEquals(429, $response->getStatusCode()); $this->assertGreaterThan(0, $response->getHeader('Retry-After')); // Test rate limit doesn't hit other IP - $_SERVER['HTTP_CLIENT_IP'] = '127.0.0.20'; - $cache->set(time() + 10, \VersionFeed\Filters\RateLimitFilter::CACHE_PREFIX . '_' . md5('127.0.0.1')); + $_SERVER['REMOTE_ADDR'] = '127.0.0.20'; + $cache->set(RateLimitFilter::CACHE_PREFIX . '_' . md5('127.0.0.1'), time() + 10); $response = $this->get($page1->RelativeLink('changes')); $this->assertEquals(200, $response->getStatusCode()); // Restore setting - Config::inst()->update(RateLimitFilter::class, 'lock_byuserip', false); - Config::inst()->update(RateLimitFilter::class, 'lock_timeout', 0); - Config::inst()->update(CachedContentFilter::class, 'cache_enabled', false); + Config::modify()->set(RateLimitFilter::class, 'lock_byuserip', false); + Config::modify()->set(RateLimitFilter::class, 'lock_timeout', 0); + Config::modify()->set(CachedContentFilter::class, 'cache_enabled', false); } public function testContainsChangesForPageOnly() { @@ -215,7 +220,7 @@ class VersionFeedFunctionalTest extends FunctionalTest { // Test requests to 'changes' action foreach(array(true, false) as $publicHistory_Config) { - Config::inst()->update(VersionFeed::class, 'changes_enabled', $publicHistory_Config); + Config::modify()->set(VersionFeed::class, 'changes_enabled', $publicHistory_Config); $expectedResponse = $publicHistory_Page && $publicHistory_Config ? 200 : 404; $response = $this->get($page->RelativeLink('changes')); $this->assertEquals($expectedResponse, $response->getStatusCode()); @@ -224,7 +229,7 @@ class VersionFeedFunctionalTest extends FunctionalTest { // Test requests to 'allchanges' action on each page foreach(array(true, false) as $allChanges_Config) { foreach(array(true, false) as $allChanges_SiteConfig) { - Config::inst()->update(VersionFeed::class, 'allchanges_enabled', $allChanges_Config); + Config::modify()->set(VersionFeed::class, 'allchanges_enabled', $allChanges_Config); $siteConfig = SiteConfig::current_site_config(); $siteConfig->AllChangesEnabled = $allChanges_SiteConfig; $siteConfig->write(); diff --git a/tests/VersionFeedTest.php b/tests/VersionFeedTest.php index ef45d19..0384456 100644 --- a/tests/VersionFeedTest.php +++ b/tests/VersionFeedTest.php @@ -7,20 +7,22 @@ use Page; use SilverStripe\VersionFeed\VersionFeed; use SilverStripe\VersionFeed\VersionFeedController; use SilverStripe\Dev\SapphireTest; +use SilverStripe\CMS\Model\SiteTree; +use SilverStripe\CMS\Controllers\ContentController; class VersionFeedTest extends SapphireTest { protected $usesDatabase = true; - protected $required_extensions = array( - 'SiteTree' => array(VersionFeed::class), - 'ContentController' => array(VersionFeedController::class), - ); + protected static $required_extensions = [ + SiteTree::class => [VersionFeed::class], + ContentController::class => [VersionFeedController::class], + ]; - protected $illegalExtensions = array( - 'SiteTree' => array('Translatable') - ); + protected $illegalExtensions = [ + 'SiteTree' => ['Translatable'] + ]; public function testDiffedChangesExcludesRestrictedItems() { $this->markTestIncomplete(); @@ -31,7 +33,7 @@ class VersionFeedTest extends SapphireTest { } public function testDiffedChangesTitle() { - $page = new Page(array('Title' => 'My Title')); + $page = new Page(['Title' => 'My Title']); $page->write(); $page->publish('Stage', 'Live');