`href` Attribute is missing when having a `link` with multiple marks

o1y commented 1 year ago

Bug Description

Not sure if this is a Bard Mutator bug, but I came across an issue since Bard Mutator 2 when having a link, which contains multiple marks. Bard Mutator then returns just for the first link mark the href attribute. In the second mark href is not available, which may cause an error if no proper check is implemented.

How to Reproduce

-
  type: paragraph
  content:
    -
      type: text
      text: 'Lorem Ipsum '
    -
      type: text
      text: 'italic link'
      marks:
        -
          type: link
          attrs:
            href: /test
        -
          type: italic
    -
      type: text
      text: ' regular'
      marks:
        -
          type: link
          attrs:
            href: /test

Mutator::html('link', function ($value) {
    $href = $value[1]['href']; // `Undefined array key "href"`
    // ... 
});

Environment

Environment
Application Name: Statamic
Laravel Version: 9.30.1
PHP Version: 8.2.3
Composer Version: 2.5.4
Environment: local
Debug Mode: ENABLED
URL: localhost
Maintenance Mode: OFF

Cache
Config: NOT CACHED
Events: NOT CACHED
Routes: NOT CACHED
Views: CACHED

Drivers
Broadcasting: log
Cache: statamic
Database: mysql
Logs: stack / single
Mail: smtp
Queue: redis
Session: file

Statamic
Addons: 1
Antlers: runtime
Stache Watcher: Enabled
Static Caching: Disabled
Version: 3.4.5 PRO

Statamic Addons
jacksleight/statamic-bard-mutator: 2.0.1

jacksleight commented 1 year ago

Hey Oli. This is an interesting one! It's due to a combination of behaviours, but the short answer is: href isn't actually a default/required attribute of the link mark, so you shouldn't assume it'll be there.

The long answer, and reason this only happens when you have data like that is:

Tiptap will call the render HTML method twice, once for the opening tag and once for the closing tag.
When it renders the closing tag it doesn't pass the attributes, and so they fall back to the defaults where href is missing.
Because calling mutators twice for the same tag (once with no attributes) isn't really necessary or very useful Mutator checks for the second render and skips your callback. Which is why you don't normally run into this issue.
When you have two separate text nodes with matching marks next to each other tiptap will merge the tags together, which means the opening tag for the second node is never rendered.
However the closing tag is rendered, and because Mutator hasn't seen that node before it doesn't know to skip your callback, so it gets called with the default attributes.

The legacy prosemirror-to-html package handled the tag rendering differently, which is why this has only cropped up with v2.

I'm not sure there's any way for me to work around this in Mutator, but I'll give it some thought.

o1y commented 1 year ago

Thanks for your good explanation, Jack! When href is not a default attribute of the link mark, then this is definately not a bug. So, feel free to close this issue.

Initially, I found it a bit confusing because both 'href' values were being set, but you clarified what happens in the tiptap background :)

jacksleight commented 1 year ago

Yeah it's certainly best to check for the href, but it would be nice if Mutators "skip closing tags” behaviour could handle this situation, as it would make things more consistent.

I’ve had an idea for a change that I’m gonna test out, so I’ll leave this open for now. 👍

jacksleight commented 1 year ago

Fixed in https://github.com/jacksleight/statamic-bard-mutator/releases/tag/2.1.1

o1y commented 1 year ago

Wow, cool, thanks! 🚀

jacksleight / statamic-bard-mutator