getkirby / kirby

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

Using `site.find(self)` in blueprint breaks change status dialog for drafts #2925

Closed nilshoerrmann closed 3 years ago

nilshoerrmann commented 3 years ago

Describe the bug
A self reference in the blueprint using site.find(…) breaks the change status dialog for pages in draft mode.
It's complicated, let me try to explain:

We've got an "awards" page that features three pages sections to display drafts, unlisted and listed pages (nothing special). As this section is needed repeatedly across the site, we defined it in a separate file, blueprints/sections/awards.yml, using a hardcoded reference to the awards page for the parent:

headline: Awards
type: pages
layout: cards
parent: site.find("awards")
status: listed
templates: award
info: '{{ page.related.toPage.title }} ({{ page.year }})'

This works fine as long a the referenced page is not a draft, because drafts are excluded by the find method. In this case, all sections using this definition will throw an error. This is expected behaviour.

In our case though, as this hardcoded reference is also used on the awards page itself, the change status dialog is broken as soon as awards is put into draft mode. It seems to promote the section error to the change status dialog:

To reproduce:
Clone the Starterkit and change blueprints/sections/notes.yml to the following:

type: pages
headline: Notes
parent: site.find("notes")
info: "{{ page.date.toDate('d.m.Y') }}"
template: note
empty: No notes yet
sortBy: date desc

Put the notes page into draft mode.

Screenshots:

This is what the section looks like when the page is in drafts mode (expected):

grafik

This is what's displayed when you try to load the change status dialog:

grafik

Expected behavior
The error should not be promoted to the change status dialog and changing status should be possible as usual.

Workaround:
Looking at the Starterkit, kirby.page(…) should be used instead of site.find(…) because it includes drafts.

Kirby Version
Kirby 3.4.x and 3.3.x, not sure about earlier versions.

REHvision commented 3 years ago

I don't know about the other parts of your question but Kirby has a method for this: $site->findPageOrDraft() https://getkirby.com/docs/reference/objects/cms/site/find-page-or-draft

texnixe commented 3 years ago

Or use kirby.page('awards') instead, like in the Starterkit sections.

nilshoerrmann commented 3 years ago

Yes, the issue can be circumvented, I noted this in the "Workaround" paragraph already.

Just to make the point of this issue clear: The displayed error messages for the pages sections are correct, they reflect the given setup. What needs to be fixed in the core though is the broken change status dialog (see the last screenshot). The change status dialog should not be affected by any error in a pages section.

lukasbestle commented 3 years ago

It's weird that the sections seem to be loaded when generating the API response for the status dialog. I would have expected that these get lazy-loaded. But in any case this will change with the new Fiber setup. I think this situation where parts of the page work and others don't won't happen here. So I think we can close this. @distantnative?

distantnative commented 3 years ago

I don't think it's the section loading, but the section is calling the open() method of the page status dialog. I assume the error occurs there and somehow gets caught by the error boundary and piped to the section component again and displayed there. The sections actually display all errors with a error.section.notLoaded messages.

I would assume with the new Fiber dialogs, this would fail at least in a different way.

I wouldn't close it without testing. Let me see.

distantnative commented 3 years ago

Ok works better now: With the step-by-step instructions, the section itself fails (as expected), but no other sections are affected:

Screen Shot 2021-07-17 at 22 00 03

Switching to site.findPageOrDraft() fixes the failing section as well (again as expected).

nilshoerrmann commented 3 years ago

@distantnative Does the change status dialog work on the Notes section again in that case, too?

distantnative commented 3 years ago

Using site.findPageOrDraft, yes - keep in mind this isn't a workaround but site.find(x) is a wrong setup if x could become a draft

nilshoerrmann commented 3 years ago

But in this case the actual issue has not been fixed yet, hasn't it? With site.findPageOrDraft the error in the section is gone, yes – that's the fix for the misconfigured sections and without that error the dialogs will work again.

But in the case of misconfigured sections, the user will still not be able to change the page status back to listed. The expected behaviour is the following in my eyes:

  1. The user changes the Notes status to unlisted.
  2. Any misconfigured section will throw an error.
  3. The user is able to change the Notes status again to listed to "fix" his issue.

And the last step is not working yet, am I right? Or did I misread your reply?

distantnative commented 3 years ago

Ok now I understand what you mean. The dialog still shows the error with a misconfigured blueprint, yes.

I'll try to dig a bit deeper.