Class-based snippets/slots (and controllers, and collections)?

[gaufde]

Hello,

I’ve been studying the starterkit and also working my way through the YouTube videos from the beginning.

Overall, I’m really impressed with how well designed Kirby is! However, I am a bit bummed that static analysis and typing seems to be quite difficult.

The custom page models feature (with DefaultPage) seems super powerful, and it is also much easier for an IDE to understand.

Have there been any discussions of doing something similar for snippets/slots, controllers, or collections?

For example, with snippets you can pass custom variables or slots and then use them inside the snippet! However, when you are calling a snippet, there is no immediate way for you or the IDE to know if you are passing variables to the snippet correctly . It seems like you would have to manually read the snippet code and decipher what it does, or keep a bunch of /** @var */ annotations perfectly up do date and then reference those. Also, the code inside the snippet won’t have any type information, so writing what the snippet does is either up to you or again dependent on proper /** @var */ annotations.

My initial concept is that if snippets were class based, then rather than calling <?php snippet('note', ['note' => $note]) ?> you would do something like <?php new NoteSnippet($note) ?>. All of the parameters you need for that snippet could be defined and typed, and now your IDE can automatically find usages, refactor parameters, statically analyze types, navigate to definition, etc.

This is as far as I’ve gotten so far with this concept. But in short, the goal would be change how things work as little as possible but make it so IDEs can fully analyze all of the code as automatically as possible to enable automatic refactoring, typing, better code navigation, etc.

I know some work is going into better typing for v5. However, as far as I can tell those changes won’t do what I am thinking about here.

• Type hinting for collections by distantnative · Pull Request #6391 · getkirby/kirby · GitHub
• [v5] Support psalm templates for typing · Issue #2312 · getkirby/getkirby.com · GitHub

It also looks like people have had some similar ideas/discussions before:

• Kirby Ink - PHP Class-based Blueprints
• Getting better code completion in your editor with Kirby
• GitHub - lukaskleinschmidt/kirby-types: IDE Helper for Kirby
• Visual Studio Code Snippets for the Kirby Cheat Sheet
• GitHub - bnomei/kirby3-schema: JSON Schema file for Kirby CMS blueprints

I haven’t seen any discussions along these lines, so I wanted to put this out there to see (1) if others agree and (2) to see if the Kirby core team has any thoughts/insights. Has anything like this been discussed before? Is this something that could eventually live in core, or would this be too many breaking changes?

[texnixe]

I don’t think functions like snippet() are really an issue, and I wouldn’t vote for instantiating classes in templates, as that is not really user-friendly. Type hinting also works for these functions, and if you prefer, you can use the “long” versions instead of those helpers, e.g.

replace

snippet('layouts', ['field' => $page->layout()]);

with

$kirby->snippet('layouts', ['field' => $page->layout()]);

You can also work around certain other problems by not using shortcuts

replace

$page->myfieldName()

with

$page->content()->get('myfieldName');

However, the main problem static analyzers complain about, are the global variables $page, $pages, $kirby, $block etc. There’s also the problem of items in a collection (but that will be taken care about in K5).

But yes, a lot can still be improved, and I’m sure will be improved. But the fact that there hasn’t been a huge discussion about this anywhere yet, is probably a sign of its priority in the community…

[gaufde]

Hi Sonja,

The work around ideas you are showing make sense to me, and I have already started using them. So, I have no real concerns there, especially since v5 will make those even better!

The thing that I’m thinking about here is a bit different. Though you might be right, it probably isn’t a concern for the community.

What I am getting at here is more about some method to make it clear what type of information a snippet depends on when you are inserting a snippet. Similarly, it would be nice to also make it clear what type of information is available to a snippet when you are writing the template for that snippet.

For example, in the starterkit, it took me a long time to discover that the note snippet has an optional boolean parameter $excerpt that can be passed to it, and that that snippet is also used used in the ‘prevnext’ snippet.

If there was some way to make these concepts explicit, then any time you make that explicit call it would be very self explanatory.

Maybe there is a better way than classes, but the idea would be to go from

snippet('slug', $data, $return, $slots)

which doesn’t necessarily tell you anything about that particular snippet, to something more akin to

// pseudocode
noteSnippet(NotePage $snippet_param1, bool $snippet_param2 = true)

This would then fully document required and optional parameters. Also, the IDE would be able to automatically index every call to noteSnippet() making it easier to traverse the code and find instances. The IDE would be able to automatically navigate to the definition of noteSnippet() and refactor parameters throughout the code base, and do all of the other nice things that are possible.

Does that make a bit more sense?

[texnixe]

@gaufde

Does that make a bit more sense?

Not really, and IMO in particular not with regards to snippets, which are supposed to be reusable independent of page, e.g. like the header snippet used in all pages, or blocks snippets, or nested snippets.

[gaufde]

@texnixe

Not really, and IMO in particular not with regards to snippets, which are supposed to be reusable independent of page, e.g. like the header snippet used in all pages, or blocks snippets, or nested snippets.

Ah, I’ll have to think about that a bit to see if it makes sense to me.

What do you think about controllers or collections? In your mind would an idea like this make any sense for those types of concepts in Kirby?

[gaufde]

I think there is still something about snippets that isn’t quite making sense to me, and I can’t tell if it is because I’m not thinking about/using them the ‘right’ way, or if I still believe that this feature could be designed differently.

For example, I am working on making a navigation menu in my header.

/snippets/header.php

<header class="primary-header">
  <h1 class="sr-only"><?= $page->title()->esc() ?></h1>

  <a href="#main" class="sr-only">Skip to content</a>
  <a href="#site-index" class="sr-only">Skip to site index</a>

  <a class="logo" href="<?= $site->url() ?>">
    <?= asset('logo.svg')->read() ?>
  </a>

  <div>
    <nav aria-label="Main Navigation">
      <ul>
        <?php
        $sub_menu_index = 1;
        foreach ($site->children()->listed() as $item):?>
          <?php
          if ($item->hasChildren()):
            $aria_controls = "main-menu-submenu-$sub_menu_index";
            $aria_labelby = "main-menu-label-$sub_menu_index";
            $sub_menu_index++;
            ?>
            <li>
              <div>
                <?php snippet('partials/menu-item', ["item" => $item, "id" => $aria_labelby]) ?>
                <button
                  type="button"
                  aria-label="Expand Submenu"
                  aria-expanded="false"
                  aria-controls="<?= $aria_controls ?>"
                  aria-labelledby="<?= $aria_labelby ?>"
                >
                  <?= asset('down-arrow.svg')->read() ?>
                </button>
              </div>
              <ul id="<?= $aria_controls ?>">
                <?php foreach ($item->children() as $subitem): ?>
                  <li>
                    <?php snippet('partials/menu-item', ["item" => $subitem]) ?>
                  </li>
                <?php endforeach; ?>
              </ul>
            </li>
          <?php else: ?>
            <li>
              <?php snippet('partials/menu-item', ["item" => $item]) ?>
            </li>
          <?php endif ?>
        <?php endforeach ?>
      </ul>
    </nav>
  </div>
</header>

Inside this snippet, you might notice that I am using another small snippet to try and keep the code DRY.

/snippets/partials/menu-item.php

<a <?php e($item->isActive(), 'aria-current="page"') ?>
  href="<?= $item->url() ?>"

  <?php if (isset($id)): ?>
    id="<?= $id ?>"
  <?php endif; ?>

>
  <?= $item->title()->esc() ?>
</a>

Now, I am already worried that menu-item.php is not very easy to use or understand since it is acting as a little helper function but it is not obvious that it has one required argument, $item, and an optional argument, $id. Also, I think that I will need to re-use my menu loop more than once, so I may need to abstract all the code further:

/snippets/partials/nav.php

<?php
/**
 * @var array<array{item: Kirby\Cms\Page, children: Kirby\Cms\Page[]}> $menu
 * @var string $menu_label
 * @var string $aria_controls
 * @var string $aria_labelby
 */
?>

<nav aria-label="<?= $menu_label ?>">
  <ul>
    <?php
    $sub_menu_index = 1;
    foreach ($menu as $menu_item):?>
      <?php
      if (!empty($menu_item["children"])):
        $aria_controls = "main-menu-submenu-$sub_menu_index";
        $aria_labelby = "main-menu-label-$sub_menu_index";
        $sub_menu_index++;
        ?>
        <li>
          <div>
            <?php snippet('partials/menu-item', ["item" => $menu_item["item"], "id" => $aria_labelby]) ?>
            <button
              type="button"
              aria-label="Expand Submenu"
              aria-expanded="false"
              aria-controls="<?= $aria_controls ?>"
              aria-labelledby="<?= $aria_labelby ?>"
            >
              <?= asset('down-arrow.svg')->read() ?>
            </button>
          </div>
          <ul id="<?= $aria_controls ?>">
            <?php foreach ($menu_item["children"] as $subitem): ?>
              <li>
                <?php snippet('partials/menu-item', ["item" => $subitem]) ?>
              </li>
            <?php endforeach; ?>
          </ul>
        </li>
      <?php else: ?>
        <li>
          <?php snippet('partials/menu-item', ["item" => $menu_item["item"]]) ?>
        </li>
      <?php endif ?>
    <?php endforeach ?>
  </ul>
</nav>

Now this new snippet would get used like this:

/snippets/header.php

<header class="primary-header">
  <h1 class="sr-only"><?= $page->title()->esc() ?></h1>

  <a href="#main" class="sr-only">Skip to content</a>
  <a href="#site-index" class="sr-only">Skip to site index</a>

  <a class="logo" href="<?= $site->url() ?>">
    <?= asset('logo.svg')->read() ?>
  </a>

  <div>
    <?php
    $menu = [];

    foreach ($site->children()->listed() as $page) {
      $menu[] = [
        'item' => $page,
        'children' => $page->children()
      ];
    }

    snippet('partials/nav', ['menu_label' => "Main Menu", 'menu' => $menu])
    ?>
  </div>
</header>

Now I am more worried that this is going to be difficult to manage over time since typing inside the snippet template depends on using @var annotations, and typing when calling the snippet doesn’t seem possible. Also, the only way I have figured out how to test if I used a snippet correctly is to just go to the site and look to see if anything is broken, which doesn’t seem like a very efficient method.

To me, it seems like a snippet of this style would be much easier to understand and use if the call looked more like this:

new NavigationMarkupSnippet($menu_arr, $menu_label)
// Where these two parameters are type hinted if the person wants

But it also seems like this has not been an issue for people, so now I wonder if this logic is better handled a different way.

So, what do you all think? Is this an overuse or misuse of the snippet feature? Or, is this appropriate use but I’ve found some downsides that I don’t like?

[texnixe]

Apart from the fact that I wouldn’t want to instantiate classes in my templates, I think this would actually make it very cumbersome to work with snippets. If I understand what you are doing there with the pseudo-code, you would have to create a class for each snippet type. Then when you want to make just a little change, i.e. pass another variable, you would have to change this snippet class. Sound awkward to me. But maybe I’m getting it all wrong.

Having said that, you can do almost anything with Kirby, and could use your snippet classes for snippets if that’s what would make you happy.

[gaufde]

@texnixe

Then when you want to make just a little change, i.e. pass another variable, you would have to change this snippet class. Sound awkward to me. But maybe I’m getting it all wrong.

I know what you mean, and I would agree, but there are tools that do this automatically and that changes things for me quite a bit.

Have you ever tried the refactor menu in PhpStorm? I only learned about this recently, but in my mind it is a huge benefit! I can introduce a new parameter and have PhpStorm automatically insert a default value for that parameter for every call throughout the entire code base. I can decide to move a file, change it’s name , etc. and PhpStorm can automatically refactor all of that automatically too. Unfortunately, none of these features work very well with snippets as is.

@texnixe

Having said that, you can do almost anything with Kirby, and could use your snippet classes for snippets if that’s what would make you happy.

You’re right, I should try writing one myself to see how well it would work. Maybe that would be a good thing to practice making a Kirby plugin!

I think my only remaining question then is whether or not I am thinking about snippets in good way. How would others manage the navigation snippet examples?

[gaufde]

Okay, I took a first stab at this.

I tried to make a base class that makes it as easy as possible to extend for each snippet template. Also, I wanted as little boilerplate as possible in the child classes, full type hinting / static analysis support, and to make using/calling the final classes as simple as possible.

In the end it looks pretty simple, but it took me quite a while to figure this out. Also, if people better at PHP have suggestions I’m all ears!

In any case, here is what I tried:

/site/plugins/class-based-snippets/index.php

<?php

require __DIR__ . '/src/Autoloader.php';
require __DIR__ . '/src/ClassBasedSnippet.php';


$site_root = kirby()->root('site');

$path_to_classes = $site_root . '/snippets-classes/';

\Gaufde\Autoloader::register($path_to_classes);

\Kirby\Cms\App::plugin('gaufde/class-based-snippets', []);

/site/plugins/class-based-snippets/src/Autoloader.php

<?php

namespace Gaufde;

class Autoloader
{
    public static function register($dirPath): void
    {
        spl_autoload_register(function ($class) use ($dirPath) {
            // Replace namespace separators with directory separators in the class name
            $file = $dirPath . str_replace('\\', '/', $class) . '.php';

            // If the file exists, require it
            if (file_exists($file)) {
                require $file;
                return true;
            }
            return false;
        });
    }
}

/site/plugins/class-based-snippets/src/ClassBasedSnippet.php

<?php

namespace Gaufde;

abstract class ClassBasedSnippet
{
    abstract protected function template(): void;

    public function __toString()
    {
        ob_start();
        $this->template();
        return ob_get_clean();
    }
}

Now, I can create the directory /site/snippets-classes/. To recreate the examples above, I can now do this:

/site/snippets-classes/MenuItemSnippet.php

<?php

use Gaufde\ClassBasedSnippet;
use Kirby\Cms\Page;

final class MenuItemSnippet extends ClassBasedSnippet
{
    private Page $item;
    private string $id;

    /**
     * @param Page $item
     * @param string $id
     */
    public function __construct(Page $item, string $id = "")
    {
        $this->item = $item;
        $this->id = $id;
    }

    /**
     * @throws \Kirby\Exception\InvalidArgumentException
     */
    protected function template(): void
    {
        $isActive = $this->item->isActive() ? 'aria-current="page"' : '';
        $url = $this->item->url();
        $title = $this->item->title()->esc();
        $idAttribute = !empty($this->id) ? 'id="' . $this->id . '"' : '';

        echo <<<HTML
        <a $isActive href="$url" $idAttribute>$title </a>
        HTML;
    }
}

/site/snippets-classes/NavigationSnippet.php

<?php

use Gaufde\ClassBasedSnippet;

final class NavigationSnippet extends ClassBasedSnippet
{
    private string $menu_label;
    /**
     * @var array<array{item: Kirby\Cms\Page, children: Kirby\Cms\Page[]}>
     */
    private array $menu;

    /**
     * @param string $menu_label
     * @param array<array{item: Kirby\Cms\Page, children: Kirby\Cms\Page[]}> $menu
     */
    public function __construct(string $menu_label, array $menu)
    {
        $this->menu_label = $menu_label;
        $this->menu = $menu;
    }


    protected function template(): void
    {
        $sub_menu_index = 1;
        ?>
        <nav aria-label="<?= $this->menu_label ?>">
            <ul>
                <?php

                foreach ($this->menu as $menu_item):?>
                    <?php
                    $children = $menu_item["children"];
                    if (!empty($children)):
                        $aria_controls = "main-menu-submenu-$sub_menu_index";
                        $aria_labelby = "main-menu-label-$sub_menu_index";
                        $sub_menu_index++;
                        ?>
                        <li>
                            <div>
                                <?= new MenuItemSnippet($menu_item["item"], "$aria_labelby") ?>
                                <button
                                        type="button"
                                        aria-label="Expand Submenu"
                                        aria-expanded="false"
                                        aria-controls="<?= $aria_controls ?>"
                                        aria-labelledby="<?= $aria_labelby ?>"
                                >
                                    <?= asset('down-arrow.svg')->read() ?>
                                </button>
                            </div>
                            <ul id="<?= $aria_controls ?>">
                                <?php foreach ($menu_item["children"] as $subitem): ?>
                                    <li>
                                        <?= new MenuItemSnippet($subitem) ?>
                                    </li>
                                <?php endforeach; ?>
                            </ul>
                        </li>
                    <?php else: ?>
                        <li>
                            <?= new MenuItemSnippet($menu_item["item"]) ?>
                        </li>
                    <?php endif ?>
                <?php endforeach ?>
            </ul>
        </nav>
        <?php
    }
}

I know it looks like one has to write a lot of boilerplate. But my the IDE should be able to take care of most of it. For example, if I just write the class properties with type hints, PhpStorm can write the entire constructor for me. Then, all I have to do is implement the template method.

I actually think this is a pretty nice solution that plays nicely with Kirby’s default snippets. I now get code completion and type hinting everywhere I use the class-based-snippet:

/site/snippets/header.php:

That by itself isn’t a huge benefit, but now if I want to refactor it is super powerful. Say I realize that I need to pass another required parameter to MenuItemSnippet. That is now super easy with PhpStorm’s change signature feature:

/site/snippets-classes/MenuItemSnippet.php:

And now, not only is the MenuItemSnippet fully refactored to include the new parameter, but each usage of that class has been updated with the placeholder I specified automatically:

/site/snippets-classes/NavigationSnippet.php:

@texnixe Thank you for your patience with me! I think this exercise has reinforced how flexible Kirby is, and also it also shows me why this type of approach is not the default. I think the trade-offs are worth it for some types of snippets, but definitely not all.

[texnixe]

This solution comes at the price of putting HTML into code, where IMO it doesn’t belong.

And once you move it out again, you are back to step one…

But hey, everyone should get happy their way

[gaufde]

Yeah, I couldn’t see any way around having the HTML in there. You could move it to a separate file and use @var annotations in that file, but then that is even more to manage.

I suppose the most ideal solution would be to make plugins for Psalm and various IDEs that can fully understand Kirby’s design. I’ve never done anything like that before, so I have no idea how hard it would be.