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.

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

Enables to override page cache adapters #2492

Merged
merged 1 commit into from May 29, 2015
Merged

Enables to override page cache adapters #2492

merged 1 commit into from May 29, 2015

Conversation

hissy
Copy link
Contributor

@hissy hissy commented May 22, 2015

Currently, page cache adapters will be loaded from concrete directory only. This change enables to add another adapters for developers.

Here is an example to use APC cache adapter.

application/config/concrete.php

<?php
return array(
    'cache' => array(
        'page' => array(
            'adapter' => 'apc',
            'ttl' => 3600,
            'namespace' => 'concrete5fullpagecache'
        )
    )
);

application/src/Cache/Page/ApcPageCache.php

<?php
namespace Application\Src\Cache\Page;

use Concrete\Core\Cache\Page\PageCache;
use Concrete\Core\Cache\Page\PageCacheRecord;
use Stash\Driver\Apc;
use Stash\Pool;
use Page as ConcretePage;
use Config;

class ApcPageCache extends PageCache
{
    static $pool;

    public function __construct()
    {
        $driver = new Apc();
        $options = array(
            'ttl'       => Config::get('concrete.cache.page.ttl'),
            'namespace' => Config::get('concrete.cache.page.namespace')
        );
        $driver->setOptions($options);

        self::$pool = new Pool($driver);
    }

    public function getRecord($mixed)
    {
        $key = $this->getCacheKey($mixed);
        if ($key) {
            $item = self::$pool->getItem($key);
            return $item->get();
        }
    }

    public function set(ConcretePage $c, $content)
    {
        $key = $this->getCacheKey($c);
        if ($key && $content) {
            $item = self::$pool->getItem($key);

            // Let other processes know that this one is rebuilding the data.
            $item->lock();

            $lifetime = $c->getCollectionFullPageCachingLifetimeValue();
            $response = new PageCacheRecord($c, $content, $lifetime);
            $item->set($response);
        }
    }

    public function purgeByRecord(\Concrete\Core\Cache\Page\PageCacheRecord $rec)
    {
        $key = $this->getCacheKey($rec);
        if ($key) {
            $item = self::$pool->getItem($key);
            $item->clear();
        }
    }

    public function purge(ConcretePage $c)
    {
        $key = $this->getCacheKey($c);
        if ($key) {
            $item = self::$pool->getItem($key);
            $item->clear();
        }
    }

    public function flush()
    {
        self::$pool->flush();
    }
}

@hissy hissy changed the title Enables to override cache adapters Enables to override page cache adapters May 22, 2015
@joe-meyer
Copy link
Contributor

The PR seems like a very reasonable thing to want to do, but I can't help but feel like we shouldn't be moving this stuff towards what we already have in stash rather than creating a new class for each different adapter for page caching.

Honestly I think that pages would be a great fit for the "expensive" cache level (possibly with the option to change the cache "level" for pages to whatever was configured, and then if someone wanted to have it leverage something such as APC it would be as simple as adding it into the config drivers array ahead of the file system. Then we'd also get the benefit of having the hierarchical caching, which provides a huge benefit for people who might be running super large websites that don't completely fit into apc cache, but could be loaded from disk as needed rather than having to regenerate the page every time it falls out of memory cache.

@joe-meyer
Copy link
Contributor

I guess what I'm getting at, is is there any real reason to have it such that the PageCache doesn't simply point to a pool (aka level) which could be something that's configurable? Then we could likely do away with creating a new class for each different kind of driver adapter we end up using and probably even make things more configurable from the dashboard in time if we wanted to rather than it becoming largely a programming chore.

@aembler
Copy link
Member

aembler commented May 29, 2015

@MeyerJL historically page cache adapters have been drastically different than the standard cache. For example, in 5.6 the only non-file-based page cache adapter that we have currently is Varnish – which I don't think really fits in the Stash model.

aembler added a commit that referenced this pull request May 29, 2015
Enables to override page cache adapters
@aembler aembler merged commit d2c5af6 into concretecms:develop May 29, 2015
@hissy hissy deleted the enhancement/cache_adapter branch May 29, 2015 22:37
@KorvinSzanto
Copy link
Member

@aembler can you elaborate on the varnish not fitting into stash's model thing? Looking at the stash documentation it doesn't seem like the driver interface is much different than our current cache type interface.

@aembler
Copy link
Member

aembler commented Jul 31, 2015

I guess I'm not really sure how it would work in the Stash model because these things aren't really going into Stash like any other objects. For example, when you purge a page from the full page cache and you're using the Varnish backend, it actually communicates out to perhaps a completely separate server and runs a request against the Varnish API. I think that's why we have the custom adapters.

Not to say that the original purpose of this issue isn't valid, because I'm sure it is – we haven't ported the Varnish cache to 5.7 yet so we haven't really come into contact with additional drivers yet.

I just see them as two fundamentally different things – that's not to say that in certain cases you might not leverage Stash to do the storing of these page objects (as in the APC example above, or in other cases where you're using some kind of system for which Stash already has a driver.)

@joe-meyer
Copy link
Contributor

@aembler I'll try and work up a vagrant driver to work with the page cache pr I put up. Conceptually I have some ideas about how this would work (probably very similar to how the stash memcached driver works with some tweaks on checking the cached classes). I think a good proof a concept would go a long way towards building some confidence in this model.

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

Successfully merging this pull request may close these issues.

None yet

4 participants