Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

NicoHood
Copy link
Contributor

@NicoHood NicoHood commented Jan 9, 2021

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:
f61725c#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 #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 #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:
b716784

But what is that field used for?

@mahagr mahagr requested review from rhukster and mahagr and removed request for rhukster January 11, 2021 09:52
@mahagr
Copy link
Member

mahagr commented Jan 11, 2021

I would like us to get some standard on namin field features, first. Like I said in #474 (review), 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 mahagr added the bug label Jan 11, 2021
{% block field %}

{% set value = value ?: (field.value ?? (field.evaluate ? evaluate(field.default) : field.default)) %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhukster Above old code is broken (and can only be fixed in base field), which means that we kind of need to implement this one.

@mahagr
Copy link
Member

mahagr commented Jan 11, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants