$pages->groupBy() with callback?

Kirby 2 Suggestions šŸ’”
1

First of all, thanks for fixing the groupBy method on the $pages collection with 2.2 @bastianallgeier :slight_smile:

I currently have a field only storing the year, so grouping with $page->children()->groupBy('year') works perfectly fine. However I would like to update the blueprint to store a date instead. Obviously I will remove the year field then.

Unfortunately that means that I canā€™t group by year anymore. A solution might be to let groupBy accept a callback like so:

$page->children()->groupBy(function ($child) {
    return $child->date('Y');
});

Said function would have to return the ā€œgroupā€ its argument page belongs to. I think this would be very powerful - you could group by number of subpages, number of images, ā€¦ (even though I have a hard time coming up with a use case for that). There are some drawbacks or possible pitfalls though. Most importantly the developer has to be cautious to only return values that can be used as array keys later.

I would happily send a pull request for that but wanted to hear everybodyā€™s opinion first. What do you think? Is this simple/intuitive enough to use? Is being able to group by year from a date field worth a few extra lines of code in the core? Or should this be handled in the template/model/controller with loops, pluck, ā€¦ like in the dark times before 2.2? :stuck_out_tongue:

EDIT: I just came up with another use case: my users have a field info possibly storing stuff like ā€œbossā€, ā€œour best developerā€ and so on. Additionally, I have a template with a list of all users with a given role. There, people with a specified ā€˜infoā€™ should be listed before those without as they are usually searched for more often. Right now my code is quite messy:

<?php foreach (array('!=', '==') as $comp): ?>
  <?php foreach($site->users()->filterBy('funktion', $comp, '')->sortBy('firstName', 'ASC') as $person): ?>
    <li><?= $person->firstName()</li> // Waaay more complicated in real code 
  <?php endforeach; ?>
<?php endforeach; ?>

So, first I filter out all people that have information, show their profiles, and then do the exact same thing with those without information. This works, but doesnā€™t look nice.

<?php foreach ($site->users()->groupBy(function($user) {
  return ($user->info() == "") ? "noInfo" : "info"
}) as $group: ?>
  <?php foreach ($group as $person): ?>
    <li><?= $person->firstName()</li> // Waaay more complicated in real code 
  <?php endforeach; ?>
<?php endforeach; ?>

Something like that could be an alternative.

1 Like
2

Great idea!

I would name the callback variant group() though to keep the same naming scheme as with filterBy/filter. Also this can be part of the Collection class so it can be used everywhere.

1 Like
3

A nice new feature for v2.3. When is that supposed to be released :joy:

4

That would probably need two different implementations as the groupBy method does (one general for collections, one for pages).

5

Maybe it helps to brainstorm to find a nice, lean way.

Edit: While working on it, I figured out that we might not need a different implementation for the Pages class, just this one for collections

public function group($callback) {
    $groups = array();

    foreach($this->data as $key => $item) {
      $value = call_user_func($callback, $item);

      if(!isset($groups[$value])) {
        // create a new entry for the group if it does not exist yet
        $groups[$value] = new static(array($key => $item));
      } else {
        // add the item to an existing group
        $groups[$value]->set($key, $item);
      }

    }

    return new static($groups);
  }

Improvements, comments are welcome. Will try to test it later today.

Edit2: Maybe also adding these tests:

// make sure that there's always a proper value to group by
      if(!$value) throw new Exception('Invalid grouping value for key: ' . $key);

      // make sure we have a proper key for each group
      if(is_array($value)) {
        throw new Exception('You cannot group by arrays or objects');
      } else if(is_object($value)) {
        if(!method_exists($value, '__toString')) {
          throw new Exception('You cannot group by arrays or objects');
        } else {
          $value = (string)$value;
        }
      }

      // ignore upper/lowercase for group names
      if($i) $value = str::lower($value);

And making the groupBy method just use the new group method:

public function groupBy($field, $i = true) {
    return $this->group(function($item) use($field) {
      return $this->extractValue($item, $field);
    }, $i);
}
1 Like
6

I came up with something similar:

// collection.php

  /**
   * Groups the collection by a given callback
   *
   * @param callable $callback
   * @return object A new collection with an item for each group and a subcollection in each group
   */
  public function group($callback) {

    $groups = array();

    foreach($this->data as $key => $item) {

      // get the value to group by
      $value = call_user_func($callback, $item);

      // make sure that there's always a proper value to group by
      if(!$value) throw new Exception('Invalid grouping value for key: ' . $key);

      // make sure we have a proper key for each group
      if(is_array($value)) {
        throw new Exception('You cannot group by arrays or objects');
      } else if(is_object($value)) {
        if(!method_exists($value, '__toString')) {
          throw new Exception('You cannot group by arrays or objects');
        } else {
          $value = (string)$value;
        }
      }

      if(!isset($groups[$value])) {
        // create a new entry for the group if it does not exist yet
        $groups[$value] = new static(array($key => $item));
      } else {
        // add the item to an existing group
        $groups[$value]->set($key, $item);
      }

    }

    return new Collection($groups);

  }

  /**
   * Groups the collection by a given field
   *
   * @param string $field
   * @return object A new collection with an item for each group and a subcollection in each group
   */
  public function groupBy($field, $i = true) {

    return $this->group(function($item) use ($field, $i) {

      $value = $this->extractValue($item, $field);

      // ignore upper/lowercase for group names
      return ($i == true) ? str::lower($value) : $value;

    });

  }

Itā€™s more or less the same, with some little tweaks:

  • Keep the case sensitivity logic in the callback
  • Always return a Collection instead of picking the constructor with late static binding. Is there even a use case where you would need the outer collection to be of a more specific type?

However, you still need a second implementation for pages. When used in combination like so $page->children()->group(...) this method will return a collection of children, instead of a collection of pages. This is not bad per se, but leads to the first entry in each sub-collection being lost:
The constructor of Children canā€™t deal with the array properly and fails to save the data. In all subsequent runs, we use the set method. This is declared neither on Children nor on Pages, so it defaults to the Collection implementation, which obviously works then.

Multiple ways to deal with that:

  • Extend the Children constructor

abstract class ChildrenAbstract extends Pages {

  protected $page = null;
  protected $cache = array();

  /**
   * Constructor
   */
  public function __construct($page) {
    // not a page...
    // hack to make collection grouping work
    if (is_array($page)) {
      foreach ($page as $key => $value) {
        $this->set($key, $value);
      }
      return;
    }
    $this->page = $page;
  }

This looks super ugly. Is there a cleaner way (that is not resorting to an implementation in Pages)?

  • Use a second implementation in Pages with basically the same code but without late static binding and fixed types instead.
1 Like