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

Ancestor and Inherited functions for Page & Pages #1362

Merged
merged 8 commits into from
Apr 10, 2017

Conversation

coder4life
Copy link
Contributor

Addes both helper methods to Grav\Common\Page and Grav instance utility functions to Grav\Common\Pages.

Addes both helper methods to `Grav\Common\Page` and Grav instance utility functions to `Grav\Common\Pages`.
@coder4life
Copy link
Contributor Author

coder4life commented Mar 19, 2017

Summary

From within Twig or PHP Class allow the query of ancestor Page objects and also inheriting ancestor fields.

Problem

Currently there is no efficient default function to traverse upwards the tree visiting an ancestor.
This can be especially difficult in an iterative driven environment like Twig. To add an iterative type function would need to be created anyway, because there is no way in Twig to traverse from the current Page object to the ancestor Page object . To be even more specific, we are not dealing with a children or descendant (aka Collection) Page objects, rather a pointer relationship to a parent of a parent (aka ancestor) Page object.

As seen in this image below, using the child and parent relationships shown in red, we are also likely to be checking for various properties within the page items themselves to make sure the loops execute correctly.

relation

Twig as a template language enforces recursive type functions are purposely not permitted. The logic behind this is, well pun unintended, business logic should remain outside the template render itself. One might say this is possible with Twig macros, although on closer examination these Twig functions actually cannot work with variables as return values only. Any variables will eventually have to be returned as part of the the output buffer inline with other generated content.

With no solution, it made sense to try to approach this concept differently and more directly in Grav as part of the business logic in relevant classes. I opted for Grav itself rather then a plugin, because it offers a two function query like interface that other plugins could benefit from.

Purposal

So as stated above, it is easy enough to have a forward iterative approach to go from parent to child to and to next child and so on to render items in Grav with Twig. When required to discover something about a ancestor Page object however, it was currently difficult without custom query logic.

Two new functions ancestor and inherited have been added to both \Grav\Common\Page and \Grav\Common\Pages.

  • ancestor - The public ancestor function has two parameter parts. The first parameter $url uses a url page path (it actually does not need to be the current page path but recommended) to start from. The second parameter $path is the conditional parameter, path to stop at. Essentially it recursively traverses to each parent page looking for a matching path string. If it fails it will terminate with a null return, useful in conditional Twig decisions.

  • inherited - The public inherited function has two parameter parts. The first parameter $url uses a url page path (it actually does not need to be the current page path but recommended) to start from. The second parameter $field is the conditional parameter, looking for a matching field. Essentially it recursively traverses to each parent page looking for a matching path string. If it fails it will terminate with a null return, useful in conditional Twig decisions. Note this is extremely flexible to essentially create container pages with global like blueprint options. Although the search is bottom-up (start from child to the parent or null), the inheritance occurs top-bottom. What this means fields found in a first occurrence parent will create the base structure. Fields will be replaced if the same field structure occurs in the current page.
    NOTE: The functions are duplicated in \Grav\Common\Page and \Grav\Common\Pages. This was intentional decision due to the design of those classes. The functions in \Grav\Common\Page are just helper type functions. In \Grav\Common\Page Pages are related to the Grav instance and the entire Pages tree structure.

Usecase

Grav already has a useful parent to child traversal methods. Calling relevant functions in template languages Twig is a simple process to visit each child Page object top-bottom in the tree. Also looking for a field criteria in descendant of current Page object (parents children of children of etc.) is relatively easy in an iterative approach. However what if you wanted to create a top level field definition to inherit from regardless of upward page level. Currently this is difficult to reach a specific ancestor of current Page object.

As seen in the image below a field structure property would have to be added to each page type individually. Pages contain YAML storage parameters setting per page type.

individual

Color color.md might have a YAML field definition that configures the child elements bellow or inserts an item to render at that page level. This is a very wasteful use of storage definition, as the data structure has to be duplicated across all Page object type. Need to update the Blueprint definition, this can be prone to error because each page needs to be updated. The situation can become even more complicated in a scenario when you have parent to descendant relationship with various nested depths, meaning Page objects are not siblings with each other. Each branch of the tree has a different path, and likely each Page branch could have varying heights, although they share the same Page object type.

The image below attempts to solve this issue by allowing a specialized function, in this case public function inherited to look in its parents for a container Page object.

inherit

This can be useful in Twig because you have a way to query an ancestor Page object to see if a field definition occurs. It is also ok if an ancestor field definition does not exist, that would be handle on the current Page object render anyway.

The image below also shows another usecase when overriding.

override

What is even more powerful we can use the current Page object to override any discovered field definition.

NOTE: There is one caveat is this returns first occurrence. Although anything more I feel this is out of scope of this implementation and adds complication on merging multiple sets of field definitions up the chain. Ideally this would latter need to be a separate function which can combine ancestors of ancestors, but will be withheld to keep the current implementation simpler. Might be something worth investigating for Grav 2.0.

Performance

Performance should be negligible. Due to the fact the algorithm behaves in a bottom-up fashion, the tree depth issues that can create memory problems should be minimal. Unless of course you have a tree depth 100s of nodes deep the algorithm must travel upwards, but one would think there are bigger problems with your website otherwise.

Conclusion

The implementation will need a quick look over to catch any potential edge cases, and has been tested in a proof of concept implementation.

@coder4life coder4life changed the title Ancestor and inherited functions for Page & Pages Ancestor and Inherited functions for Page & Pages Mar 19, 2017
@coder4life
Copy link
Contributor Author

Changed a few functions.

public function inherited modifies the current page with the new parameters and returns the original parent object.

public function inheritedFieldmaintains the old behavior.

@coder4life coder4life changed the title Ancestor and Inherited functions for Page & Pages [WIP] Ancestor and Inherited functions for Page & Pages Mar 20, 2017
@coder4life coder4life changed the title [WIP] Ancestor and Inherited functions for Page & Pages Ancestor and Inherited functions for Page & Pages Mar 20, 2017
Copy link
Member

@rhukster rhukster left a comment

Choose a reason for hiding this comment

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

Methods in the Page object should be specific to the current Page, and not accept other arbitrary page route value. $url should be the $this->route() for Page-based methods.

It makes sense that Pages can take an arbitrary $url (BTW this really this should be called '$route' as this is what is looked up), as Pages is the global Page-manager object.

@coder4life
Copy link
Contributor Author

coder4life commented Mar 24, 2017

"Methods in the Page object should be specific to the current Page, and not accept other arbitrary page route value. $url should be the $this->route() for Page-based methods."

I can see why but wondering why find breaks this convention. I am guessing because you just lookup a page.

"It makes sense that Pages can take an arbitrary $url (BTW this really this should be called '$route' as this is what is looked up), as Pages is the global Page-manager object."

To clarify do you just want a variable name change.

Made changes to act on current Page object only for the Page functions

Changed variable name to better reflect the intention of the passed param for the Pages class functions
@coder4life
Copy link
Contributor Author

coder4life commented Mar 24, 2017

Made some changes. Let me know if those suffice.

Copy link
Member

@rhukster rhukster left a comment

Choose a reason for hiding this comment

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

There's code replication in Page methods inheritedField and inherited. Seems inherited could call inheritedField before modifyHeader. This would be much cleaner and no code copying.

Ancestor and inherited in Pages suffer from similar issue. Lots of code copying here that could be optimized.

@rhukster
Copy link
Member

Btw the changes based on my first comments are fine.

@coder4life
Copy link
Contributor Author

coder4life commented Mar 24, 2017

inheritedField returns only the field array merged with the current Page object fields. It does not actually modify the current Page object header, but allows you to returned a combined data set.

While inherited actually returns the ancestor Page object you inherited from while still modifying the header of the current page object. Reason being is you may want to use the Ancestor object for more information, but still specify a criteria field to check against.

The two methods do two different things. However I think I could simplify the repetition.

As for pages, not sure how to clean it up. Yes the route code sections are the same, however the criteria conditionals in the if statements are different. As such not sure if we really can clean stuff up there.

@rhukster
Copy link
Member

This is untested, just edited on the fly:

    /**
     * Helper method to return an ancestor page to inherit from. The current
     * page object is returned.
     *
     * @param string   $field Name of the parent folder
     *
     * @return Page
     */
    public function inherited($field)
    {
        list($inherited, $currentParams) = $this->getInheritedParams($field);
        $this->modifyHeader($field, $currentParams);
        
        return $inherited;
    }
    /**
     * Helper method to return an ancestor field only to inherit from. The
     * first occurrence of an ancestor field will be returned if at all.
     *
     * @param string   $field Name of the parent folder
     *
     * @return array
     */
    public function inheritedField($field)
    {
        list($inherited, $currentParams) = $this->getInheritedParams($field)

        return $currentParams;
    }

    /**
     * Method that contains shared logic for inherited() and inheritedField()
     * 
     * @param string   $field Name of the parent folder
     * 
     * @return array
     */
    protected function getInheritedParams($field)
    {
        $pages = Grav::instance()['pages'];
        $inherited = $pages->inherited($this->route, $field);
        $inheritedParams = (array) $inherited->value('header.' . $field);
        $currentParams = (array) $this->value('header.' . $field);
        if($inheritedParams && is_array($inheritedParams)) {
            $currentParams = array_replace_recursive($inheritedParams, $currentParams);
        }
        return [$inherited, $currentParams];
    }

Copy link
Member

@rhukster rhukster left a comment

Choose a reason for hiding this comment

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

Can you do similar in Pages ?

@rhukster
Copy link
Member

I want to do a little cleanup on this, but at this point, it's probably easier for me to merge and modify.

@rhukster rhukster merged commit 082d4e3 into getgrav:develop Apr 10, 2017
@rhukster
Copy link
Member

BTW some CLI tests for these would be appreciated so we know if they work as intended :)

@coder4life
Copy link
Contributor Author

Sure thing! Been a bit under the weather, and have to look into the test stuff a bit. I will do a separate pull request

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.

2 participants