Changes 1: Refactor content storage to use new VersionId class

[bastianallgeier]

This PR …

This is the first bigger step in a series of PRs that will hopefully lead to our new changes system one day :)

Refactored

• New Kirby\Content\VersionId class to represent versions
• Refactored all content storage classes and models to use the new VersionId.

Breaking changes

• All content storage methods must now use the VersionId instead of a simple string.

Reason

With the new VersionId class, we can reduce quite some logic and get proper type hinting here. This feels like a much stronger/clearer path for already complex classes and methods.

Code coverage

We have a minor reduction in code coverage, but it's only in the ContentStorage class which will be gone in a later step anyway. That's why I think we should ignore it.

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]

Is an enum a good choice here if later on we might want to support dynamic IDs (revisions)?

[bastianallgeier]

After digging more into git I really keep wondering if we should ever have revisions as additional versions or if it would make more sense to have published and changes and then there could be various iterations of the changes version in git.

Alternatively, I was also thinking that we could use the enum with a predefined set of slots.

VersionId::CHANGES_1
VersionId::CHANGES_2
VersionId::CHANGES_3
VersionId::CHANGES_4

Having a system that has an unlimited amount of changes seems pretty scary to me. But no matter what we go for, having the class hint in there already will make another round of refactoring 10 times easier.

[distantnative]

I think the whole idea around revisions is not to have to engage with gut for normal people. And even with git it makes handling versions tied to a specific page difficult.

But totally fine with going this way now and also predefined amounts/names sound like it could be a viable way.

[bastianallgeier]

Maybe @lukasbestle should have another look at it. We could totally go for VersionId::published() and VersionId::changes() as values as well, which would return a VersionId instance. But I quite like the enums :)

[lukasbestle]

We are going full circle here. 😄
In a previous version of the concept, we had a VersionIdentifier class and replaced it with strings for simplicity. I'm in favor of more type safety, so I support the idea to replace the strings with a more specific type again.

Regarding the revisions, I agree with Nico though.

IMO we would definitely want to use Git for revisions to avoid reinventing the wheel. Git is really great at what it does and we could never implement revisions better ourselves.

But independent of the revision backend (Git in this case), we still need a PHP API to access a specific revision like so:

$model->content($lang)->revision($id)

I can't really see limiting ourselves to a finite number of revisions TBH. If content of a model was modified often, having no access to older changes would make the feature a lot less useful. Also, the REVISION_1, REVISION_2... versions would have to be sorted somehow and that sorting will change whenever a new revision is created or an older one is deleted. This could lead to race-condition situations where a user selects a specific revision in the Panel but because we cannot identify it by commit hash, the backend will have to select "the second one" and that can be a different one than the user intended by the time the request reaches the server.

So IMO we should go for a VersionId class that can later also keep the revision ID.

[distantnative]

IMO we would definitely want to use Git for revisions to avoid reinventing the wheel. Git is really great at what it does and we could never implement revisions better ourselves.

Our revisions feature would be quite limited in terms of audience then. I really think, if we want to ship a revisions feature, git has to be optional. Even more cause I think the vast majority of asking for it are those who would not manage to run git on their server. If they could, they would less ask for this feature.

So IMO we should go for a VersionId class that can later also keep the revision ID.

I still think passing a whole $this->handler->create(VersionId::published()) is a bit bulky, out of the usual in Kirby and not easy to understand: What's that method call? Why aren't we passing a value? etc.

I think we should then stick to simple values and resolve them to a VersionId instance (or similar) internally. But not as requirement to blow up the method calls.

[bastianallgeier]

We are definitely going full circle but I think this is fine. It's partly a matter of another year of learning, of being less under pressure and more deeply immersed again.

The content storage handlers must definitely get fully typed arguments. A next step in my list is to always pass a full language object. Letting the handlers deal with additional argument resolvers is not a good idea and I don't worry about passing more bulky args here. It's a layer that's mostly internal. We can still resolve string values one level up in model methods to keep the Kirbyesque simplicity. The added logic of working with simpler arguments is often getting us in trouble in other parts of the content mess and I believe that we need to get this right and make use of new PHP awesomeness to let us help with the chaos here.

I agree about the points to keep options for revisions more open and will turn the enum into a full class.

[afbora]

@bastianallgeier I think we should also update following lines, right?
https://github.com/getkirby/kirby/blob/v5/changes/version-id/src/Cms/PageActions.php#L474-L475

[bastianallgeier]

@afbora yikes, this should have been caught by a unit test somewhere, but it didn't. I just fixed it.

[afbora]

Just an idea: I realized that some checks repeats (3 places) and I think they are reusable with new VersionId::for() method or something like that.

class VersionId implements Stringable
{	
  public static function for($model): static
  {
    if ($model instanceof Page === true && $model->isDraft() === true) {
      return static::changes();
    }

    return static::published();
  }
}

[bastianallgeier]

@afbora that's a great idea and a much better place than the ContentStorage class. I've just added it.

[distantnative]

Looks good to me, but I think @lukasbestle will do the final review since you're both a bit "deeper" into the matter.

[bastianallgeier]

I wonder in general how we should move forward with my PRs. It's basically just a transitional state and I try to break it down step by step, but things will change more over the next few PRs.

[lukasbestle]

Very awesome, love the idea to use the class constants for the allowed IDs. Makes the syntax really clean and feels like an enum with added features. 👍

During review I came across two minor things, the rest is great and ready to merge.