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

getFormFieldTypes() does not merge settings in frontend forms #3219

Open
NicoHood opened this issue Feb 11, 2021 · 9 comments
Open

getFormFieldTypes() does not merge settings in frontend forms #3219

NicoHood opened this issue Feb 11, 2021 · 9 comments
Assignees
Labels

Comments

@NicoHood
Copy link
Contributor

NicoHood commented Feb 11, 2021

Imagine a plugin code like this:

    /** @var array */
    public $features = [
        'formfields' => 0
    ];

    /**
     * Get list of form field types specified in this plugin.
     * Only special types needs to be listed.
     * NOTE: This function gets called by the grav plugin framework.
     *
     * @return array
     */
    public function getFormFieldTypes()
    {
        return [
            'rating' => [
                'validate' => [
                    'type' => 'number',
                    'step' => 1,
                    'min' => 1,
                    'max' => 5
                ]
            ]
        ];
    }

Now I got the following yaml file:

        - name: stars
          type: rating
          label: PLUGIN_RATINGS.RATING_LABEL
          validate:
            required: true

Because I specified validate.required: true all other settings set by getFormFieldTypes() are now lost. I need to repeat them again, which is more error prone:

        - name: stars
          type: rating
          label: PLUGIN_RATINGS.RATING_LABEL
          validate:
            min: 1
            max: 5
            step: 1
            type: number
            required: true
@mahagr
Copy link
Member

mahagr commented Feb 11, 2021

Yes, that is kind of what is expected as it's not a merge, but just adding missing keys (defaults) to the main level. In most cases merging would be wrong anyway, but I think that for validate it should do an exception.

@mahagr mahagr self-assigned this Feb 11, 2021
@mahagr mahagr added the bug label Feb 11, 2021
@mahagr
Copy link
Member

mahagr commented Feb 11, 2021

Needs a fix in vendor/rockettheme/toolbox/Blueprints/src/BlueprintSchema.php line 509 and 514.

@mahagr
Copy link
Member

mahagr commented Apr 9, 2021

Should be fixed in toolbox.

@mahagr mahagr added the fixed label Apr 9, 2021
@mahagr mahagr closed this as completed Apr 13, 2021
@NicoHood
Copy link
Contributor Author

What is the target milestone/version?

@mahagr
Copy link
Member

mahagr commented Apr 14, 2021

Next version, it'll be out today.

@NicoHood
Copy link
Contributor Author

This still does not work for me with 1.7.12. And yes, I've done a composer update.

@mahagr
Copy link
Member

mahagr commented Apr 22, 2021

I tested adding the field into a page.

I'm getting this error when trying to enter invalid value:

Invalid input in "PLUGIN_RATINGS.RATING_LABEL"
Save Failed: Form validation failed, please check your input

So the validation surely works, at least in admin and with flex objects.

@NicoHood
Copy link
Contributor Author

NicoHood commented Apr 22, 2021

Seems not to be the case for frontend forms. I am using this form field template to test (i've manually modified the max to 10, but the code should limit it to 5)

{% extends "forms/field.html.twig" %}

{% set originalValue = value %}

{% block input %}
    <fieldset class="rating-input {{ form_field_wrapper_classes }} {{ field.wrapper_classes }}">
        {# It is important to count from max to min, as this is required by the css. #}
        {% for i in 10..field.validate.min|default(1) %}
            {% set id = field.id|default(field.name) ~ '-star' ~ i %}
            <input type="radio"
                   value="{{ i|e }}"
                   id="{{ id|e }}"
                   name="{{ (scope ~ field.name)|fieldName }}"
                   class="{{ form_field_rating_classes }} {{ field.classes }}"
                   {% if i == value %}checked="checked" {% endif %}
                   {% if field.disabled or isDisabledToggleable %}disabled="disabled"{% endif %}
                   {% if required %}required="required"{% endif %}
                   {% if field.tabindex %}tabindex="{{ field.tabindex }}"{% endif %}
                   min="{{ field.validate.min|default(1) }}"
                   max="{{ field.validate.max|default(5) }}"
                   step="{{ field.validate.step|default(1) }}"
            />
            <label for="{{ id|e }}" title="{{ field.titles[i]|t|default(i ~ ' ' ~ 'PLUGIN_RATINGS.STARS'|t) }}"></label>
        {% endfor %}
    </fieldset>

{% endblock %}

And you must make sure when adding the form, you must set it to required.

Now I got the following yaml file:

        - name: stars
          type: rating
          label: PLUGIN_RATINGS.RATING_LABEL
          validate:
            required: true

Maybe the problem only occurs because I've added the form via code?
https://github.com/NicoHood/grav-plugin-ratings/blob/master/ratings.php#L112

Steps to reproduce:

  1. Download grav https://getgrav.org/downloads
  2. install database plugin https://github.com/getgrav/grav-plugin-database
  3. install my ratings plugin https://github.com/NicoHood/grav-plugin-ratings
  4. modify the config as written above
  5. modify the template as written above
  6. Add {% include 'partials/ratings-form.html.twig' %} to the base.html template somewhere to test
  7. Open the browser and give a 10 star rating. no error pops up.

Ready to test example:
grav-admin.zip

@mahagr mahagr reopened this Apr 23, 2021
@mahagr mahagr removed the fixed label Apr 23, 2021
@mahagr mahagr changed the title getFormFieldTypes() does not merge settings getFormFieldTypes() does not merge settings in frontend forms Apr 23, 2021
@mahagr
Copy link
Member

mahagr commented Apr 23, 2021

I reopened the issue so I won't forget it.

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

No branches or pull requests

2 participants