Skip to content

Introduced MediaGalleryMetadata services with XMP support #1514

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

Merged
merged 12 commits into from
Jul 4, 2020
Merged
59 changes: 59 additions & 0 deletions MediaGalleryMetadata/Model/File.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace Magento\MediaGalleryMetadata\Model;

/**
* File
*/
class File
{
/**
* @var string
*/
private $path;

/**
* @var string
*/
private $compressedImage;

/**
* @var array
*/
private $segments;

/**
* @param string $path
* @param string $compressedImage
* @param array $segments
*/
public function __construct(string $path, string $compressedImage, array $segments)
{
$this->path = $path;
$this->compressedImage = $compressedImage;
$this->segments = $segments;
}

public function getCompressedImage(): string
{
return $this->compressedImage;
}

/**
* @return Segment[]
*/
public function getSegments(): array
{
return $this->segments;
}

public function getPath(): string
{
return $this->path;
}
}
135 changes: 135 additions & 0 deletions MediaGalleryMetadata/Model/Reader/File.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace Magento\MediaGalleryMetadata\Model\Reader;

use Magento\Framework\Exception\LocalizedException;
use Magento\MediaGalleryMetadata\Model\File as FileDataObject;
use Magento\MediaGalleryMetadata\Model\FileFactory;
use Magento\MediaGalleryMetadata\Model\SegmentFactory;
use Magento\MediaGalleryMetadata\Model\SegmentNames;

/**
* File segments reader
*/
class File
{
private const MARKER_IMAGE_FILE_START = "\xD8";
private const MARKER_IMAGE_PREFIX = "\xFF";
private const MARKER_IMAGE_END = "\xD9";
private const MARKER_IMAGE_START = "\xDA";

/**
* @var SegmentFactory
*/
private $segmentFactory;

/**
* @var FileFactory
*/
private $fileFactory;

/**
* @var SegmentNames
*/
private $segmentNames;

public function __construct(FileFactory $fileFactory, SegmentFactory $segmentFactory, SegmentNames $segmentNames)
{
$this->fileFactory = $fileFactory;
$this->segmentFactory = $segmentFactory;
$this->segmentNames = $segmentNames;
}

/**
* @param string $path
* @return FileDataObject
* @throws LocalizedException
*/
public function execute(string $path): FileDataObject
{
$fileHandle = @fopen($path, 'rb');

if (!$fileHandle) {
throw new LocalizedException(__('Cannot open file'));
}

$data = $this->read($fileHandle, 2);

if ($data != self::MARKER_IMAGE_PREFIX . self::MARKER_IMAGE_FILE_START) {
fclose($fileHandle);
Copy link
Contributor

Choose a reason for hiding this comment

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

For fclose function, we can use Magento\Framework\Filesystem\DriverInterface::fileClose()

throw new LocalizedException(__('Not an image'));
}

$data = $this->read($fileHandle, 2);

if ($data[0] != self::MARKER_IMAGE_PREFIX) {
fclose($fileHandle);
throw new LocalizedException(__('File is corrupted'));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add these validations to a private method?


$segments = [];

while (($data[1] != self::MARKER_IMAGE_END) && (!feof($fileHandle))) {
if ((ord($data[1]) < 0xD0) || (ord($data[1]) > 0xD7)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it worth to wrap these conditions on methods to make the behavior explicit, especially because we are dealing with functions and operations that are not used that often (by us at least).

$sizestr = $this->read($fileHandle, 2);

$decodedsize = unpack("nsize", $sizestr);

$segmentDataStartPosition = ftell($fileHandle);

$segmentData = $this->read($fileHandle, $decodedsize['size'] - 2);

$segments[] = $this->segmentFactory->create([
'name' => $this->segmentNames->getSegmentName(ord($data[1])),
'dataStart' => $segmentDataStartPosition,
'data' => $segmentData,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we wrap this to a private function? Something like createSegment($segmentName, $fileHandle)

]);
}

if ($data[1] == self::MARKER_IMAGE_START) {
$compressedImage = '';
do {
$compressedImage .= $this->read($fileHandle, 1048576);
} while (!feof($fileHandle));

$endOfImageMarkerPosition = strpos($compressedImage, self::MARKER_IMAGE_PREFIX . self::MARKER_IMAGE_END);
$compressedImage = substr($compressedImage, 0, $endOfImageMarkerPosition);

fclose($fileHandle);

return $this->fileFactory->create([
'path' => $path,
'compressedImage' => $compressedImage,
'segments' => $segments,
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar thing here, can we wrap this on private function? Maybe also add constant for values like 1048576 and 2, that are used on read method, mostly to explain what they are.

}

$data = $this->read($fileHandle, 2);

if ($data[0] != self::MARKER_IMAGE_PREFIX) {
fclose($fileHandle);
throw new LocalizedException(__('File is corrupted'));
}
}

fclose($fileHandle);

throw new LocalizedException(__('File is corrupted'));
}

private function read($fileHandle, int $length): string
{
$data = '';

while (!feof($fileHandle) && strlen($data) < $length) {
$data .= fread($fileHandle, $length - strlen($data));
}

return $data;
}
}
65 changes: 65 additions & 0 deletions MediaGalleryMetadata/Model/Segment.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace Magento\MediaGalleryMetadata\Model;

/**
* Segment
*/
class Segment
{
/**
* @var array
*/
private $name;

/**
* @var int
*/
private $dataStart;

/**
* @var string
*/
private $data;

/**
* @param string $name
* @param int $dataStart
* @param string $data
*/
public function __construct(string $name, int $dataStart, string $data)
{
$this->name = $name;
$this->dataStart = $dataStart;
$this->data = $data;
}

/**
* @return string
*/
public function getName(): string
{
return $this->name;
}

/**
* @return int
*/
public function getDataStart(): int
{
return $this->dataStart;
}

/**
* @return string
*/
public function getData(): string
{
return $this->data;
}
}
104 changes: 104 additions & 0 deletions MediaGalleryMetadata/Model/SegmentNames.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace Magento\MediaGalleryMetadata\Model;

/**
* Segment types to names mapper
*/
class SegmentNames
{
private const SEGMENT_TYPE_TO_NAME = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to use hexadecimal values here? I guess it makes it harder to understand and when debugging the application.

0xC0 => "SOF0",
0xC1 => "SOF1",
0xC2 => "SOF2",
0xC3 => "SOF4",
0xC5 => "SOF5",
0xC6 => "SOF6",
0xC7 => "SOF7",
0xC8 => "JPG",
0xC9 => "SOF9",
0xCA => "SOF10",
0xCB => "SOF11",
0xCD => "SOF13",
0xCE => "SOF14",
0xCF => "SOF15",
0xC4 => "DHT",
0xCC => "DAC",
0xD0 => "RST0",
0xD1 => "RST1",
0xD2 => "RST2",
0xD3 => "RST3",
0xD4 => "RST4",
0xD5 => "RST5",
0xD6 => "RST6",
0xD7 => "RST7",
0xD8 => "SOI",
0xD9 => "EOI",
0xDA => "SOS",
0xDB => "DQT",
0xDC => "DNL",
0xDD => "DRI",
0xDE => "DHP",
0xDF => "EXP",
0xE0 => "APP0",
0xE1 => "APP1",
0xE2 => "APP2",
0xE3 => "APP3",
0xE4 => "APP4",
0xE5 => "APP5",
0xE6 => "APP6",
0xE7 => "APP7",
0xE8 => "APP8",
0xE9 => "APP9",
0xEA => "APP10",
0xEB => "APP11",
0xEC => "APP12",
0xED => "APP13",
0xEE => "APP14",
0xEF => "APP15",
0xF0 => "JPG0",
0xF1 => "JPG1",
0xF2 => "JPG2",
0xF3 => "JPG3",
0xF4 => "JPG4",
0xF5 => "JPG5",
0xF6 => "JPG6",
0xF7 => "JPG7",
0xF8 => "JPG8",
0xF9 => "JPG9",
0xFA => "JPG10",
0xFB => "JPG11",
0xFC => "JPG12",
0xFD => "JPG13",
0xFE => "COM",
0x01 => "TEM",
0x02 => "RES",
];

/**
* Get segment name by type
*
* @param int $type
* @return string
*/
public function getSegmentName(int $type): string
{
return self::SEGMENT_TYPE_TO_NAME[$type];
}

/**
* Get segment type by name
*
* @param string $name
* @return int
*/
public function getSegmentType(string $name): int
{
return array_search($name, self::SEGMENT_TYPE_TO_NAME);
}
}
Loading