Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

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

Closed
wants to merge 6 commits into from
Closed

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

wants to merge 6 commits into from

Conversation

poisa
Copy link
Contributor

@poisa poisa commented Feb 10, 2013

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

…ace. Also made set() trigger a set.post event which makes it possible for the user to know what the saved file name is.
@marc-mabe
Copy link
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.

@poisa
Copy link
Contributor Author

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;
});

@marc-mabe
Copy link
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.

…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.
@poisa
Copy link
Contributor Author

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.

@marc-mabe
Copy link
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()

@poisa
Copy link
Contributor Author

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.

@marc-mabe
Copy link
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;
}

…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
@poisa
Copy link
Contributor Author

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!

@marc-mabe
Copy link
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

@poisa
Copy link
Contributor Author

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!

@ghost ghost assigned Maks3w and weierophinney Feb 15, 2013
@weierophinney
Copy link
Member

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

weierophinney added a commit that referenced this pull request Feb 19, 2013
@weierophinney
Copy link
Member

Merged to develop.

weierophinney added a commit to zendframework/zend-cache that referenced this pull request May 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants