[FEATURE] Introduce DocumentService for one-time loading of current document #1776
Conversation
…of document and save to temp
sebastian-meyer
left a comment
sebastian-meyer
left a comment
There was a problem hiding this comment.
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?
| use Kitodo\Dlf\Domain\Repository\DocumentRepository; | ||
| use TYPO3\CMS\Core\Log\LogManager; | ||
| use TYPO3\CMS\Core\Utility\GeneralUtility; | ||
| use Kitodo\Dlf\Common\AbstractDocument; | ||
| use Kitodo\Dlf\Domain\Model\Document; | ||
| use TYPO3\CMS\Core\Utility\MathUtility; |
There was a problem hiding this comment.
We typically sort imported namespaces alphabetically for better readability.
| use Kitodo\Dlf\Domain\Repository\DocumentRepository; | |
| use TYPO3\CMS\Core\Log\LogManager; | |
| use TYPO3\CMS\Core\Utility\GeneralUtility; | |
| use Kitodo\Dlf\Common\AbstractDocument; | |
| use Kitodo\Dlf\Domain\Model\Document; | |
| use TYPO3\CMS\Core\Utility\MathUtility; | |
| use Kitodo\Dlf\Common\AbstractDocument; | |
| use Kitodo\Dlf\Domain\Model\Document; | |
| use Kitodo\Dlf\Domain\Repository\DocumentRepository; | |
| use TYPO3\CMS\Core\Log\LogManager; | |
| use TYPO3\CMS\Core\Utility\GeneralUtility; | |
| use TYPO3\CMS\Core\Utility\MathUtility; |
| // Get document ID from request data if not passed as parameter. | ||
| if (!empty($recordId)) { | ||
| $documentId = $recordId; | ||
| } |
There was a problem hiding this comment.
This does not what the comment suggests. In fact, if the document ID is not passed as parameter, document loading will fail...
Please have a closer look at the original implementation!
| public function __construct() | ||
| { | ||
| $this->documentRepository = GeneralUtility::makeInstance(DocumentRepository::class); | ||
| $this->logger = GeneralUtility::makeInstance(LogManager::class)->getLogger(__CLASS__); |
There was a problem hiding this comment.
Please consider using dependency injection (recommended) or the LoggerAwareTrait.
| /** | ||
| * Get document from repository by uid. | ||
| * | ||
| * @access private | ||
| * | ||
| * @param int $documentId The document's UID | ||
| * | ||
| * @return AbstractDocument | ||
| */ | ||
| private function getDocumentByUid(int $documentId): AbstractDocument|null | ||
| { | ||
| $doc = null; | ||
| $this->document = $this->documentRepository->findOneByIdAndSettings($documentId); | ||
|
|
||
| if ($this->document) { | ||
| $doc = AbstractDocument::getInstance($this->document->getLocation(), $this->settings, false); | ||
| } else { | ||
| $this->logger->error('Invalid UID "' . $documentId . '" or PID "' . $this->settings['storagePid'] . '" for document loading'); | ||
| } | ||
|
|
||
| return $doc; | ||
| } | ||
|
|
||
| /** | ||
| * Get document by URL. | ||
| * | ||
| * @access private | ||
| * | ||
| * @param string $documentId The document's URL | ||
| * | ||
| * @return AbstractDocument | ||
| */ | ||
| private function getDocumentByUrl(string $documentId) | ||
| { | ||
| $doc = AbstractDocument::getInstance($documentId, $this->settings, false); | ||
|
|
||
| if ($doc !== null) { | ||
| if ($doc->recordId) { | ||
| // find document from repository by recordId | ||
| $docFromRepository = $this->documentRepository->findOneByRecordId($doc->recordId); | ||
| if ($docFromRepository !== null) { | ||
| $this->document = $docFromRepository; | ||
| } else { | ||
| // create new dummy Document object | ||
| $this->document = GeneralUtility::makeInstance(Document::class); | ||
| } | ||
| } | ||
|
|
||
| // Make sure configuration PID is set when applicable | ||
| if ($doc->cPid == 0) { | ||
| $doc->cPid = max($this->settings['storagePid'], 0); | ||
| } | ||
|
|
||
| $this->document->setLocation($documentId); | ||
| } else { | ||
| $this->logger->error('Invalid location given "' . $documentId . '" for document loading'); | ||
| } | ||
|
|
||
| return $doc; | ||
| } |
There was a problem hiding this comment.
Those are duplicates of the same methods in AbstractController. Please try to avoid duplicate code.
| * @return void | ||
| */ | ||
| protected function loadDocument(string $documentId = ''): void | ||
| protected function loadDocument(): void |
There was a problem hiding this comment.
There are still instances where loadDocument() is called with a parameter (e. g. in BasketController). Please make sure this doesn't break.
2 participants
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.