getkirby / ideas

This is the backlog of ideas and feature requests from the last two years. Use our new feedback platform to post your new ideas or vote on existing ideas.
https://feedback.getkirby.com
20 stars 0 forks source link

KirbyTag: set flag when used with `->excerpt()` #403

Open hdodov opened 5 years ago

hdodov commented 5 years ago

Describe the bug I have a plugin with the following KirbyTag definition:

'tags' => [
    'article' => [
        'html' => function ($tag) {
            return 'lots of markup';
        }
    ]
]

This tag returns markup that forms a link to an article. The intention is for this tag to be used in an article's content and link to another article. The markup contains a title and an excerpt from that linked article.

The problem comes when the cross-linked article (the target of the kirbytag) also contains a link to an article and that link is in the beginning. This creates two problems:

  1. There would be an article link box inside the article link box.
  2. If (and this is a hardcore edge case, but still) the article links to itself, you get endless recursion.

Proposed solution

There should be a flag on the $tag argument that tells whether the tag is rendered for an excerpt. If it is, then I would want to simply render an empty string or just the linked article's name. If it's not, I render the actual markup that I want to render.

distantnative commented 5 years ago

This is currently more an idea than a bug.

lukasbestle commented 5 years ago

I don't think this issue is actually related to rendering an excerpt. Wouldn't it occur even if you weren't excerpting?

Endless recursion is always something that can only be prevented in the code itself. If you post your full code, I can take a look at it – maybe there's a simpler solution.

hdodov commented 5 years ago

Wouldn't it occur even if you weren't excerpting?

Actually, yes, I think. It's a problem of rendering kirbytags within kirbytags, not excerpts within excerpts. As you said:

Endless recursion is always something that can only be prevented in the code itself

Then there must be a way to know if the current tag is rendered within another tag. Perhaps there could be a $tag->stack or something that allows the user to know how the tags are nested and if the current tag is being rendered inside itself.

Overall, there a two issues:

lukasbestle commented 5 years ago

As I said, I need your code so that I can tell why it runs into an endless loop.

hdodov commented 5 years ago

Plugin tag in index.php:

'article' => [
  'html' => function ($tag) {
    $posts = site()->index()->template('post');
    $post = $posts->find($tag->value);
    if ($post) {
      return snippet('partials/article', ['page' => $post], true);
    } else {
      return '';
    }
  }
]

article.php snippet:

<a href="<?= $page->url() ?>">
  <div class="article flex justify-between my-8 border">
    <div class="px-6 py-6">
      <h5 class="mb-2 font-bold text-base"><?= $page->title() ?></h5>
      <p class="text-sm"><?= $page->text()->excerpt(140) ?></p>
    </div>
    <?php if (($image = $page->image()) && $image = $image->thumb(['width' => 500])): ?>
      <div class="flex-shrink-0 w-1/3 bg-cover bg-center" data-src="<?= $image->url() ?>" mb-image></div>
    <?php endif ?>
  </div>
</a>

It does nothing fancy. Tag calls snippet, snippet calls excerpt(), excerpt() calls tag and the circle closes.

lukasbestle commented 5 years ago

You can use a static variable to fix this:

'article' => [
  'html' => function ($tag) {
    static $recursion = false;

    if ($recursion === true) {
      // no need to have an article link inside an article link
      return '';
    }
    $recursion = true;

    $posts = site()->index()->template('post');
    $post = $posts->find($tag->value);
    if ($post) {
      $return = snippet('partials/article', ['page' => $post], true);
    } else {
      $return = '';
    }

    $recursion = false;
    return $return;
  }
]

Not tested, but you get the idea. :)

hdodov commented 5 years ago

Hm, I suppose that fixes the recursion problem. But the box within a box problem still remains and I don't think that can be avoided in a similar manner. Kirby has to indicate somehow that the tag is rendered in an excerpt.

I think that $tag->isExcerpt() should be in core because allowing kirbytags to define their own excerpts is better than straight up rendering them. That could cause severe markup problems if, for example, you render an <a> tag within an <a> tag.

lukasbestle commented 5 years ago

That makes sense!

If I remember correctly, the Kirby 2 excerpt() method didn't run KirbyText, so it just took the first 140 (or whatever) raw chars. That's not optimal either. But the current behavior (convert to KirbyText, cut after the first xyz chars) has its problems too.

I generally think that excerpts for complex content (that doesn't just use a bit of Markdown formatting) aren't optimal – complex content might get cut in the middle. So I think it's hard to find a good solution. Best way is to have a separate excerpt field that can be populated by the editors. But of course that's more work.

hdodov commented 5 years ago

Yes, but you can't know if a content is complex or not - it's up to the editor. In my case, you could simply have the article title and then the content as markdown. Creating an excerpt for that is straightforward. But if the editor decides to put a kirbytag in the beginning - that's where things get complicated.

I generally think that excerpts for complex content (that doesn't just use a bit of Markdown formatting) aren't optimal – complex content might get cut in the middle.

I agree. That's why developers should have the choice to print no markup (return '';) when the tag is rendered in an excerpt. In my case, that would be the perfect solution. The article link is skipped and the next paragraph is included in the excerpt. On the other hand, some other tags might render excerpt-specific content that is perfectly fine to be cut off. That's why the decision should be up to the developer as he can best decide what the behavior should be.

lukasbestle commented 5 years ago

Yep, I see your point. :)

Maybe we could even use that new flag in the tags that get shipped with the core: For example the image tag doesn't make sense in an excerpt.