sortBy('sort', 'desc') from template re-orders content

[maxackerman]

Describe the bug
using ->sortBy('sort', 'desc') in a template on a collection of pages causes them to be re-numbered in the content folder.

Example code:

<?php foreach ($site->children()->listed()->sortBy('sort', 'desc') as $p) : ?>
  <h2><?= $p->title()->html()?></h2>
<?php endforeach ?>

I realize this is not the correct way to change the order of the pages, but the result seems like a bug that should be fixed.

To Reproduce
Steps to reproduce the behavior:

1. download example of the bug added to the plainkit here: https://github.com/maxackerman/plainkit
2. create a user and log in
3. vist the home page in your browser
4. refresh to see the order change, and look at folders in /content

Expected behavior
Template code should not change the content folder

Screenshots

Kirby Version
3.3.4

[afbora]

In the accidental use, the entire sorting will lost and if it is a user who is not logged in, gets an error.

This issue about conflicting Collection::getAttributeFromObject() and PageActions::sort()method on ->sortBy('sort') requests.

https://github.com/getkirby/kirby/blob/3.3.4/src/Cms/PageActions.php#L725-L728
https://github.com/getkirby/kirby/blob/3.3.4/src/Toolkit/Collection.php#L476-L479

[afbora]

@lukasbestle Do you have any idea or solution on this issue?

[lukasbestle]

To be honest, I think that this is quite hard to fix in general. It's intended behavior that sortBy() calls methods on the objects that are being sorted and it's also intended behavior that $page->sort() sorts the page.

What we could do is to introduce a class-based blacklist (like Page::$sortingBlacklist) where we would list all methods that must not be called when the object is being sorted. But we would need that for all model classes and it would get pretty complex and hard to maintain.
Or we could block all writes during the sort to make absolutely sure that no write method is called by accident. But this is again pretty complex.

So I'm not sure if we should rather add a warning to the docs that sortBy() should only be used for fields that aren't action methods.

[distantnative]

I think the main issue isn't so much that sortBy() calls the methods, but that the naming of sort() is so ambiguous. Maybe it could be changed to changeSort()?

[texnixe]

While this is a lot easier, the problem is that people tend not to read the docs or even understand what that means before the damage is done 😉 .

+1 to change the name of the method to changeSort().

[afbora]

@distantnative I was testing it too 👍 Because I think it is confused by users (including me) whether it is a noun or a verb 🤔

[afbora]

I've done a few tests, it looks great for now 🎉

public function changeSort($position = null)
{
    return $this->changeStatus('listed', $position);
}

/**
 * @deprecated 3.3.5 Use `Page::num()` instead
 *
 * @return int|null
 */
public function sort($position = null)
{
    deprecated('$page->sort() is deprecated, use $page->num() instead. $page->sort() will be removed in Kirby 3.5.0.');

    return $this->num();
}

[afbora]

We will also need to prepare a migration in code for users using the sort permission.

[lukasbestle]

You are right! I agree that we should get rid of the sort() method somehow. The issue is: We have kind of a mess in the PageActions trait right now. There already is changeNum(), which expects a "final" sorting number. sort() on the other hand is a shorthand for changeStatusToListed(). Renaming sort() to changeSort() would be very confusing as we would then have changeNum() and changeSort() side by side.

So maybe we should instead completely deprecate the sort() shorthand without any replacement. It's only a shorthand after all.

@afbora Regarding deprecation there are two things to keep in mind in general: Deprecations should ideally be done in minor versions (i.e. 3.4.0, not 3.3.5), otherwise there will suddenly be warnings for patch updates. Also the deprecation must point to the new method, which would be changeSort() in your example, not num() – otherwise the deprecation will be a breaking-change in itself.

Regarding the sort permission: We could rename that to changeNum to be consistent. But I think it's not as urgent as removing the sort() method. Also the permission name sort makes sense in the Panel context as the permission not only applies to explicit number changes, but also drag-and-drop sorting.

[afbora]

@lukasbestle I appreciate you to explain any subject with detailed tips and tricks. I learn a lot thanks to you 👍

Btw can you label the issue? Is this a bug or enhancement? 😅

[manuelmoreale]

I guess this is also an issue:

sortBy('sort') used to be a legit way to sort using the manual sorting attribute.

[distantnative]

@manuelmoreale that is for files, not pages

[manuelmoreale]

@distantnative I know but it's still confusing because of muscle memory 😄

[manuelmoreale]

Maybe it's a personal option but I find it confusing to have code written the same way that does different things in different contexts.

In this example for example, coming from K2, ->sortBy('sort') was a legit way to sort files by the manual sorting number so it's intuitive to expect ->sortBy('sort') to sort the pages by their respective sorting number.

[distantnative]

I agree, @manuelmoreale - it's a bit unfortunate that for pages it's called num while for files it's called sort.

Considering that, maybe we should proceed as suggested above, but instead of getting rid of $page->sort() completely, we switch it over to be an alias for $page->num().

I know this is a bit tricky since it is a breaking change, but I think it might be a good chance to unify the experience a bit (as @manuelmoreale pointed towards).

[lukasbestle]

I agree that it makes sense to unify the two classes in this regard. If we decide to do it, we should however first deprecate the current sort() method and then replace it with the num() alias later.

The question is: Wouldn't it make more sense to rename $file->sort() to $file->num() instead? In the long term I think that num() is the more logical term. However as $file->sort() comes from the content field, we would need to rename that content field as well. So it's not easy to change and therefore not optimal either.

[distantnative]

Not sure if first deprecating and then lat reintroducing it is really viable - sounds like a year-long timeframe to me.

And I wouldn't touch actual content.

[lukasbestle]

Not sure if first deprecating and then lat reintroducing it is really viable - sounds like a year-long timeframe to me.

Yes, that's true. To be honest, that's a reason for not doing it at all. Simply replacing the method in one go is not viable either.

What we could do instead: Deprecating $page->sort() entirely and introducing a new $file->num() alias for $file->sort(). What do you think?

[distantnative]

Simply replacing the method in one go is not viable either.

For sure it wouldn’t be ideal, but we have other breaking changes in 3.x releases too. Since it is a change towards a method that only returns something (not altering data or even content files), there wouldn’t be any danger of data loss or so.

Having the file alias method could be a way. But since $file->sort() continues to exist, people will continue to use that and thus expect a $page->sort().

[lukasbestle]

Since it is a change towards a method that only returns something (not altering data or even content files), there wouldn’t be any danger of data loss or so.

That's true. :)

But since $file->sort() continues to exist, people will continue to use that and thus expect a $page->sort().

The question is which method makes more sense in the long term. Having aliases in both ways ($page->sort() and $file->num()) isn't viable as that's going to be very confusing.

So in my opinion we should just offer $file->num() and change all docs to that method so that $file->sort() isn't as prominently displayed anymore. sort() is just too ambiguous.

[manuelmoreale]

sort() is more intuitive IMO. num() is a bit vague. It makes sense for people that are used to Kirby but for new users it might create some confusion.

That said, I'd intuitively think at the sort() method as a shorthand for sortBy().
So doing this $pages->sort('desc') would simply sort the content by the sorting number when present or alphabetical as a fallback, in descending order. Similar to flip() but a bit more specific.

Having sort() to modify the content and sortBy() to just sorting is IMO confusing.

sort and sortBy should behave similarly to filter and filterBy.

But as you both said, it's tricky to now change the behaviour of those methods.

[lukasbestle]

sort() is more intuitive IMO. num() is a bit vague.

Interesting! I would have thought sort() can be more confusing because it's not clear whether it's a getter or setter. But maybe the name num() only makes sense if the user knows about the page numbering system and the numbers in the directory names.

sort and sortBy should behave similarly to filter and filterBy.

In this issue we are discussing $page->sort() (for a single page object), not $pages->sort(). I like that idea, however that's something for a separate idea issue (I'm also not sure if it's viable, but that's also something to be discussed separately).

[manuelmoreale]

Interesting! I would have thought sort() can be more confusing because it's not clear whether it's a getter or setter.

O yes I was just comparing sort() and num() in the abstract. sort() is still confusing as a method in itself. But in general, if I read sort() I'm expecting something sorting related but if I read num() I honestly don't know what to expect.

In this issue we are discussing $page->sort() (for a single page object), not $pages->sort()

Totally. What I was referring to is the consistency of naming convention in general. I was using filter and filterBy as example because those are two methods with similar names that do similar things. But then we have sort and sortBy that do completely different things.

Anyway, to go back to the original topic:

I'm personally in favour of using changeSort since changeXYZ is a convention already in used inside kirby. The question is, what's then the difference between changeSort and changeNum?

[lukasbestle]

I'm personally in favour of using changeSort since changeXYZ is a convention already in used inside kirby. The question is, what's then the difference between changeSort and changeNum?

That's exactly the reason why I suggested to drop the sort() method with its current behavior completely as we already have changeStatusToListed(), which does exactly the same thing internally – sort() is only a shorthand for changeStatusToListed().

[distantnative]

New thought:
->sortBy() is dangerous whenever the provided filename is actually a method that alters or removes content. sort() is just one example.
Couldn't we run the call to read the field via $kirby->impersonate with a nobody user with no permissions? Then any method call that isn't a field but a method like sort() or so would fail.

The other aspect of getting the names between $page and $file more consistent remains important.
Since sort: is written to a file's content, I think we have to stick with that one. Having $file->num() but then inside the content file sort: seems confusing as well.

So ideally I'd suggest that $page->sort() is turned into an alias for $page->num(). Which sadly is a big breaking change. So I don't know how we can do this.

In addition, maybe $page->changeSort is added as alias for $page->changeStatusToListed().

[lukasbestle]

Couldn't we run the call to read the field via $kirby->impersonate with a nobody user with no permissions?

👍

However running each single call through $kirby->impersonate() is going to be quite slow. Simpler: A Kirby\Cms\Collection::sortBy() method that wraps the whole sorting process in $kirby->impersonate().

So ideally I'd suggest that $page->sort() is turned into an alias for $page->num().

Do we really need consistency here? $file->sort() is not a file method, it's just the field. So it doesn't seem entirely necessary to introduce the $page->sort() alias, at least not for now.

I'm all in for introducing that later, but not with a breaking-change. Better first deprecate the existing $page->sort() method in 3.5 and then introduce the new alias in 3.6.

[distantnative]

• Kirby\Cms\Collection::sortBy() that wraps the sortBy call in a $kirby->impersonate() with no permissions
• Mark $page->sort() as deprecated in 3.5
• Add $page->changeSort() as public alias for $page->changeStatusToListed()

[distantnative]

@lukasbestle as far as I can see we did implement the solutions discussed here with 3.6.0-alpha - do you see anything missing?

[lukasbestle]

As far as I can tell, we solved this in 3.5 already.