Closed kicken closed 9 years ago
nice approach for #256 this works for twig only, maybe we could implement it on core level. So PagesCollection should be located in lib/Phile/Core
I'm :+1: on the idea and in principal I'm OKish with the implementation. But of course I'm having a small "but" and here it comes. :wink:
What's a little bit uncomfortable is to split the worker code of findByPath
and findAll
– both involving "loading from filesystem" – into different source files. They should stay together or at least not split up just for implementing lazy-loading findAll
. Imho.
I suggest we use a proxy-pattern for lazy-loading and subclass PageCollection from Repository\Page and keep findByPath
and findAll
in Repository\Page. Along those lines:
In Repository\Page:
class Page {
public function findAll(array $options = array(), $folder = CONTENT_DIR) {
return new PageProxy($options, $folder);
}
protected function _findAll(array $options = array(), $folder) {
// ... existing code
}
A $folder setter in the constructor needs to be implemented, but imho that's not bad thing to have anyway.
Then use the simple PageCollection from the PR's first commit just as proxy extending Page:
class PageProxy extends Page implements \ArrayAccess, \IteratorAggregate {
// ...
private function load() {
if ($this->pages === null) {
$this->pages = parent::_findAll($this->options, $this->folder);
}
}
With that _findAll
could also continue to call a protected getPage
. :clap:
But that's purely academical at the moment. Overall and practically a :+1: from me.
I was not thrilled with the code separation initially either. I will give the proxy method a shot, that sounds like a good approach.
I think we can run with that. :+1:
merged into develop in 62496b5f2b8a2b4a29a68efa7251fd3888e125b1
Delays scanning the content directory for pages until the pages variable is used. Speeds up loading time if the variable is not required.