rasteiner / k3-pagesdisplay-section

K3 plugin: display any page list in a section. Any parent, many parents, filtered, don't care.
Other
45 stars 8 forks source link

Allow using pagesdisplay in user and file blueprints #21

Closed moevbiz closed 1 year ago

moevbiz commented 1 year ago

The native pages section can only be used on page and site blueprints, not on user or file blueprints. As far as I can tell, there is no reason to prevent using pagesdisplay everywhere, i.e on a user view to show their latest contributions. But in this case a query value needs to be present. https://github.com/getkirby/kirby/blob/3.8.3/config/sections/pages.php#L54

rasteiner commented 1 year ago

Thank you for your time :)

The parent property is, afaik, only used in the original pages section to:

Since the errors that it throws, like you say, don't really apply to pagesdisplay (there's no reason you shouldn't be able to use the display on users and files), I wonder if it wouldn't be "simpler" to override it with a function that just always returns null:

[
    'parent' => fn() => null,
    //snip
]

Then replace the page.children "magic value" as default for the query prop also with a null, and only in pages method replace that null with an appropriate default query depending on the type of the parent model. One would also need to change the context variables passed to the query (replace page with either user or file).

That's why I didn't merge for now. I have some experiments to do first...

moevbiz commented 1 year ago

Thank you for the plugin, it's much appreciated! :~)

You're totally right with everything. I thought it was a simple fix, but didn't notice before that I have set the query for the section on my user blueprint as page.myCustomMethod instead of user.myCustomMethod... Which works! But shouldn't. Did a little more experimenting and it seems like the overridden parent still needs to return a model for this one to work out, but will look deeper when I have time.

moevbiz commented 1 year ago

I can't come up with a default query that would make sense on user or file blueprints – and I doubt that the section would be expected to work here with no query prop set.

Updated proposition; the query bit can be changed like this:

'pages' => function () {
    // ...
    $model = $this->model();
    $q = new Query($this->query ?? 'page.children', [
        'kirby' => $kirby,
        'site' => $kirby->site(),
        'pages' => $kirby->site()->pages(),
        'page' => $model instanceof \Kirby\Cms\Page ? $model : null,
        'user' => $model instanceof \Kirby\Cms\User ? $model : null,
        'file' => $model instanceof \Kirby\Cms\File ? $model : null,
    ]);

    // ...
}

The query prop function can be entirely removed from the extension as it is already implemented in the normal pages section, and the parent function updated to this:

'parent' => fn() => $this->parentModel(),

As far as I can tell (needs more testing) this should be enough to work.

If no query is present in the blueprint on a user or file model it will run, but throw the error: Access to method/property xyz on null. If you want nicer error handling, the parent function could stay similar:

'parent' => function () {
    $parent = $this->parentModel();
    if (
        $parent instanceof \Kirby\Cms\File === false &&
        $parent instanceof \Kirby\Cms\User === false &&
        !$this->query
    ) {
        throw new InvalidArgumentException("You must provide a query when using pagesdisplay in a user or file blueprint.");
    }
    return $parent;
},

...or simply do without setting any default and throw an error if query is not set. Since this issue hadn't come up before, I personally don't feel like the use case justifies a complex rework of the plugin, so it's up to you :~) Let me know if you'd like me to create another PR.

rasteiner commented 1 year ago

Thank you very much for your efforts :)