fix: Code Review

eZ/Publish/API/Repository/Tests/NonRedundantFieldSetTest.php

@@ -152,7 +152,7 @@ public function testCreateContentEmptyValuesTranslationNotStored()
      *
      * @param \eZ\Publish\API\Repository\Values\Content\Content $content
      */
-    public function testCreateContentEmptyValuesTranslationVirtualFields(Content $content)
+    public function testCreateContentEmptyValuesTranslationVirtualFields(Content $content): void
     {
         $emptyValue = $this->getRepository()->getFieldTypeService()->getFieldType('ezstring')->getEmptyValue();
 
@@ -266,7 +266,7 @@ public function testCreateContentTwoLanguagesSecondTranslationNotStored()
      *
      * @param \eZ\Publish\API\Repository\Values\Content\Content $content
      */
-    public function testCreateContentTwoLanguagesSecondTranslationVirtualFields(Content $content)
+    public function testCreateContentTwoLanguagesSecondTranslationVirtualFields(Content $content): void
     {
         $emptyValue = $this->getRepository()->getFieldTypeService()->getFieldType('ezstring')->getEmptyValue();
 

eZ/Publish/Core/Persistence/Legacy/Content/FieldHandler.php

@@ -148,7 +148,7 @@ protected function getEmptyField(FieldDefinition $fieldDefinition, $languageCode
      *
      * @param \eZ\Publish\SPI\Persistence\Content $content
      */
-    public function createExistingFieldsInNewVersion(Content $content)
+    public function createExistingFieldsInNewVersion(Content $content): void
     {
         foreach ($content->fields as $field) {
             if ($field->id === null) {

eZ/Publish/Core/Persistence/Legacy/Content/Mapper.php

@@ -46,12 +46,6 @@ class Mapper
      */
     private $contentTypeHandler;
 
-    /**
-     * Creates a new mapper.
-     *
-     * @param \eZ\Publish\Core\Persistence\Legacy\Content\FieldValue\ConverterRegistry $converterRegistry
-     * @param \eZ\Publish\SPI\Persistence\Content\Language\Handler $languageHandler
-     */
     public function __construct(
         Registry $converterRegistry,
         LanguageHandler $languageHandler,
@@ -187,11 +181,17 @@ public function convertToStorageValue(Field $field)
      *
      * @param array $rows
      * @param array $nameRows
+     * @param string $prefix
      *
      * @return \eZ\Publish\SPI\Persistence\Content[]
+     *
+     * @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException
      */
-    public function extractContentFromRows(array $rows, array $nameRows, $prefix = 'ezcontentobject_')
-    {
+    public function extractContentFromRows(
+        array $rows,
+        array $nameRows,
+        string $prefix = 'ezcontentobject_'
+    ): array {
         $versionedNameData = [];
         $languageCodes = [];
 
@@ -205,58 +205,74 @@ public function extractContentFromRows(array $rows, array $nameRows, $prefix = '
         $contentInfos = [];
         $versionInfos = [];
         $fields = [];
-        $fieldDefinitions = [];
+
+        $fieldDefinitions = $this->loadCachedVersionFieldDefinitionsPerLanguage(
+            $rows,
+            $languageCodes,
+            $prefix
+        );
 
         foreach ($rows as $row) {
             $contentId = (int)$row["{$prefix}id"];
             $versionId = (int)$row["{$prefix}version_id"];
-            $contentTypeId = (int)$row["{$prefix}contentclass_id"];
+
             if (!isset($contentInfos[$contentId])) {
                 $contentInfos[$contentId] = $this->extractContentInfoFromRow($row, $prefix);
             }
+
             if (!isset($versionInfos[$contentId])) {
                 $versionInfos[$contentId] = [];
             }
 
-            if (!isset($fieldDefinitions[$contentId][$versionId])) {
-                $contentType = $this->contentTypeHandler->load($contentTypeId);
-                foreach ($contentType->fieldDefinitions as $fieldDefinition) {
-                    foreach ($languageCodes as $languageCode) {
-                        $fieldDefinitions[$contentId][$versionId][$languageCode][$fieldDefinition->id] = $fieldDefinition;
-                    }
-                }
-            }
-
             if (!isset($versionInfos[$contentId][$versionId])) {
                 $versionInfos[$contentId][$versionId] = $this->extractVersionInfoFromRow($row);
             }
 
             $fieldId = (int)$row["{$prefix}attribute_id"];
             $fieldDefinitionId = (int)$row["{$prefix}attribute_contentclassattribute_id"];
             $languageCode = $row["{$prefix}attribute_language_code"];
-            if (!isset($fields[$contentId][$versionId][$fieldId]) && isset($fieldDefinitions[$contentId][$versionId][$languageCode][$fieldDefinitionId])) {
+
+            if (!isset($fields[$contentId][$versionId][$fieldId])
+                && isset($fieldDefinitions[$contentId][$versionId][$languageCode][$fieldDefinitionId])
+            ) {
                 $fields[$contentId][$versionId][$fieldId] = $this->extractFieldFromRow($row);
                 unset($fieldDefinitions[$contentId][$versionId][$languageCode][$fieldDefinitionId]);
             }
         }
 
+        return $this->buildContentObjects(
+            $contentInfos,
+            $versionInfos,
+            $fields,
+            $fieldDefinitions,
+            $versionedNameData
+        );
+    }
+
+    /**
+     * @return \eZ\Publish\SPI\Persistence\Content[]
+     */
+    private function buildContentObjects(
+        array $contentInfos,
+        array $versionInfos,
+        array $fields,
+        array $fieldDefinitions,
+        array $versionedNameData
+    ): array {
         $results = [];
+
         foreach ($contentInfos as $contentId => $contentInfo) {
             foreach ($versionInfos[$contentId] as $versionId => $versionInfo) {
                 // Fallback to just main language name if versioned name data is missing
-                if (isset($versionedNameData[$contentId][$versionInfo->versionNo])) {
-                    $names = $versionedNameData[$contentId][$versionInfo->versionNo];
-                } else {
-                    $names = [$contentInfo->mainLanguageCode => $contentInfo->name];
-                }
+                $names = $versionedNameData[$contentId][$versionInfo->versionNo]
+                    ?? [$contentInfo->mainLanguageCode => $contentInfo->name];
 
                 $content = new Content();
                 $content->versionInfo = $versionInfo;
                 $content->versionInfo->names = $names;
                 $content->versionInfo->contentInfo = $contentInfo;
                 $content->fields = array_values($fields[$contentId][$versionId]);
 
-                /** @var string $languageCode */
                 foreach ($fieldDefinitions[$contentId][$versionId] as $languageCode => $versionFieldDefinitions) {
                     foreach ($versionFieldDefinitions as $fieldDefinition) {
                         $content->fields[] = $this->createEmptyField(
@@ -273,6 +289,37 @@ public function extractContentFromRows(array $rows, array $nameRows, $prefix = '
         return $results;
     }
 
+    /**
+     * @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException
+     */
+    private function loadCachedVersionFieldDefinitionsPerLanguage(
+        array $rows,
+        array $languageCodes,
+        string $prefix
+    ): array {
+        $fieldDefinitions = [];
+        $contentTypes = [];
+
+        foreach ($rows as $row) {
+            $contentId = (int)$row["{$prefix}id"];
+            $versionId = (int)$row["{$prefix}version_id"];
+            $contentTypeId = (int)$row["{$prefix}contentclass_id"];
+
+            if (!isset($fieldDefinitions[$contentId][$versionId])) {
+                $contentType = $contentTypes[$contentTypeId] = $contentTypes[$contentTypeId]
+                    ?? $this->contentTypeHandler->load($contentTypeId);
+                foreach ($contentType->fieldDefinitions as $fieldDefinition) {
+                    foreach ($languageCodes as $languageCode) {
+                        $id = $fieldDefinition->id;
+                        $fieldDefinitions[$contentId][$versionId][$languageCode][$id] = $fieldDefinition;
+                    }
+                }
+            }
+        }
+
+        return $fieldDefinitions;
+    }
+
     /**
      * Extracts a ContentInfo object from $row.
      *

eZ/Publish/Core/Persistence/Legacy/Content/StorageHandler.php

@@ -79,7 +79,7 @@ public function copyFieldData(VersionInfo $versionInfo, Field $field, Field $ori
     public function getFieldData(VersionInfo $versionInfo, Field $field)
     {
         $storage = $this->storageRegistry->getStorage($field->type);
-        if ($storage->hasFieldData() && $field->id !== null) {
+        if ($field->id !== null && $storage->hasFieldData()) {
             $storage->getFieldData($versionInfo, $field, $this->context);
         }
     }

eZ/Publish/Core/Persistence/Legacy/Tests/Content/MapperTest.php

@@ -246,10 +246,13 @@ public function testExtractContentFromRowsWithRemovedFieldDefinitions(): void
         $nameRowsFixture = $this->getNamesExtractFixture();
 
         $contentType = $this->getContentTypeFromRows($rowsFixture);
-        $contentType->fieldDefinitions = array_filter($contentType->fieldDefinitions, static function (Content\Type\FieldDefinition $fieldDefinition) {
-            // ref. fixtures, ezauthor
-            return $fieldDefinition->id !== 185;
-        });
+        $contentType->fieldDefinitions = array_filter(
+            $contentType->fieldDefinitions,
+            static function (Content\Type\FieldDefinition $fieldDefinition) {
+                // ref. fixtures, ezauthor
+                return $fieldDefinition->id !== 185;
+            }
+        );
 
         $contentTypeHandlerMock = $this->getContentTypeHandler();
         $contentTypeHandlerMock->method('load')->willReturn($contentType);
@@ -324,24 +327,24 @@ public function testExtractContentFromRowsMultipleVersions()
     }
 
     /**
-     * @param string[] $fields
+     * @param string[] $fieldTypeIdentifiers
      */
     private function getFieldRegistry(
-        array $fields = [],
-        int $expectedConverterCalls = null
+        array $fieldTypeIdentifiers = [],
+        ?int $expectedConverterCalls = null
     ): Registry {
-        $convMock = $this->createMock(Converter::class);
-        $convMock->expects(
+        $converterMock = $this->createMock(Converter::class);
+        $converterMock->expects(
             $expectedConverterCalls === null
-            ? $this->any()
-            : $this->exactly($expectedConverterCalls)
+            ? self::any()
+            : self::exactly($expectedConverterCalls)
         )
             ->method('toFieldValue')
-            ->will($this->returnValue(new FieldValue()));
+            ->willReturn(new FieldValue());
 
         $converters = [];
-        foreach ($fields as $field) {
-            $converters[$field] = $convMock;
+        foreach ($fieldTypeIdentifiers as $fieldTypeIdentifier) {
+            $converters[$fieldTypeIdentifier] = $converterMock;
         }
 
         return new Registry($converters);

eZ/Publish/Core/Persistence/Legacy/Tests/Content/StorageHandlerTest.php

@@ -128,6 +128,34 @@ public function testGetFieldDataNotAvailable()
             ->with($this->equalTo('foobar'))
             ->will($this->returnValue($storageMock));
 
+        $field = new Field();
+        $field->id = 123;
+        $field->type = 'foobar';
+        $field->value = new FieldValue();
+
+        $handler = $this->getStorageHandler();
+        $handler->getFieldData($this->getVersionInfoMock(), $field);
+    }
+
+    /**
+     * @covers \eZ\Publish\Core\Persistence\Legacy\Content\StorageHandler::getFieldData
+     */
+    public function testGetFieldDataNotAvailableForVirtualField()
+    {
+        $storageMock = $this->getStorageMock();
+        $storageRegistryMock = $this->getStorageRegistryMock();
+
+        $storageMock->expects(self::never())
+            ->method('hasFieldData');
+
+        $storageMock->expects(self::never())
+            ->method('getFieldData');
+
+        $storageRegistryMock->expects(self::once())
+            ->method('getStorage')
+            ->with($this->equalTo('foobar'))
+            ->willReturn($storageMock);
+
         $field = new Field();
         $field->type = 'foobar';
         $field->value = new FieldValue();