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

Delay load pages #257

Closed
wants to merge 5 commits into from
Closed

Delay load pages #257

wants to merge 5 commits into from

Conversation

kicken
Copy link
Contributor

@kicken kicken commented Jun 28, 2015

Delays scanning the content directory for pages until the pages variable is used. Speeds up loading time if the variable is not required.

@NeoBlack
Copy link
Contributor

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

@@ -171,7 +119,7 @@ public function getPageOffset(\Phile\Model\Page $page, $offset = 0) {
*
* @return mixed|\Phile\Model\Page
*/
protected function getPage($filePath, $folder = CONTENT_DIR) {
public function getPage($filePath, $folder = CONTENT_DIR) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should try to keep that protected. Users are supposed to call findByPath.

@Schlaefer
Copy link
Member

I'm 👍 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. 😉

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. 👏

But that's purely academical at the moment. Overall and practically a 👍 from me.

@kicken
Copy link
Contributor Author

kicken commented Jun 30, 2015

I was not thrilled with the code separation initially either. I will give the proxy method a shot, that sounds like a good approach.

Collection class accepts a callback function which will be executed when
the pages need to be loaded.  This allows for minimal changes to the
existing repository class and keeps the search code where it belongs.
@Schlaefer
Copy link
Member

I think we can run with that. 👍

@Schlaefer Schlaefer modified the milestone: 1.6.0 Jul 8, 2015
@Schlaefer
Copy link
Member

merged into develop in 62496b5

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.

3 participants