-
Notifications
You must be signed in to change notification settings - Fork 45
[FEATURE] Introduce DocumentService for one-time loading of current document #1776
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
base: main
Are you sure you want to change the base?
Conversation
…of document and save to temp
sebastian-meyer
left a comment
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.
Thank you for your contribution and welcome to Kitodo! ;o)
Please see my comments below.
In general, please add (or restore) phpDoc where necessary according to the Coding Guidelines.
Also, maybe I am missing something, but wouldn't the DocumentService have to be a singleton in order for every controller to query the same instance? And how does this work when instantiating multiple documents (like for the BasketController or the new MultiView feature?
…ading for basketController
…n 3dviewController - therefore not removed
…ngua/kitodo-presentation-DFG into Feature_DocumentSerivce
…ngua/kitodo-presentation-DFG into Feature_DocumentSerivce
|
The Service Type is recommended since Typo3 Version 12 and Singletons are considered obsolete this is why we chose this pattern. |
Configuration/Services.yaml
Outdated
| autowire: false | ||
|
|
||
| Kitodo\Dlf\Service\DocumentService: | ||
| public: true |
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.
| public: true | |
| public: true | |
| } | ||
| /** |
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.
| } | |
| /** | |
| } | |
| /** |
| } | ||
| /** |
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.
| } | |
| /** | |
| } | |
| /** |
| } | ||
| protected function reset() |
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.
| } | |
| protected function reset() | |
| } | |
| protected function reset() |
| protected DocumentRepository $documentRepository; | ||
| public function injectDocumentRepository(DocumentRepository $documentRepository): void |
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.
| protected DocumentRepository $documentRepository; | |
| public function injectDocumentRepository(DocumentRepository $documentRepository): void | |
| protected DocumentRepository $documentRepository; | |
| public function injectDocumentRepository(DocumentRepository $documentRepository): void |
| * | ||
| * @param string $documentId The document's UID or URL (id), fallback: record ID (recordId) | ||
| * | ||
| * @param string $documentId |
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.
Why did you remove the full documentation of this parameter?
| public function injectDocumentService(DocumentService $documentService): void | ||
| { | ||
| $this->documentService = $documentService; | ||
| } |
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.
| } | |
| } | |
Classes/Service/DocumentService.php
Outdated
| * @access public | ||
| * @param int $recordId | ||
| * @param array $settings | ||
| * @return ?Document |
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.
Please complete PHPDoc with short description of parameters and method.
Classes/Service/DocumentService.php
Outdated
| * @access public | ||
| * @param string $documentId | ||
| * @param int $recordId | ||
| * @param array $settings |
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.
Please complete PHPDoc with short description of parameters and method.
| } | ||
| /** |
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.
| } | |
| /** | |
| } | |
| /** |
…ngua/kitodo-presentation-DFG into Feature_DocumentSerivce
This pull-request introduces a Document Service for document loading, so that the initialized document object persists across controllers. It adresses the current state issue, that every plugin controller (that implements AbstractController) loads the current document, initializes it and once the controller finishes it is deconstructed.
Every consecutive controller needs to load and initialize the document again.
This solution reuses the loaded document for the whole request and significantly optimizes execution time:
Eventually it is required to flush all cashes in Admin->Maintainance so the dependency injection cache is cleared.