getkirby / kirby

Kirby's core application folder
https://getkirby.com
Other
1.32k stars 168 forks source link

Problem using the custom create dialog with multiple required fields #5616

Closed lukaskleinschmidt closed 1 year ago

lukaskleinschmidt commented 1 year ago

Description

I have a custom create dialog that uses two required fields and automatically publishes the page. But it seems like that only the first custom field actually prevents the page creation.

create:
  title:
    label: Internal Label
  fields:
    - question
    - answer
  redirect: false
  status: unlisted

sections:
  fields:
    type: fields
    fields:
      question:
        label: Question
        type: text
        required: true

      answer:
        label: Answer
        type: writer
        required: true

custom_create_dialog

Your setup

4.0.0-beta.1

Your system (please complete the following information)

afbora commented 1 year ago

There is a missing something. Because writer field unsupported:

Field type "writer" not supported in create dialog

lukaskleinschmidt commented 1 year ago

I have actually declared it.

Kirby\Panel\PageCreateDialog::$fieldTypes[] = 'writer';

But also just checked with a default text field. And it works with that … Is the description here still valid regarding declaring custom field types?

afbora commented 1 year ago

textarea and writer removed intentionally in https://github.com/getkirby/kirby/pull/5243, pinging @distantnative

lukaskleinschmidt commented 1 year ago

I just did another test using the textarea field and it is working with that. So there seems to be a problem with the writer field in particular.

distantnative commented 1 year ago

Textures and writer were first of all removed because of the link, email etc dialogs will not work. But that's also why we haven't tested those further.

Declaring custom types is rather meant for plugin fields. Declaring core fields that we left out for reasons is a bit counterintuitive 😅

But would be good to narrow it down nevertheless: is it an issue with a specific field type or an issue of multiple validation rules not really being considered?

afbora commented 1 year ago

IMHO, native form validation seems to be working in the create dialog, not fiber validation. Writer field output doesn't have a native input, so it doesn't enter validation at all.

lukaskleinschmidt commented 1 year ago

Declaring custom types is rather meant for plugin fields. Declaring core fields that we left out for reasons is a bit counterintuitive 😅

Fair enough. Did not take the dialog problem into consideration. I would be fine disableing those options for the create dialog. In that regard it is only possible to define the fields used in the create dialog to be an array of (field name) references?

lukaskleinschmidt commented 1 year ago

By the way loving computed fields! Now only the validation needs to work in the dialog. But no pressure. I will fallback to a textarea for now to make it work for editors 🙂

<?php

use Kirby\Cms\App;

return fn (App $kirby) => [
    'label'    => 'Answer',
    'type'     => 'writer',
    'required' => true,
    'marks'    => $kirby->request()->path() == 'panel/dialogs/pages/create' ? [
        'bold',
        'italic',
        'underline',
        'strike',
        'code',
    ] : null,
];
distantnative commented 1 year ago

Sorry, this discussion is jumping around a lot, could one of you please summarize what the actual issue is now?

afbora commented 1 year ago

As far as I understand; If writer field is added to create dialog which is not supported yet (adding like that), it does not apply create dialog form validation.

lukasbestle commented 1 year ago

Does this mean that field validation doesn't work in general in the create dialog or is it a writer-specific problem?

afbora commented 1 year ago

@lukasbestle No, validation works as expected for supported fields in create dialog. The issue is only for writer field (create dialog already doesn't support writer field due to link dialog). I think this is not a bug for now but will be when create dialog supports the writer field.

distantnative commented 1 year ago

I think it is a bug: the dialog doesn't support the writer field, but we only disabled it because of the dialogs (and us not having nested dialogs). But I don't see a reason why the writer field validation isn't working, so that seems like a bug.

I think e need to investigate

  1. Is the broken validation really limited to the page create dialog or is the writer field/input validation also broken as normal standalone field?
  2. There was some mention of the first works, but not the second: can we determine if the order of fields makes any difference to whether validation works or not?
  3. Is it all validation rules that don't work for the writer field, or specific ones not?
afbora commented 1 year ago

I still think the issue is related with that. I've checked with structure field, same issue.

distantnative commented 1 year ago

@afbora I don't understand the linked comment to be honest, I think you need to explain more detailed what you are referring to, maybe with code examples.

afbora commented 1 year ago

@distantnative I don't know how to explain it. There is no novalidate attr for form in the create dialog and native form validation run. But the form in page view has novalidate attr and I think we are validating the form with fiber. Am I thinking wrong?

Is it more descriptive?

lukaskleinschmidt commented 1 year ago

So custom validation rules don't work either with the current create dialog (writer field aside).

create:
  title:
    label: Internal Label
  fields:
    - question
    - answer
  redirect: false
  status: unlisted

sections:
  fields:
    type: fields
    fields:
      question:
        label: Question
        type: text
        required: true
        validate:
          min: 10 # This one
          max: 140 # And this one

      answer:
        label: Answer
        type: text
        required: true

The page gets created but will not be published.

grafik

distantnative commented 1 year ago

@lukaskleinschmidt does it work for you if you add just these highlighted line? https://github.com/getkirby/kirby/pull/5687/files#diff-382775edc1b8bd986c5e0f5121f9772234c21e94711812de545e6cc5c38f858cR279-R285

lukaskleinschmidt commented 1 year ago

You mean this bit of code?

// create temporary form to validate the input
$form = Form::for($this->model(), ['values' => $input]);

if ($form->isInvalid() === true) {
    throw new InvalidArgumentException([
        'key' => 'page.changeStatus.incomplete'
    ]);
}

It prevents the page creation. But it prevents it all the time even if the form data is actually ok. I'm currently on 4.0.0-beta.1 and also tried replacing all files mentioned here with no success.