Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes 4: Refactor PlainTextContentStorageHandler to use Language object #6448

Merged
merged 10 commits into from
Jun 10, 2024

Conversation

bastianallgeier
Copy link
Member

@bastianallgeier bastianallgeier commented May 17, 2024

This PR …

Refactoring

  • New Language::single() method to create a Language placeholder object in single language installations
  • Use full language objects in ContentStorageHandler and PlainTextContentStorageHandler methods
  • Refactor ContentStorage to always provide a full language object to the storage methods. This is only temporary to keep all the unit tests working. ContentStorage will be removed later, as planned.

Outlook

  • 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

@bastianallgeier bastianallgeier force-pushed the v5/changes/language-type-hinting branch from bc9e8cb to 8ab215a Compare June 5, 2024 09:04
src/Cms/Language.php Show resolved Hide resolved
src/Content/ContentStorage.php Show resolved Hide resolved
src/Content/PlainTextContentStorageHandler.php Outdated Show resolved Hide resolved
tests/Content/ContentTranslationTest.php Show resolved Hide resolved
bastianallgeier and others added 3 commits June 5, 2024 19:18
Co-authored-by: Lukas Bestle <lukas@getkirby.com>
Co-authored-by: Lukas Bestle <lukas@getkirby.com>
Copy link
Member

@lukasbestle lukasbestle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That all looks very sensible now. 👍
The ContentStorage class is in a weird limbo now, but as it will get replaced anyway quite soon, it doesn't really matter.

@lukasbestle lukasbestle merged commit 8c8dcbb into v5/develop Jun 10, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants