-
-
Notifications
You must be signed in to change notification settings - Fork 166
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 3: New Version class #6442
Conversation
80b56cc
to
e884d2b
Compare
d62afaf
to
5bcc7ef
Compare
5bcc7ef
to
d01a480
Compare
d01a480
to
a9f038d
Compare
*/ | ||
public function content(string $language = 'default'): Content | ||
{ | ||
return new Content( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume Version
will later cache the content object for each language, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly. But this isn't implemented in the other PRs so far. It's only on the todo list in Notion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added more context and links to our todo list for later. I agree it makes sense to merge this as is to avoid merge conflicts and endless rebasing. The class is not fully there yet, but we can and will improve it later in the process.
Added some comments on the tests. Same applies here: You don't have to change anything right now. I'd also be totally fine with merging the PR and adding those to the todo list for later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Could you add it to the todo list please?
This PR β¦
Refactoring
Version
class which inherits most of the logic from theContentStorage
handler and will eventually replace it.Outlook
Version
class and add additional unit tests (Changes 6: Version class improvements and unit testsΒ #6450) The steps are separated to make sure that all other unit tests always keep passing. It's sometimes a bit of a chicken and egg situation here.ContentStorageHandler
methods accept aLanguage
object instead of a string (Changes 4: Refactor PlainTextContentStorageHandler to use Language objectΒ #6448) theVersion
class also needs to pass aLanguage
object instead of language codes. It will therefor get a new protectedVersion::language()
method that is mostly adapted fromContentStorage::language()
but will always return aLanguage
object - even in a single-language installation. There's a newLanguage::single()
method coming up in Changes 4: Refactor PlainTextContentStorageHandler to use Language objectΒ #6448 for that.Version
class will also inherit a few more specific logic methods fromContentStorage
in preparation of removingContentStorage
entirely::deleteLanguage
::touchLanguage
::contentFile
Version::modified
will later return null if the version does not exist.Version::ensure
will later useContentStorageHandler::ensure
Changes 10: New MemoryContentStorageHandlerΒ #6457Breaking changes
None
Ready?
For review team