Changes 2: Plain Text Handler Refactoring

[bastianallgeier]

This PR …

• 🚨 Merge first: Changes 1: Refactor content storage to use new VersionId class #6436

Refactoring

• New internal PlainTextContentStorageHandler::write method
• New contentFile methods to split logic per model type and make use of type hinting
• Remove option to pass null as language to ContentStorageHandler::exists to avoid unwanted logic in handler methods.
• Alphabetically sorted methods

Outlook

• All $lang arguments in handler methods will be converted to full Language objects instead of strings in a later PR. I did not solve all of this at once to keep the PR more straight forward. Changes 4: Refactor PlainTextContentStorageHandler to use Language object #6448
• The PlainTextContentStorageHandler class will no longer implement the interface but extend the abstract ContentStorageHandler class. This change will also remove the __construct method, which will then be implemented by the abstract. Changes 5: ContentStorageHandler Abstract #6449
• The new ::write method will later be moved to the ContentStorageHandler class as an abstract method. ContentStorageHandler will then also get a non-abstract update and create method, which will use the new write method by default. This will add the option for storage handlers to only modify the write method if no further logic is needed for create and update. Changes 10: New MemoryContentStorageHandler #6457

Breaking changes

None

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 and we don't need the use case anymore that ::exists() can check whether any language file exists?

[bastianallgeier]

We don't have that case anywhere and if we should ever have it, it should happen one level up. Just imagine that you would want to create your own storage handler one day and you need to implement that kind of logic in your own handler instead of simply defining a call to a single source.

Keep in mind that we have the version class as a layer between model and storage handler anyway. Additional logic should happen there or in the model actions in my opinion.

[distantnative]

Was just asking, not making an argument for keeping it :)

[lukasbestle]

The changes in this PR look good.

However I'm also not sure about the removal of language-independent exists(). It was intended for checks of "does the version exist at all". This can then be used for $model->version() to immediately return null.

You are right that it adds complexity to the handler, but it allows the handler to optimize for performance. E.g. a database handler would only have to send a single database query no matter if there is one language or 10. If we implement it in the ContentStorage/Version class, we would still have to loop through all languages there.

Compared to that performance opportunity, I think the added complexity is justified.

[bastianallgeier]

@lukasbestle that's a good point. I wonder though if we should already optimize for that future case or if we keep it as simple as possible for now and then maybe add it later if we really run into an issue.

[lukasbestle]

My thoughts were around the breaking change that such a later introduction would cause. But as the whole storage interface/abstract is currently internal, we don't really need to worry about that. I just feel like we will forget about it later and not think about reintroducing it when we need it. Since it's just a few lines, I'd vote to keep it. But I'd also be fine to remove it if it makes other stuff simpler down the road.

[bastianallgeier]

With the new ContentStorageHandler abstract, I would vote to move this into its own method. Then a handler can still optimize it, but we can keep it separate to avoid the ambigous logic. In a later step, I already moved over deleteLanguage and touchLanguage from the ContentStorage class and I think adding more specific exist methods would really make sense.

[lukasbestle]

As discussed on Discord, let's leave the nullable language param for exists() out for now. We can still reintroduce it later.

I've reviewed the code of the entire PR now. Nice refactoring work! Just one minor detail left: