Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Open
SeriousKen opened this issue May 7, 2024 · 7 comments

Comments

@SeriousKen
Copy link
Contributor

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)

  • Device:
  • OS:
  • Browser:
  • Version:
    Linux Ubuntu 20.04 LTS running on WSL2 on Windows 11 Dell laptop

Additional context

@SeriousKen
Copy link
Contributor Author

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.

// don't sort if not necessary
if ($this->status() === 'listed' && $num === $this->num()) {
return $this;
}

@afbora
Copy link
Member

afbora commented May 7, 2024

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

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

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

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

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

@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.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants