getkirby / kirby

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

Could not save content for select field if queried value is just created #3592

Closed PatricB closed 2 years ago

PatricB commented 3 years ago

Describe the bug
I have a select field for a user with a options query which filters for a specific template. Know I want to create in a controller a new page of this template and fill the field on a user with value for new page. That doesn't work, because the new page is not in the queried options and therefore not allowed.

In my case I used the another field of the page as value for the select field - not page.id.

To Reproduce
Steps to reproduce the behavior:

  1. Create a field (on a user or page blueprint) like this
    company:
    type: select
    multiple: false
    options: query
    query:
      fetch: site.children.filterBy('template', 'company')
      text: "{{ page.title }}"
      value: "{{ page.id }}"
  2. Create dynamically a new page with the "company" template and update the content of the object with the field
$company = site()->createChild([
    'slug' => str::slug(microtime()),
    'template' => 'company',
    'draft' => false
]);

\Kirby\Cms\User::create([
    'name'  => 'User' . microtime(),
    'email' => 'email@email.com',
    'content' => ['company' => $company->id()]
]);
  1. Check the object with the field. The field will be empty

Expected behavior
Value of the select field should be filled with the given value, because it is a valid option.

Kirby Version
v3.5.7.1

afbora commented 3 years ago

'draft' => false I don't think that this will work. Your child page still draft. I guess you should change the child status before user create, for ex: $company->changeStatus('unlisted');

Or may be include draft pages with childrenAndDrafts() like site.childrenAndDrafts.filterBy('template', 'company')

lukasbestle commented 3 years ago

The property is called isDraft, which should actually work.

afbora commented 3 years ago

Ahh, yes I've tested and works like your said.

@PatricB I can reproduce the problem with no template file in /site/templates/company.php. Is there a file of the company template? Could you try with using intendedTemplate ìf no template file exists: site.children.filterBy('intendedTemplate', 'company')?

PatricB commented 3 years ago

@afbora thx for the fast response!

I use draft because otherwise isDraft would not be set. (see in Code) Seems to be special for the Page::create method.

I have a company template. But just figured out, that my problem existed if my query was a little bit different. Previously I had this query: site.index.filterBy('template', 'company'). Seems like using "index" instead of "children" caused this issue. What also works is site.index(true).filterBy('template', 'company') (index with drafts).

Seems to happen because I already used site()->index() a few lines before. Because of this, the PageCollection for site()->children() / site()->index() is cached. I can not clear this cache property. I only could avoid using site()->index() before creating the new page...

Do you have a other suggestion?

lukasbestle commented 3 years ago

I use draft because otherwise isDraft would not be set. (see in Code) Seems to be special for the Page::create method.

Ah, yes. Good point!

Seems to happen because I already used site()->index() a few lines before.

Yep, now that you say it that's very likely indeed. The index collection is cached for the whole rest of the request.

I think we changed this behavior in Kirby 3.6 (many collections are now automatically updated when a collection item is created or deleted). I'm not sure if we also did it with $site->index() yet, but if not, then it's a bug. Could you please test if you can confirm the issue with 3.6-alpha.3?

PatricB commented 3 years ago

@lukasbestle yes I can test it, but it have to wait at least a week. Sooner I do not get to.

PatricB commented 3 years ago

@lukasbestle Ok, today I run a test with the v3.6-alpha.3 and I can confirm also for this version.

Here my test scenario (slightly updated):

user blueprint

fields:
  company:
    label: Firma
    type: select
    multiple: false
    options: query
    query:
      fetch: site.index.filterBy('template', 'company')
      text: "{{ page.title }}"
      value: "{{ page.id }}"

route for creation test

[
                'pattern' => 'test',
                'action'  => function () {
                    kirby()->impersonate('kirby');
                    $timestamp = timestamp('now');

                    // fill index cache
                    $index = site()->index();

                   // create the page
                    $company = site()->createChild([
                        'slug'     => 'test-' . $timestamp,
                        'template' => 'company',
                        'draft'    => false
                    ]);

                    // create the user with the field "company" filled
                    $user = \Kirby\Cms\User::create([
                        'name'    => 'User' . $timestamp,
                        'role'    => 'client',
                        'email'   => $timestamp . '.email@email.com',
                        'content' => ['company' => $company->id()]
                    ]);

                    return kirby()->response()->json([
                        'test'            => $user->company()->value() === $company->id(),
                        'user'            => $user->toArray(),
                        'newCompany'      => $company->toArray()
                    ]);
                },
]

My workaround for know is still, that I use fetch: site.children.filterBy('template', 'company') on the field config.

Maybe can inspect the core code in the next day, for further assistance.

lukasbestle commented 3 years ago

It looks like we don't purge/update the $pages->index() collections on model updates so far. Now that I think about it, we have the situation that every single $pages object (be it $site or any $page->children()) can have an index() collection – or actually two (one with drafts and one without). So we'd need to purge or update every index() collection of all parents up to the $site, which could be a performance issue.

One big gotcha we need to keep in mind with every solution: Kirby should at no point start actually building index() collections on updates. If a $pages collection does not already have an index(), it should be skipped.

I'm still a bit worried about potential performance concerns with large page trees though. Thoughts @getkirby/kirby-staff?

distantnative commented 3 years ago

Hard to say for me in regards to performance.

Another half-fix could be that we allow force-storing values via ::create() or ::update() that the system thinks don't (yet) exist.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. This is for us to prioritize issues that are still relevant to our community. It will be closed if no further activity occurs within the next 14 days. If this issue is still relevant to you, please leave a comment.