getkirby / kirby

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

Unexpected issue with sorting on setting the num prop when creating pages in code. #6443

Open SeriousKen opened 4 months ago

SeriousKen commented 4 months ago

Description

The following two pieces of code do not produce the same results:

<?php

use Kirby\Cms\Page;

require __DIR__ . '/vendor/autoload.php';

$faker = \Faker\Factory::create();
$faker->seed(29670);

$kirby = new \Kirby\Cms\App();
$kirby->impersonate('kirby');

for ($i = 1; $i <= 10; $i++) {
    $page = Page::create([
        'slug'     => join('-', $faker->words(5)),
        'draft'    => false,
    ])->changeStatus('listed', 1);
}
<?php

use Kirby\Cms\Page;

require __DIR__ . '/vendor/autoload.php';

$faker = \Faker\Factory::create();
$faker->seed(29670);

$kirby = new \Kirby\Cms\App();
$kirby->impersonate('kirby');

for ($i = 1; $i <= 10; $i++) {
    $page = Page::create([
        'slug'     => join('-', $faker->words(5)),
        'draft'    => false,
        'num'      => 1,
    ]);
}

Expected behavior

I would expect both pieces of code to create a series of pages, inserting each page with sort number of 1 so the pages should be in the reverse order they were created.

The first one works, the second prefixes all pages with 1. Which is odd, because the last thing the create method does is call changeStatus('listed', $props['num']) before returning the page object.

I presume that setting num on the array is doing something internally that is causing an issue when calling the changeStatus method before returning the object.

Screenshots

To reproduce

  1. Create a project from plainkit
  2. Create two php files in the root, on for each for the above code.
  3. Run one file on the CLI and observe the results
  4. Clean up and run the other one

Your setup

Kirby Version

Kirby 4.2.0

Console output

Your system (please complete the following information)

Additional context

SeriousKen commented 4 months ago

Figured out where the difference occurs - line 281 in the PageActions trait. When setting the num prop directly the logic says that it doesn't need sorting.

https://github.com/getkirby/kirby/blob/c1e6ff77b056774543f201035f5dab6a02bfdb3b/src/Cms/PageActions.php#L280-L283

afbora commented 4 months ago

I have not an idea about the fixing the issue but as workaround you can use 'num' => 0 to create pages with reverse order.

SeriousKen commented 4 months ago

@afbora yes, setting num to 0 does work but I just needed some example code to highlight the issue. Something needs fixing somewhere, but not sure if it is code or documentation. There is an inconsistency in using num in the props vs setting it after creation with changeStatus, and I think changeStatus after creation is probably the more understandable way, but num is documented as a prop and the difference in the behaviour is not obvious.

SeriousKen commented 4 months ago

Seems that setting draft to false in conjuction with setting num is causing the issue, setting num doesn't need draft as false anyway as it sets the status to listed so should there be a warning in the docs about not using both settings together?

philipmarnef commented 3 months ago

I ran into the same issue today trying to create a number of pages sorted by publishing date. I feel the draft property should just be ignored when num is present.

distantnative commented 2 months ago

@philipmarnef when you are saying "the same issue", you mean the draft vs. num competition, right? Not the passing num directly as prop vs. calling the method afterwards?

@SeriousKen I think the consideration here is that passing num as prop should be taken more literally (setting it directly) as when called via the method.

philipmarnef commented 2 months ago

@distantnative I tried setting both 'draft' => false and 'num' => [...] in Page::create(). That doesn't work, you can only set num in that case. It does make sense because a page can't have a num prop and be a draft at the same time, but if draft is set to false maybe it can be ignored since those two props aren't strictly contradictory in that case.