Additional field is saved different

[jenstornell]

I use your code in the config.php:

c::set('comments.custom-fields', array(
    array(
      'name' => 'stars',
      'title' => 'Stars',
      'required' => true, // you decide, default is `false`
      'validate' => function ($value, $page) {
        $intval = intval($value);
        return $intval >= 1 && $intval <= 5;
      },
      'sanitize' => function ($value, $page) {
        return intval($value);
      }
    )
  ));

When the file is saved it looks like this:

Cid: 1

----

Date: 2017-10-25 06:52:07

----

Name: Jens Törnell

----

Email: something@gmail.com

----

Website: http://www.test.com

----

Text: Hello message

----

Customfields: 

stars: 3

At first I thought it was wrong but when using echo $comment->customField('stars');, it was working. The problem is that it's not super simple to present this value in the panel. To do that, a new field would need to be created.

Why not just add it like this?

----

Text: Hello message

---

stars: 3

[florianpircher]

You are right, adding custom fields at the top-level in theory is the better solution. This, however, makes it impossible for me to introduce new fields in future versions of Kirby Comments.

Say stars become a standard feature one day. In that case, I would use the stars key on the top level to store this information. But what if the new, standard stars feature uses stars in the range of 1–10? Now your website will not work anymore because it does not expect values above 5.

This is why all custom fields are nested as children of the Customfields key. No conflicts and it’s. future proof. Storing nested data as YAML is the recommended way in Kirby. It’s the way the starterkit does it. I think the problem is Kirby panel not supporting nested structures.

[jenstornell]

If I would prefix stars to jenstornell_stars it would never ever be a collision and I could still use the fields in the panel to present the data without building a custom field for it.

I would prefer adding a prefix to protect the value, instead of making it nested and unreadable by the panel.

Maybe you could make it optional, or would it be too much work? In that case a warning in the docs would be in place like:

WARNING: If this option is set to true, it may collide with the built in values. Prefix your inline field keys with something very unique.

Would that be a good idea or don't you like it at all?

[florianpircher]

Using namespaces feels like a hack to me. It works, but it’s not good software design. The current design is save and future proof. Namespaces are unsafe and have to be remembered by the developer. Namespaces make your template-code less readable and require lots of repetition. And adding WARNINGS feels like making Kirby Comments secure someone else’s problem.

I would prefer adding a prefix to protect the value, instead of making it nested and unreadable by the panel.

I still think this is a problem that should be addressed by the panel and not my plugin. The plugin stores the data in the cleaned possible way and I don’t want to change that.

That said, if you need, need, the value to be stored at the top level you can use the did-save-comment hook, get the comment page and insert your custom value at root level using $page->update. It’s a hack, but this hack does not apply to other users of the plugin.

[jenstornell]

If namespacing is a hack, then I think that nesting it by Customfields is a hack as well. I think both solutions try to solve the same problem, but at the cost of different kind of issues.

Namespacing is a problem in Kirby in other places, for example with c::set where we need to prefix the config with a plugin name to prevent collisions with the core options and other plugins.

About the hook idea. It's quite good for my case. What I don't like about hooks of today is that there is no pre-hook. I mean it needs to save the content first and then save it again with the new data.

Anyway, I accept your stand. :) Should we close this issue?

[florianpircher]

I don’t consider nesting-by-customfield a hack; it is one of the applications for which you might want to use custom fields. Nesting is one of the features and custom fields are on of the features. Combined they can be used to implement replies (the most common form of nesting), but you are not limited to that specific use-case.

c::set uses namespaces because setting values of other namespaces is encouraged. Kirby places most option on the root level (like license or debug), caching is done in the cache. domain and comments are managed in the comments. domain. With custom fields you never want to set anything outside of the Customfields domain. Changing the Id or Text field of a comment could result in Kirby Comments not been able to read that field and crash.

A pre-save hook is an interesting idea. The hook would need the proposed contents of the page and return the modified array. I will be looking into the implications of adding such a hook, in the meantime you can re-save the page.

[jenstornell]

Ahh, so you can do it in the plugin. That's interesting. I was thinking about a pre hook for the panel but I guess it will take a while before it's implemented there.

A bit off topic but, I think it's starting to look quite nice visually on my site (localhost). =)

[florianpircher]

Version 1.5.1 adds the $commentPage argument to the did-save-comment hook. You can use this hook to run your custom code.

[florianpircher]

Can I close this issue?