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

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

Closed
lukaskleinschmidt opened this issue Sep 11, 2023 · 19 comments · Fixed by #5687
Closed

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

lukaskleinschmidt opened this issue Sep 11, 2023 · 19 comments · Fixed by #5687
Assignees
Labels
type: bug 🐛 Is a bug; fixes a bug
Milestone

Comments

@lukaskleinschmidt
Copy link
Contributor

lukaskleinschmidt commented Sep 11, 2023

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)

  • Device: Desktop
  • OS: Windows 10
  • Browser: Firefox 118
@afbora
Copy link
Member

afbora commented Sep 11, 2023

There is a missing something. Because writer field unsupported:

Field type "writer" not supported in create dialog

@afbora afbora added the needs: information ❓ Requires more information to proceed label Sep 11, 2023
@lukaskleinschmidt
Copy link
Contributor Author

lukaskleinschmidt commented Sep 11, 2023

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
Copy link
Member

afbora commented Sep 11, 2023

textarea and writer removed intentionally in #5243, pinging @distantnative

@lukaskleinschmidt
Copy link
Contributor Author

lukaskleinschmidt commented Sep 11, 2023

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
Copy link
Member

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
Copy link
Member

afbora commented Sep 11, 2023

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
Copy link
Contributor Author

lukaskleinschmidt commented Sep 12, 2023

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
Copy link
Contributor Author

lukaskleinschmidt commented Sep 12, 2023

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
Copy link
Member

distantnative commented Sep 16, 2023

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

@afbora
Copy link
Member

afbora commented Sep 16, 2023

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
Copy link
Member

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

@afbora
Copy link
Member

afbora commented Sep 16, 2023

@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
Copy link
Member

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
Copy link
Member

afbora commented Sep 16, 2023

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

@distantnative
Copy link
Member

@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
Copy link
Member

afbora commented Sep 18, 2023

@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
Copy link
Contributor Author

lukaskleinschmidt commented Sep 18, 2023

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 distantnative self-assigned this Sep 24, 2023
@distantnative distantnative added type: bug 🐛 Is a bug; fixes a bug and removed needs: information ❓ Requires more information to proceed labels Sep 24, 2023
@distantnative distantnative added this to the 4.0.0 milestone Sep 24, 2023
@distantnative
Copy link
Member

@lukaskleinschmidt
Copy link
Contributor Author

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.

@distantnative distantnative linked a pull request Sep 24, 2023 that will close this issue
5 tasks
@distantnative distantnative modified the milestones: 4.0.0, 4.0.0-beta.3 Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug 🐛 Is a bug; fixes a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants