Added evaluate option for all non-iterable field types and checkboxes - #473
Open
NicoHood wants to merge 5 commits into
Open
Added evaluate option for all non-iterable field types and checkboxes#473NicoHood wants to merge 5 commits into
NicoHood wants to merge 5 commits into
Conversation
NicoHood
referenced
this pull request
Jan 9, 2021
NicoHood
force-pushed
the
evaluate_defaults
branch
from
January 9, 2021 12:47
7c630e2 to
57373ef
Compare
mahagr
requested review from
mahagr and
rhukster
and removed request for
rhukster
January 11, 2021 09:52
Member
|
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
reviewed
Jan 11, 2021
|
|
||
| {% block field %} | ||
|
|
||
| {% set value = value ?: (field.value ?? (field.evaluate ? evaluate(field.default) : field.default)) %} |
Member
There was a problem hiding this comment.
@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.
Member
|
This issue may also affect admin forms, so we need to make sure those keep on working as expected. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds the option to
evaluateall 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:
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_twiginstead of "only"evaluate. This gives us more options, and the syntax is used in multipleprocessoptions 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: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 ofdefault: "'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_twignow and added backwards compatibility support for thehiddenfield.Edit3: I just noticed, that the default value is set in the form like that. Maybe that should be disabled when using evaluate?

It can be fixed with:
b716784
But what is that field used for?