grav-plugin-form icon indicating copy to clipboard operation
grav-plugin-form copied to clipboard

Added evaluate option for all non-iterable field types and checkboxes

Open NicoHood opened this issue 3 years ago • 2 comments

This PR adds the option to evaluate all kind of field default values. Currently every string based field and checkboxes are supported. This is very helpful to prefill forms based on query strings for example. This PR is safe against XSS, as only the default value gets evaluated. The evaluation function should be written with care, as always.

It also fixes the hidden field again: https://github.com/getgrav/grav-plugin-form/commit/f61725c9e1f79cf8d414fc2f7a6a62d234cc8715#commitcomment-45792369

Examples:

name:
    label: Title
    placeholder: 'Enter the new title'
    type: text
    evaluate: true
    default: 'page.title'
    validate:
        required: true

my_field:
    type: checkboxes
    label: A couple of checkboxes
    evaluate: true
    default:
        - "{{uri.query('happy') is same as('true') ? 'option2'}}"
    options:
        option1: Option 1
        option2: Option 2

my_field_keys:
    type: checkboxes
    label: A couple of checkboxes
    evaluate: true
    default:
        option1: `{{uri.query('happy') is same as('true')}}`
    options:
        option1: Option 1
        option2: Option 2

This PR needs a rebase after https://github.com/getgrav/grav-plugin-form/pull/472 is merged. I will take care of that then.

Edit: I'd also like to discuss to make use of evaluate_twig instead of "only" evaluate. This gives us more options, and the syntax is used in multiple process options as well. It would break for hidden fields, but I can add a fix if desired. If we do not add this, more complex functions would need the following workaround, which is quite ugly:

default: "null -}}
    {% set test = page.find('/blog/my_first_post').title %}
    {{- test -}}
    {{- null"

Edit 2: The evaluated twig gets directly injected into the template. Meaning that setting variables with names that have been previously used will cause a lot of trouble. I found, that it makes more sense to create a new page template that inherits the form template with some additional macros that can be used inside the form. This should also improve performance, if the variables are used multiple times in the form. On the other side i found it way simpler to write: default: "New {{ page.title }}" instead of default: "'New' ~ page.title". When looking at https://github.com/getgrav/grav-plugin-form/pull/474 it would make sense to keep them consistent. It would make sense to document, that setting variables here is dangerous. Also I dont know how many users will use it combined with setting variables. ==> I've choosen evaluate_twig now and added backwards compatibility support for the hidden field.

Edit3: I just noticed, that the default value is set in the form like that. Maybe that should be disabled when using evaluate? image

It can be fixed with: https://github.com/getgrav/grav-plugin-form/pull/473/commits/b7167846de8e509f8a2124fd651e85d1d61593df

But what is that field used for?

NicoHood avatar Jan 09 '21 12:01 NicoHood

I would like us to get some standard on namin field features, first. Like I said in https://github.com/getgrav/grav-plugin-form/pull/474#pullrequestreview-565195428, we need some better approach which both indicates the intent better (allowing you to guess what the property does) and allows us to add more similar features without conflicts.

mahagr avatar Jan 11 '21 09:01 mahagr

This issue may also affect admin forms, so we need to make sure those keep on working as expected.

mahagr avatar Jan 11 '21 10:01 mahagr