getkirby / kirby

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

Pages::factory() ignores individual isDraft statuses #5041

Closed Jayshua closed 1 year ago

Jayshua commented 1 year ago

Description

The documentation for Pages::factory() indicates that the props array for each page can use 'isDraft' to set the draft flag for each page individually. However, Pages.php:178 unconditionally sets the 'isDraft' property for each page array to the value of the $draft parameter, which defaults to false.

This means that all pages created with a single call to the Pages::factory() method will always have the same draft status - false by default.

Expected behavior
Calling Pages::factory() like this:

$pages = Pages::factory([
    [
        'template' => 'default',
        'slug' => 'page-1',
        'content' => ['title' => 'Page 1'],
        'isDraft' => false,
    ], [
        'template' => 'default',
        'slug' => 'page-2',
        'content' => ['title' => 'Page 2'],
        'isDraft' => true,
    ],
]);

Should return a collection with two pages, one being a draft and the other not. Instead both pages are not drafts.

To reproduce

<?php

$pages = Pages::factory([
    [
        'template' => 'default',
        'slug' => 'page-1',
        'content' => ['title' => 'Page 1'],
        'isDraft' => false,
    ], [
        'template' => 'default',
        'slug' => 'page-2',
        'content' => ['title' => 'Page 2'],
        'isDraft' => true,
    ],
]);

var_dump($pages->nth(0)->isDraft());
var_dump($pages->nth(1)->isDraft());

?>

Your setup

Kirby Version 3.8.1.1 & 3.9.1

SeriousKen commented 1 year ago

I'm having this problem (see this support forum post) and I've found the source. The create method in the Page class sets isDraft based on the draft prop which is missing so is always true

https://github.com/getkirby/kirby/blob/617a07aa0151aa4b1a3735b3b5e1a41ccc244a43/src/Cms/PageActions.php#L526

Then later in the create method, the status is only changed if the num prop is set. There is no logic based on the isDraft prop, but even if there was, it will always be overridden with true in the previously highlighted code.

https://github.com/getkirby/kirby/blob/617a07aa0151aa4b1a3735b3b5e1a41ccc244a43/src/Cms/PageActions.php#L563-L566

Jayshua commented 1 year ago

@SeriousKen I believe the isDraft prop on the Page class is normally set when the Page constructor calls $this->setProperties($props). The Properties trait defines that method, which iterates over every declared property on the Page class (including $isDraft) using get_object_vars() and sets them to the value provided in the $props array, or a default value.

Jayshua commented 1 year ago

@SeriousKen Now that I think about it more, I believe your issue is unrelated to this one. You seem to be having problems with the Page::create() method, which internally calls Page::factory() at some point but does a bunch of other work like actually saving the page to disk with Page::writeContent() and setting $props['isDraft'] before passing it along to Page::factory().

This issue is about problems with Pages::factory(), which is a method on the Pages collection class (not the Page class) and never runs the code you mentioned in your comment.

I think the problem in your forum post was because Page::create() expects the field setting the draft flag to be called draft, but you were giving it a field called isDraft instead. I don't know why Page::create() expects the field name to be draft while Page::factory() expects the field name to be isDraft, but it's kind of confusing that it is that way.

SeriousKen commented 1 year ago

@Jayshua

I don't know why Page::create() expects the field name to be draft while Page::factory() expects the field name to be isDraft

In the documentation it is documented as isDraft same as factory but in code it expects draft (not sure why). I thought they were related because the intent is to create a bunch of pages with some being draft and other being published. I believe that the Pages factory passes on page creation to the Page class anyway, so your issue probably does lie within the Page class.

SeriousKen commented 1 year ago

You are right, this isn't the same as my issue. The problematic line in the Kirby is in the Kirby\Cms\Pages class.

https://github.com/getkirby/kirby/blob/617a07aa0151aa4b1a3735b3b5e1a41ccc244a43/src/Cms/Pages.php#L174-L183

I got it to work by changing one line.

foreach ($pages as $props) {
    $props['kirby']   = $kirby;
    $props['parent']  = $parent;
    $props['site']    = $site;
    $props['isDraft'] ??= $draft;

    $page = Page::factory($props);

    $children->data[$page->id()] = $page;
}
distantnative commented 1 year ago