Changes 13: New translation classes

[bastianallgeier]

This PR …

• 🚨 Merge first: Changes 1: Refactor content storage to use new VersionId class #6436
• 🚨 Merge first: Changes 2: Plain Text Handler Refactoring #6439
• 🚨 Merge first: Changes 3: New Version class #6442
• 🚨 Merge first: Changes 4: Refactor PlainTextContentStorageHandler to use Language object #6448
• 🚨 Merge first: Changes 5: ContentStorageHandler Abstract #6449
• 🚨 Merge first: Changes 6: Version class improvements and unit tests #6450
• 🚨 Merge first: Changes 7: Move ContentStorage methods to ContentStorageHandler #6454
• 🚨 Merge first: Changes 8: New ModelWithContent::version method #6455
• 🚨 Merge first: Changes 9: Remove content storage class #6456
• 🚨 Merge first: Changes 10: New MemoryContentStorageHandler #6457
• 🚨 Merge first: Changes 11: Language::ensure & Languages::ensure #6483
• 🚨 Merge first: Changes 12: More Version class improvements #6490

Reasoning

The ContentTranslation class is one of our internal classes that still uses array props in the constructor and also has some logic that shouldn't be in there. To untangle the translation knot, it really helps to refactor this class to named properties and turn most of its methods into proxies to access the underlying version or language objects.

In order to turn this into a smooth transition, I've decided to extend the ContentTranslation class for now. This way, we can use it as a replacement for ContentTranslation in many parts already before we replace it later. We can also change methods more freely without affecting the CMS Models and their unit tests too drastically for now.

Features

• New Kirby\Content\Translation class (to eventually replace Kirby\Content\ContentTranslation)
• New Kirby\Content\Translations class to help in ModelWithContent::translations later and to provide a more type-safe collection for all translations.

The following changes to Translation will be new features as soon as it replaces the ContentTranslation class.

• New Translation::version method to return the parent Version instance
• New Translation::create method to setup a new translation from an array of fields.

Deprecated

• Translation::code Use ::language()->code() instead
• Translation::content Use ::version()->content()->toArray() instead
• Translation::contentFile Use ::version()->contentFile() instead
• Translation::exists Use ::version()->exists() instead
• Translation::isDefault Use ::language()->isDefault() instead

Breaking changes

The following changes to the Translation class will be breaking changes as soon as it will replace the ContentTranslation class.

• Translation::parent has been replace with Translation::model
• Translation::update has been removed

Code coverage

No matter what I do, Translations::load is simply not covered and I don't know why.

Ready?

• In-code documentation (wherever needed)
• Unit tests for fixed bug/feature
• Tests and checks all pass

For review team

• Add changes & docs to release notes draft in Notion

[distantnative]

@bastianallgeier regarding your code coverage: none of your test methods says that it covers the load method. So even if the code does, it won't be reported as the test is limit to only cover the methods mentioned (and none mentions load).

[bastianallgeier]

@distantnative wait, am I missing something? https://github.com/getkirby/kirby/pull/6491/files#diff-be3909337c2195af14e3332b8945acaa88ae6801f3b6f92f4fa40343240c0747R111 I really think I'm going crazy here. I even tried to re-type that @Covers part in case I had an invisible character in it or some other typo that I just don't see.

[distantnative]

@bastianallgeier sorry, my bad, mistook Translations::load() for Translation::load()

[distantnative]

@bastianallgeier it seems tied to the method name. When I rename the method to loadd, it shows up fully covered.

Question: What is the reason for using the deprecated methods in the tests? https://github.com/getkirby/kirby/pull/6491/files#diff-ee064be1fccc6c3b78a97760793cd868737cdcf4273b70b1b9e61e189975b45aR63 failed for me when I replaced all deprecated methods with their suggested alternatives.

[bastianallgeier]

So weird about the name! We could try what happens when we add a coverage ignore rule.

About the deprecated methods. We don't need to be that strict. My thought behind is that the translation class really acts more like glue for us between language, version and content. To repeat a lot of the version methods would be quite redundant. That's why I think we should encourage to prefer going through the versions. But that's totally up for discussions.

Did you specify the language when you used the alternative?

[distantnative]

I'm for the depreciations. But we should adapt the tests then, shouldn't we? Currently the CI throws all the deprecation warnings.

[bastianallgeier]

I have to be honest, I don't really know how to treat the depredations in unit tests so far

[distantnative]

@bastianallgeier I am talking about replacing the deprecated code. Currently the test still use all the deprecated methods, e.g.

$this->assertSame('en', $translationEN->code());

should be changed to

$this->assertSame('en', $translationEN->language()->code());

etc.

[bastianallgeier]

@distantnative I've replaced all deprecated methods in the unit tests.

[lukasbestle]

Two methods of ContentTranslation that Translation does not implement anymore (and doesn't need to) are parent() and update(). In order to make the transition smoother, we need to find bugs early where we maybe still rely on these methods. So I suggest to add overrides for these two methods to the Translation class that throw an exception.

With that I think we can finally merge this.