fix(#94): Update page using latest version in clean command

[afbora]

I think @morja's issue in #94 (comment) here about related with new versions system. I've used ->version('latest') and seems it's working. To be honest I'm not sure it's correct solution or not. What do you think about that? @bastianallgeier @lukasbestle @distantnative

Fixes #94

[morja]

@afbora When I run this code I get an error in my multi language project.
The cleanup fails with: Version "latest (de)" does not already exist.

I do not have translated versions for most files yet...

I removed this comment because first I thought I made a mistake. But it still appears. If site.en.txt has changes, I do get the error mentioned above. If another one has changed, it works.

[afbora]

@morja I tried your steps and I can't still reproduce the issue. Works for me great.

[morja]

@afbora I created a test with multiple variations and they confirm the report I made before when run without the ->version("latest") . With the ->version("latest"), most of them fail. as I only created single language content for multi language sites. But also the single language seems to fail (I did not dig deeper)

With my real project, if I use ->version("latest") then it fails when I have no de version for a changed en file: Version "latest (de)" does not already exist.

Reproduce:

• Setup Kirby in multi language with a second language e.g. de
• Create a 1_home/default.en.txt but no de version
• Add some obsolete fields
• Run the clean command

With the version without the ->version("latest") I still get the issue mentioned before: if I have a page with obsolete fields, but I do not add obsolete fields to the site, it deletes that page file with the error Storage for the page is immutable and cannot be updated. Make sure to use the last alteration of the object..

[morja]

Here is my test file: https://gitlab.com/-/snippets/4905286
I wasnt able to create a gist here on github for some reason...

For the test to run, you'll need to add and install kirby via composer

	"require-dev": {
		"getkirby/cms": "^5.0"
	},

I then call it like this: phpunit tests/CLI/commands/CleanContentCommandTest.php

Not sure if this is the way to do it. Just wanted to get it running quickly...

[afbora]

@morja Thank you for great helping to tracking the issue. I've one more shot. Could you test it, please? 🙏

[morja]

@afbora thanks for this version. It now works for the multilanguage tests, but fails for single language site setup.
I also tried

if ($version->exists($lang) === true) {
   $version->update($data, $lang);
} else {
   $item->update($data, $lang);
}

but this also fails.

[morja]

I found a solution that works for me:

if ($lang === null) {
   // single language site, update data without language
   $item->update($data);
} else {
   // multi language site
   $version = $item->version('latest');
   // check if the version exists for the given language
   // and try to update the page with the data
   if ($version->exists($lang) === true) {
	  $version->update($data, $lang);
   }
}

[afbora]

I have to ask @bastianallgeier here. Should we still use the version system in a single language?

[morja]

This also works for me:

    $version = $item->version('latest');
	if ($lang === null) {
		// check if the version exists without a language
		// and try to update the page with the data
		if ($version->exists() === true) {
			$version->update($data);
		}
	} else {
		// check if the version exists for the given language
		// and try to update the page with the data
		if ($version->exists($lang) === true) {
			$version->update($data, $lang);
		}
	}

[morja]

What also works is:

    $version = $item->version('latest');
	// check if the version exists for the given language
	// and try to update the page with the data
	if ($version->exists($lang) === true) {
		$version->update($data, $lang);
	}

and then call the method with default as language instead of null

$cleanContent = function (
	$cli,
	Generator $collection,
	array|null $ignore = null,
	string $lang = 'default'
): void {

[afbora]

@morja According to the core codes, I think you're right 👍I've applied the changes.

[morja]

@afbora this again fails my tests. The exists() method also needs the 'default'. This is why I think it makes sense to define it as default in the cleanContent method. The content() method will in any case use a static en for single language sites as far as I can tell.

[afbora]

Sorry @morja, I missed that. You're completely right 👍I've pushed the commit. @bastianallgeier Ready to review.

[bastianallgeier]

This looks good to me. I've added a couple ideas to another branch and will create a PR In a minute, but this one can definitely be merged.