getkirby / kirby

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

Reordering subpages generates duplicate thumbnails #5102

Closed RomanWu closed 5 months ago

RomanWu commented 1 year ago

Description

Reordering subpages in the panel is resulting in regeneration of all thumbnails of all pages, that received a new sorting number.

To reproduce

  1. Download starterkit.
  2. Visit all the Photography subpages in a browser
  3. Move one of the Photography pages to the first position (in the panel)
  4. Re-visit all the Photography pages
  5. Now all the thumbnails are re-generated with a new folder name.

Expected behavior

The thumbnails should not be regenerated, when reordering subpages.

Screenshots

Here is a screenshot of the waterfall page. The thumbnails were generated once at 10:41 during the first visit. And after the reordering at 10:44 during the second visit.

Thumbnails

Your setup

Kirby Version

Tested in 3.9.1 and 3.9.2. Happened also in previous versions.

Your system

Tested on local server and productive debian systems. Happens with GD or ImageMagick. Happens with PHP 8.1.0, PHP 8.1.13, and also 7.4.33.

Additional context

For multiple Kirby sites we have running, this issue repeatedly brings the according server to its knees. Especially instances, where the template shows a gallery of images of all the subpages. With GD thumbnail creation, the Debian server would regulary be unresponsive. With ImageMagick, the server does not crash, but CPU and Memory usage go to 100% for a few minutes. Needless to say, for these usecases we have, this bug is very severe.

Also, each time this happens, the used storage space is increased dramatically. From the viewpoint of the first new visitor of a page, the page load time is very bad.

We think, that many users might be affected, especially those, where blog-style subpages change frequently.

RomanWu commented 1 year ago

I found the cause myself. I am guessing, that the function kirby/Cms/App/contentToken() should not use $model->root() as a salt, but rather $model->id().

But I have no experience with the kirby source code, so I don't know, if there might be other implications with this change, or if something else might be a better salt.

afbora commented 1 year ago

Thanks for the keep digging!

Yes, the issue is seems related to the use of $model->root() in App::contentToken(). But as far as I see, I don't think using $model->id() will be a solution. For ex: In a multi-site setup, different pages in different content directories with the same ID will return the same token value.

- /content.dev/test/default.txt
- /content.stage/test/default.txt
- /content.prod/test/default.txt

I'm not sure if we can extract num from root correctly. Let's hear from other team members.

RomanWu commented 1 year ago

Thanks. Through my digging I found a good workaround, which solves the problem for us. Adding the following in the config:

'content.salt' => 'somefixedsalt'

distantnative commented 1 year ago

@afbora I'm not really sure if it would be an issue if between different sites in a multi-site setup the tokens could be the same. But if we want to not risk that, would something like $this->root('content') . '/' . $model->id() be an option?

afbora commented 1 year ago

Hmm, then let me ask this; Isn't contentToken has nothing to do with mediaHash directory (like /media/pages/test/3c9deeecd5-1666095575) in the media directory?

lukasbestle commented 1 year ago

The reason why we use the root is to make it a bit harder to guess the hashes when no custom content salt is used (considering that the sorting numbers are often not public, so adding more that needs to be guessed). But I think it's not worth it compared to the performance impact with thumbs that we have not considered so far.

So I think what Nico suggested should work fine.

RomanWu commented 5 months ago

I just tried it again in Kirby 4.2.0 and the problem definitely still exists. Also the workaround with a fixed salt is still working.