Add getFilename() to Zend\Cache\Pattern\CaptureCache #3747

Closed
wants to merge 6 commits into
from

Projects

None yet

4 participants

@poisa
Contributor
poisa commented Feb 10, 2013

This is particularly useful for pushing the saved file to a CDN or a message queue.

@poisa poisa Made the CaptureCache cache pattern implement EventManagerAwareInterf…
…ace. Also made set() trigger a set.post event which makes it possible for the user to know what the saved file name is.
9d415ea
Member

@poisa I don't think this is the right way to get the internal filename. I think a resolver class+interface would be a better idea.

Contributor
poisa commented Feb 12, 2013

@marc-mabe I'm not exactly sure what you mean but if you can show me an example I'll try to whip it up.

I first thought of adding a property to the class that contained the file name but since this function is run within the scope of a ob_start(), I didn't know how to get a reference to the class to set the property. See this:

$that = $this;
ob_start(function ($content) use ($that, $pageId) {
    $that->set($content, $pageId); // <-- I trigger the event inside set() where $this is not in scope
                                   // any instance properties I set here are gone when the method returns
    return false;
});
Member

@poisa OK a resolver class + interface is a little bit too much. An alternative would be to add a public method getFilename($pageId = null) and make detectPageId() public. Than you cen get the auto-generated page id and get the filename for it in a simple way.

poisa added some commits Feb 14, 2013
@poisa poisa Revert "Made the CaptureCache cache pattern implement EventManagerAwa…
…reInterface. Also made set() trigger a set.post event which makes it possible for the user to know what the saved file name is."

This reverts commit 9d415ea.
ce02f72
@poisa poisa New public method getFilename() and supporting unit tests. 2ef683a
Contributor
poisa commented Feb 14, 2013

@marc-mabe I tried your suggestion and it looks much simpler and cleaner.
I've also attempted some unit tests to support this though I didn't know how to test when $pageId = null as that would take stuff from $_SERVER['REQUEST_URI'] and I don't know who to mock that.

Member

@poisa Please return the full filename incl. public_dir else it looks good.
$_SERVER['REQUEST_URI'] is a super global so you can set it before and buffer and restore it on setUp() and tearDown()

Contributor
poisa commented Feb 14, 2013

@marc-mabe as it turns out $_SERVER['REQUEST_URI'] isn't set via CLI so I just created it and discarded it for each test. I've added the unit tests to support not passing $pageId too.

Member

@poisa Because it's not sure that the test will be run in CLI and because a failed test will return the test function before you reset the value please use setUp()+ tearDown()

protected $bufferedServerSuperGlobal;
public function setUp()
{
    $this->bufferedServerSuperGlobal = $_SERVER;
}
public function tearDown()
{
    $_SERVER = $this->bufferedServerSuperGlobal;
}
@poisa poisa Buffered $_SERVER superglobal on unit testsBuffered $_SERVER superglo…
…bal on unit testsBuffered $_SERVER superglobal on unit testsBuffered $_SERVER superglobal on unit testsBuffered $_SERVER superglobal on unit testsBuffered $_SERVER superglobal on unit testsBuffered $_SERVER superglobal on unit testsBuffered $_SERVER superglobal on unit tests
fc0ce21
Contributor
poisa commented Feb 14, 2013

@marc-mabe you are right. I didn't even consider that scenario. I've made the changes you requested. I have no idea what's up with the commit message on this one... maybe I screwed something up while typing in vim. Git won't even allow me to ammend... Sorry about that!

Member

@poisa looks good to me - please update title of PR and if nobody else has comments please rebase and it could be merged.

@Maks3w @weierophinney ping

Contributor
poisa commented Feb 15, 2013

I've updated the title of the pull request. I don't know how to rebase my branch with your own so I'm going to need a little help there!

Owner

As this adds to the API, I'm scheduling for 2.2.0 (end of April).

@weierophinney weierophinney added a commit that referenced this pull request Feb 19, 2013
@weierophinney weierophinney Merge branch 'feature/3747' into develop
Close #3747
12e18cb
Owner

Merged to develop.

@weierophinney weierophinney added a commit to zendframework/zend-cache that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'feature/3747' into develop 884bade
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment