-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
Addes both helper methods to `Grav\Common\Page` and Grav instance utility functions to `Grav\Common\Pages`.
SummaryFrom within Twig or PHP Class allow the query of ancestor Page objects and also inheriting ancestor fields. ProblemCurrently there is no efficient default function to traverse upwards the tree visiting an ancestor. 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. 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. PurposalSo 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
UsecaseGrav 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. Color The image below attempts to solve this issue by allowing a specialized function, in this case 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. 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. PerformancePerformance 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. ConclusionThe implementation will need a quick look over to catch any potential edge cases, and has been tested in a proof of concept implementation. |
Changed a few functions.
|
There was a problem hiding this 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.
…e/ancestor-lookup
"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
Made some changes. Let me know if those suffice. |
There was a problem hiding this 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.
Btw the changes based on my first comments are fine. |
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. |
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];
} |
There was a problem hiding this 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 ?
Simpilfied duplicate code. Also clarified variable naming for similar functions.
…e/ancestor-lookup
…e/ancestor-lookup
I want to do a little cleanup on this, but at this point, it's probably easier for me to merge and modify. |
BTW some CLI tests for these would be appreciated so we know if they work as intended :) |
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 |
Addes both helper methods to
Grav\Common\Page
and Grav instance utility functions toGrav\Common\Pages
.