From ceb7c7bd65bf5e62c4f0475e149fd4b539e85e9d Mon Sep 17 00:00:00 2001 From: jona Date: Mon, 13 Jan 2025 04:53:05 +0100 Subject: [PATCH] Split attachment path into internal and external path, so the external source URL can be retained after a file is downloaded --- migrations/Version20250112051523.php | 39 ++++ src/Controller/AttachmentFileController.php | 16 +- src/DataFixtures/PartFixtures.php | 2 +- src/DataTables/AttachmentDataTable.php | 70 +++--- src/Entity/Attachments/Attachment.php | 216 +++++++++++------- .../AttachmentDeleteListener.php | 4 +- src/Repository/AttachmentRepository.php | 17 +- src/Serializer/AttachmentNormalizer.php | 2 +- .../Attachments/AttachmentManager.php | 54 ++--- .../Attachments/AttachmentReverseSearch.php | 2 +- .../Attachments/AttachmentSubmitHandler.php | 28 +-- .../Attachments/AttachmentURLGenerator.php | 18 +- .../Mergers/EntityMergerHelperTrait.php | 3 +- src/Services/EntityURLGenerator.php | 31 ++- .../PartKeeprImporter/PKImportHelperTrait.php | 2 +- .../LabelSystem/SandboxedTwigFactory.php | 4 +- .../parts/edit/edit_form_styles.html.twig | 32 ++- .../parts/info/_attachments_info.html.twig | 21 +- .../API/Endpoints/AttachmentsEndpointTest.php | 2 +- tests/Entity/Attachments/AttachmentTest.php | 133 +++++------ translations/messages.en.xlf | 6 +- 21 files changed, 399 insertions(+), 303 deletions(-) create mode 100644 migrations/Version20250112051523.php diff --git a/migrations/Version20250112051523.php b/migrations/Version20250112051523.php new file mode 100644 index 00000000..e4aa347c --- /dev/null +++ b/migrations/Version20250112051523.php @@ -0,0 +1,39 @@ +addSql('ALTER TABLE attachments ADD internal_path VARCHAR(255) DEFAULT \'\' NOT NULL'); + $this->addSql('ALTER TABLE attachments RENAME COLUMN path TO external_path'); + + $this->addSql('UPDATE attachments SET internal_path=external_path WHERE external_path LIKE \'#%MEDIA#%%\' ESCAPE \'#\''); + $this->addSql('UPDATE attachments SET internal_path=external_path WHERE external_path LIKE \'#%BASE#%%\' ESCAPE \'#\''); + $this->addSql('UPDATE attachments SET internal_path=external_path WHERE external_path LIKE \'#%SECURE#%%\' ESCAPE \'#\''); + $this->addSql('UPDATE attachments SET internal_path=external_path WHERE external_path LIKE \'#%FOOTPRINTS#%%\' ESCAPE \'#\''); + $this->addSql('UPDATE attachments SET internal_path=external_path WHERE external_path LIKE \'#%FOOTPRINTS3D#%%\' ESCAPE \'#\''); + $this->addSql('UPDATE attachments SET external_path=\'\' WHERE internal_path <> \'\''); + } + + public function down(Schema $schema): void + { + $this->addSql('UPDATE attachments SET external_path=internal_path WHERE internal_path <> \'\''); + + $this->addSql('ALTER TABLE attachments DROP internal_path'); + $this->addSql('ALTER TABLE attachments RENAME COLUMN external_path TO path'); + } +} diff --git a/src/Controller/AttachmentFileController.php b/src/Controller/AttachmentFileController.php index 936d27c5..d8bd8d87 100644 --- a/src/Controller/AttachmentFileController.php +++ b/src/Controller/AttachmentFileController.php @@ -51,15 +51,15 @@ class AttachmentFileController extends AbstractController $this->denyAccessUnlessGranted('show_private', $attachment); } - if ($attachment->isExternal()) { - throw $this->createNotFoundException('The file for this attachment is external and can not stored locally!'); + if (!$attachment->hasInternal()) { + throw $this->createNotFoundException('The file for this attachment is external and not stored locally!'); } - if (!$helper->isFileExisting($attachment)) { + if (!$helper->isInternalFileExisting($attachment)) { throw $this->createNotFoundException('The file associated with the attachment is not existing!'); } - $file_path = $helper->toAbsoluteFilePath($attachment); + $file_path = $helper->toAbsoluteInternalFilePath($attachment); $response = new BinaryFileResponse($file_path); //Set header content disposition, so that the file will be downloaded @@ -80,15 +80,15 @@ class AttachmentFileController extends AbstractController $this->denyAccessUnlessGranted('show_private', $attachment); } - if ($attachment->isExternal()) { - throw $this->createNotFoundException('The file for this attachment is external and can not stored locally!'); + if (!$attachment->hasInternal()) { + throw $this->createNotFoundException('The file for this attachment is external and not stored locally!'); } - if (!$helper->isFileExisting($attachment)) { + if (!$helper->isInternalFileExisting($attachment)) { throw $this->createNotFoundException('The file associated with the attachment is not existing!'); } - $file_path = $helper->toAbsoluteFilePath($attachment); + $file_path = $helper->toAbsoluteInternalFilePath($attachment); $response = new BinaryFileResponse($file_path); //Set header content disposition, so that the file will be downloaded diff --git a/src/DataFixtures/PartFixtures.php b/src/DataFixtures/PartFixtures.php index 0c8ea36d..a60d037d 100644 --- a/src/DataFixtures/PartFixtures.php +++ b/src/DataFixtures/PartFixtures.php @@ -131,7 +131,7 @@ class PartFixtures extends Fixture implements DependentFixtureInterface $attachment = new PartAttachment(); $attachment->setName('Test2'); - $attachment->setPath('invalid'); + $attachment->setInternalPath('invalid'); $attachment->setShowInTable(true); $attachment->setAttachmentType($manager->find(AttachmentType::class, 1)); $part->addAttachment($attachment); diff --git a/src/DataTables/AttachmentDataTable.php b/src/DataTables/AttachmentDataTable.php index 0d6c5b53..04299ca9 100644 --- a/src/DataTables/AttachmentDataTable.php +++ b/src/DataTables/AttachmentDataTable.php @@ -50,8 +50,8 @@ final class AttachmentDataTable implements DataTableTypeInterface { $dataTable->add('dont_matter', RowClassColumn::class, [ 'render' => function ($value, Attachment $context): string { - //Mark attachments with missing files yellow - if(!$this->attachmentHelper->isFileExisting($context)){ + //Mark attachments yellow which have an internal file linked that doesn't exist + if($context->hasInternal() && !$this->attachmentHelper->isInternalFileExisting($context)){ return 'table-warning'; } @@ -64,8 +64,8 @@ final class AttachmentDataTable implements DataTableTypeInterface 'className' => 'no-colvis', 'render' => function ($value, Attachment $context): string { if ($context->isPicture() - && !$context->isExternal() - && $this->attachmentHelper->isFileExisting($context)) { + && $this->attachmentHelper->isInternalFileExisting($context)) { + $title = htmlspecialchars($context->getName()); if ($context->getFilename()) { $title .= ' ('.htmlspecialchars($context->getFilename()).')'; @@ -93,26 +93,6 @@ final class AttachmentDataTable implements DataTableTypeInterface $dataTable->add('name', TextColumn::class, [ 'label' => 'attachment.edit.name', 'orderField' => 'NATSORT(attachment.name)', - 'render' => function ($value, Attachment $context) { - //Link to external source - if ($context->isExternal()) { - return sprintf( - '%s', - htmlspecialchars((string) $context->getURL()), - htmlspecialchars($value) - ); - } - - if ($this->attachmentHelper->isFileExisting($context)) { - return sprintf( - '%s', - $this->entityURLGenerator->viewURL($context), - htmlspecialchars($value) - ); - } - - return $value; - }, ]); $dataTable->add('attachment_type', TextColumn::class, [ @@ -136,15 +116,42 @@ final class AttachmentDataTable implements DataTableTypeInterface ), ]); - $dataTable->add('filename', TextColumn::class, [ - 'label' => $this->translator->trans('attachment.table.filename'), + $dataTable->add('internal_link', TextColumn::class, [ + 'label' => 'Internal copy', #TODO: translation 'propertyPath' => 'filename', + 'render' => function ($value, Attachment $context) { + if ($this->attachmentHelper->isInternalFileExisting($context)) { + return sprintf( + '%s', + $this->entityURLGenerator->viewURL($context), + htmlspecialchars($value) + ); + } + + return $value; + } + ]); + + $dataTable->add('external_link', TextColumn::class, [ + 'label' => 'External copy', #TODO: translation + 'propertyPath' => 'host', + 'render' => function ($value, Attachment $context) { + if ($context->hasExternal()) { + return sprintf( + '%s', + htmlspecialchars((string) $context->getExternalPath()), + htmlspecialchars($value) + ); + } + + return $value; + } ]); $dataTable->add('filesize', TextColumn::class, [ 'label' => $this->translator->trans('attachment.table.filesize'), 'render' => function ($value, Attachment $context) { - if ($context->isExternal()) { + if (!$context->hasInternal()) { return sprintf( ' %s @@ -153,8 +160,13 @@ final class AttachmentDataTable implements DataTableTypeInterface ); } - if ($this->attachmentHelper->isFileExisting($context)) { - return $this->attachmentHelper->getHumanFileSize($context); + if ($this->attachmentHelper->isInternalFileExisting($context)) { + return sprintf( + ' + %s + ', + $this->attachmentHelper->getHumanFileSize($context) + ); } return sprintf( diff --git a/src/Entity/Attachments/Attachment.php b/src/Entity/Attachments/Attachment.php index 30d9e257..3d92bb98 100644 --- a/src/Entity/Attachments/Attachment.php +++ b/src/Entity/Attachments/Attachment.php @@ -78,11 +78,16 @@ use LogicException; denormalizationContext: ['groups' => ['attachment:write', 'attachment:write:standalone', 'api:basic:write'], 'openapi_definition_name' => 'Write'], processor: HandleAttachmentsUploadsProcessor::class, )] -#[DocumentedAPIProperty(schemaName: 'Attachment-Read', property: 'media_url', type: 'string', nullable: true, - description: 'The URL to the file, where the attachment file can be downloaded. This can be an internal or external URL.', - example: '/media/part/2/bc547-6508afa5a79c8.pdf')] -#[DocumentedAPIProperty(schemaName: 'Attachment-Read', property: 'thumbnail_url', type: 'string', nullable: true, - description: 'The URL to a thumbnail version of this file. This only exists for internal picture attachments.')] +//This property is added by the denormalizer in order to resolve the placeholder +#[DocumentedAPIProperty( + schemaName: 'Attachment-Read', property: 'internal_path', type: 'string', nullable: false, + description: 'The URL to the internally saved copy of the file, if one exists', + example: '/media/part/2/bc547-6508afa5a79c8.pdf' +)] +#[DocumentedAPIProperty( + schemaName: 'Attachment-Read', property: 'thumbnail_url', type: 'string', nullable: true, + description: 'The URL to a thumbnail version of this file. This only exists for internal picture attachments.' +)] #[ApiFilter(LikeFilter::class, properties: ["name"])] #[ApiFilter(EntityFilter::class, properties: ["attachment_type"])] #[ApiFilter(DateFilter::class, strategy: DateFilterInterface::EXCLUDE_NULL)] @@ -119,10 +124,6 @@ abstract class Attachment extends AbstractNamedDBElement */ final public const MODEL_EXTS = ['x3d']; - /** - * When the path begins with one of the placeholders. - */ - final public const INTERNAL_PLACEHOLDER = ['%BASE%', '%MEDIA%', '%SECURE%']; /** * @var array placeholders for attachments which using built in files @@ -152,10 +153,21 @@ abstract class Attachment extends AbstractNamedDBElement protected ?string $original_filename = null; /** - * @var string The path to the file relative to a placeholder path like %MEDIA% + * @var string If a copy of the file is stored internally, the path to the file relative to a placeholder + * path like %MEDIA% */ - #[ORM\Column(name: 'path', type: Types::STRING)] - protected string $path = ''; + #[ORM\Column(type: Types::STRING)] + protected string $internal_path = ''; + + + /** + * @var string The path to the external source if the file is stored externally or was downloaded from an + * external source + */ + #[ORM\Column(type: Types::STRING)] + #[Groups(['attachment:read'])] + #[ApiProperty(example: 'http://example.com/image.jpg')] + protected string $external_path = ''; /** * @var string the name of this element @@ -237,7 +249,7 @@ abstract class Attachment extends AbstractNamedDBElement /** * Check if this attachment is a picture (analyse the file's extension). - * If the link is external, it is assumed that this is true. + * If the link is only external and doesn't contain an extension, it is assumed that this is true. * * @return bool * true if the file extension is a picture extension * * otherwise false @@ -245,54 +257,63 @@ abstract class Attachment extends AbstractNamedDBElement #[Groups(['attachment:read'])] public function isPicture(): bool { - if ($this->isExternal()) { + if($this->hasInternal()){ + + $extension = pathinfo($this->getInternalPath(), PATHINFO_EXTENSION); + + return in_array(strtolower($extension), static::PICTURE_EXTS, true); + + } + if ($this->hasExternal()) { //Check if we can extract a file extension from the URL - $extension = pathinfo(parse_url($this->path, PHP_URL_PATH) ?? '', PATHINFO_EXTENSION); + $extension = pathinfo(parse_url($this->getExternalPath(), PHP_URL_PATH) ?? '', PATHINFO_EXTENSION); //If no extension is found or it is known picture extension, we assume that this is a picture extension return $extension === '' || in_array(strtolower($extension), static::PICTURE_EXTS, true); } - - $extension = pathinfo($this->getPath(), PATHINFO_EXTENSION); - - return in_array(strtolower($extension), static::PICTURE_EXTS, true); + //File doesn't have an internal, nor an external copy. This shouldn't happen, but it certainly isn't a picture... + return false; } /** * Check if this attachment is a 3D model and therefore can be directly shown to user. - * If the attachment is external, false is returned (3D Models must be internal). + * If no internal copy exists, false is returned (3D Models must be internal). */ #[Groups(['attachment:read'])] #[SerializedName('3d_model')] public function is3DModel(): bool { //We just assume that 3D Models are internally saved, otherwise we get problems loading them. - if ($this->isExternal()) { + if (!$this->hasInternal()) { return false; } - $extension = pathinfo($this->getPath(), PATHINFO_EXTENSION); + $extension = pathinfo($this->getInternalPath(), PATHINFO_EXTENSION); return in_array(strtolower($extension), static::MODEL_EXTS, true); } /** - * Checks if the attachment file is externally saved (the database saves an URL). + * Checks if this attachment has a path to an external file * - * @return bool true, if the file is saved externally + * @return bool true, if there is a path to an external file */ #[Groups(['attachment:read'])] - public function isExternal(): bool + public function hasExternal(): bool { - //When path is empty, this attachment can not be external - if ($this->path === '') { - return false; - } + return $this->external_path !== ''; + } - //After the %PLACEHOLDER% comes a slash, so we can check if we have a placeholder via explode - $tmp = explode('/', $this->path); - - return !in_array($tmp[0], array_merge(static::INTERNAL_PLACEHOLDER, static::BUILTIN_PLACEHOLDER), true); + /** + * Checks if this attachment has a path to an internal file. + * Does not check if the file exists. + * + * @return bool true, if there is a path to an internal file + */ + #[Groups(['attachment:read'])] + public function hasInternal(): bool + { + return $this->internal_path !== ''; } /** @@ -306,7 +327,7 @@ abstract class Attachment extends AbstractNamedDBElement public function isSecure(): bool { //After the %PLACEHOLDER% comes a slash, so we can check if we have a placeholder via explode - $tmp = explode('/', $this->path); + $tmp = explode('/', $this->internal_path); return '%SECURE%' === $tmp[0]; } @@ -320,7 +341,7 @@ abstract class Attachment extends AbstractNamedDBElement #[Groups(['attachment:read'])] public function isBuiltIn(): bool { - return static::checkIfBuiltin($this->path); + return static::checkIfBuiltin($this->internal_path); } /******************************************************************************** @@ -332,13 +353,13 @@ abstract class Attachment extends AbstractNamedDBElement /** * Returns the extension of the file referenced via the attachment. * For a path like %BASE/path/foo.bar, bar will be returned. - * If this attachment is external null is returned. + * If this attachment is only external null is returned. * * @return string|null the file extension in lower case */ public function getExtension(): ?string { - if ($this->isExternal()) { + if (!$this->hasInternal()) { return null; } @@ -346,7 +367,7 @@ abstract class Attachment extends AbstractNamedDBElement return strtolower(pathinfo($this->original_filename, PATHINFO_EXTENSION)); } - return strtolower(pathinfo($this->getPath(), PATHINFO_EXTENSION)); + return strtolower(pathinfo($this->getInternalPath(), PATHINFO_EXTENSION)); } /** @@ -361,52 +382,54 @@ abstract class Attachment extends AbstractNamedDBElement } /** - * The URL to the external file, or the path to the built-in file. + * The URL to the external file, or the path to the built-in file, but not paths to uploaded files. * Returns null, if the file is not external (and not builtin). + * The output of this function is such, that no changes occur when it is fed back into setURL(). + * Required for the Attachment form field. */ - #[Groups(['attachment:read'])] - #[SerializedName('url')] public function getURL(): ?string { - if (!$this->isExternal() && !$this->isBuiltIn()) { - return null; + if($this->hasExternal()){ + return $this->getExternalPath(); } - - return $this->path; + if($this->isBuiltIn()){ + return $this->getInternalPath(); + } + return null; } /** * Returns the hostname where the external file is stored. - * Returns null, if the file is not external. + * Returns null, if there is no external path. */ public function getHost(): ?string { - if (!$this->isExternal()) { + if (!$this->hasExternal()) { return null; } - return parse_url((string) $this->getURL(), PHP_URL_HOST); + return parse_url($this->getExternalPath(), PHP_URL_HOST); } - /** - * Get the filepath, relative to %BASE%. - * - * @return string A string like %BASE/path/foo.bar - */ - public function getPath(): string + public function getInternalPath(): string { - return $this->path; + return $this->internal_path; + } + + public function getExternalPath(): string + { + return $this->external_path; } /** * Returns the filename of the attachment. * For a path like %BASE/path/foo.bar, foo.bar will be returned. * - * If the path is a URL (can be checked via isExternal()), null will be returned. + * If there is no internal copy of the file, null will be returned. */ public function getFilename(): ?string { - if ($this->isExternal()) { + if (!$this->hasInternal()) { return null; } @@ -415,7 +438,7 @@ abstract class Attachment extends AbstractNamedDBElement return $this->original_filename; } - return pathinfo($this->getPath(), PATHINFO_BASENAME); + return pathinfo($this->getInternalPath(), PATHINFO_BASENAME); } /** @@ -488,15 +511,12 @@ abstract class Attachment extends AbstractNamedDBElement } /** - * Sets the filepath (with relative placeholder) for this attachment. - * - * @param string $path the new filepath of the attachment - * - * @return Attachment + * Sets the path to a file hosted internally. If you set this path to a file that was not downloaded from the + * external source in external_path, make sure to reset external_path. */ - public function setPath(string $path): self + public function setInternalPath(?string $internal_path): self { - $this->path = $path; + $this->internal_path = $internal_path ?? ''; return $this; } @@ -512,34 +532,62 @@ abstract class Attachment extends AbstractNamedDBElement } /** - * Sets the url associated with this attachment. - * If the url is empty nothing is changed, to not override the file path. - * - * @return Attachment + * Sets up the paths using a user provided string which might contain an external path or a builtin path. Allows + * resetting the external path if an internal path exists. Resets any other paths if a (nonempty) new path is set. */ #[Groups(['attachment:write'])] #[SerializedName('url')] + #[ApiProperty(description: 'Set the path of the attachment here. + Provide either an external URL, a path to a builtin file (like %FOOTPRINTS%/Active/ICs/IC_DFS.png) or an empty + string if the attachment has an internal file associated and you\'d like to reset the external source. + If you set a new (nonempty) file path any associated internal file will be removed!')] public function setURL(?string $url): self { - //Do nothing if the URL is empty - if ($url === null || $url === '') { + $url = $url ?? ''; + + //Don't allow the user to set an empty external path if the internal path is empty already + if ($url === '' && !$this->hasInternal()) { return $this; } - $url = trim($url); - //Escape spaces in URL - $url = str_replace(' ', '%20', $url); - - //Only set if the URL is not empty - if ($url !== '') { - if (str_contains($url, '%BASE%') || str_contains($url, '%MEDIA%')) { - throw new InvalidArgumentException('You can not reference internal files via the url field! But nice try!'); - } - - $this->path = $url; - //Reset internal filename - $this->original_filename = null; + //The URL field can also contain the special builtin internal paths, so we need to distinguish here + if ($this::checkIfBuiltin($url)) { + $this->setInternalPath($url); + //make sure the external path isn't still pointing to something unrelated + $this->setExternalPath(''); + } else { + $this->setExternalPath($url); } + return $this; + } + + + /** + * Sets the path to a file hosted on an external server. Setting the external path to a (nonempty) value different + * from the the old one _clears_ the internal path, so that the external path reflects where any associated internal + * file came from. + */ + public function setExternalPath(?string $external_path): self + { + //If we only clear the external path, don't reset the internal path, since that could be confusing + if($external_path === null || $external_path === '') { + $this->external_path = ''; + return $this; + } + + $external_path = trim($external_path); + //Escape spaces in URL + $external_path = str_replace(' ', '%20', $external_path); + + if($this->external_path === $external_path) { + //Nothing changed, nothing to do + return $this; + } + + $this->external_path = $external_path; + $this->internal_path = ''; + //Reset internal filename + $this->original_filename = null; return $this; } diff --git a/src/EntityListeners/AttachmentDeleteListener.php b/src/EntityListeners/AttachmentDeleteListener.php index e9df5972..1f39b2d0 100644 --- a/src/EntityListeners/AttachmentDeleteListener.php +++ b/src/EntityListeners/AttachmentDeleteListener.php @@ -52,8 +52,8 @@ class AttachmentDeleteListener #[PreUpdate] public function preUpdateHandler(Attachment $attachment, PreUpdateEventArgs $event): void { - if ($event->hasChangedField('path')) { - $old_path = $event->getOldValue('path'); + if ($event->hasChangedField('internal_path')) { + $old_path = $event->getOldValue('internal_path'); //Dont delete file if the attachment uses a builtin ressource: if (Attachment::checkIfBuiltin($old_path)) { diff --git a/src/Repository/AttachmentRepository.php b/src/Repository/AttachmentRepository.php index 865443d2..a4969bc0 100644 --- a/src/Repository/AttachmentRepository.php +++ b/src/Repository/AttachmentRepository.php @@ -58,7 +58,7 @@ class AttachmentRepository extends DBElementRepository { $qb = $this->createQueryBuilder('attachment'); $qb->select('COUNT(attachment)') - ->where('attachment.path LIKE :like ESCAPE \'#\''); + ->where('attachment.internal_path LIKE :like ESCAPE \'#\''); $qb->setParameter('like', '#%SECURE#%%'); $query = $qb->getQuery(); @@ -66,7 +66,7 @@ class AttachmentRepository extends DBElementRepository } /** - * Gets the count of all external attachments (attachments only containing a URL). + * Gets the count of all external attachments (attachments containing an external path). * * @throws NoResultException * @throws NonUniqueResultException @@ -75,17 +75,14 @@ class AttachmentRepository extends DBElementRepository { $qb = $this->createQueryBuilder('attachment'); $qb->select('COUNT(attachment)') - ->where('ILIKE(attachment.path, :http) = TRUE') - ->orWhere('ILIKE(attachment.path, :https) = TRUE'); - $qb->setParameter('http', 'http://%'); - $qb->setParameter('https', 'https://%'); + ->where('attachment.external_path <> \'\''); $query = $qb->getQuery(); return (int) $query->getSingleScalarResult(); } /** - * Gets the count of all attachments where a user uploaded a file. + * Gets the count of all attachments where a user uploaded a file or a file was downloaded from an external source. * * @throws NoResultException * @throws NonUniqueResultException @@ -94,9 +91,9 @@ class AttachmentRepository extends DBElementRepository { $qb = $this->createQueryBuilder('attachment'); $qb->select('COUNT(attachment)') - ->where('attachment.path LIKE :base ESCAPE \'#\'') - ->orWhere('attachment.path LIKE :media ESCAPE \'#\'') - ->orWhere('attachment.path LIKE :secure ESCAPE \'#\''); + ->where('attachment.internal_path LIKE :base ESCAPE \'#\'') + ->orWhere('attachment.internal_path LIKE :media ESCAPE \'#\'') + ->orWhere('attachment.internal_path LIKE :secure ESCAPE \'#\''); $qb->setParameter('secure', '#%SECURE#%%'); $qb->setParameter('base', '#%BASE#%%'); $qb->setParameter('media', '#%MEDIA#%%'); diff --git a/src/Serializer/AttachmentNormalizer.php b/src/Serializer/AttachmentNormalizer.php index bb167fc6..29f589bb 100644 --- a/src/Serializer/AttachmentNormalizer.php +++ b/src/Serializer/AttachmentNormalizer.php @@ -52,8 +52,8 @@ class AttachmentNormalizer implements NormalizerInterface, NormalizerAwareInterf $context[self::ALREADY_CALLED] = true; $data = $this->normalizer->normalize($object, $format, $context); + $data['internal_path'] = $this->attachmentURLGenerator->getInternalViewURL($object); - $data['media_url'] = $this->attachmentURLGenerator->getViewURL($object); //Add thumbnail url if the attachment is a picture $data['thumbnail_url'] = $object->isPicture() ? $this->attachmentURLGenerator->getThumbnailURL($object) : null; diff --git a/src/Services/Attachments/AttachmentManager.php b/src/Services/Attachments/AttachmentManager.php index 4429179e..1075141b 100644 --- a/src/Services/Attachments/AttachmentManager.php +++ b/src/Services/Attachments/AttachmentManager.php @@ -44,35 +44,31 @@ class AttachmentManager * * @param Attachment $attachment The attachment for which the file should be generated * - * @return SplFileInfo|null The fileinfo for the attachment file. Null, if the attachment is external or has + * @return SplFileInfo|null The fileinfo for the attachment file. Null, if the attachment is only external or has * invalid file. */ public function attachmentToFile(Attachment $attachment): ?SplFileInfo { - if ($attachment->isExternal() || !$this->isFileExisting($attachment)) { + if (!$this->isInternalFileExisting($attachment)) { return null; } - return new SplFileInfo($this->toAbsoluteFilePath($attachment)); + return new SplFileInfo($this->toAbsoluteInternalFilePath($attachment)); } /** - * Returns the absolute filepath of the attachment. Null is returned, if the attachment is externally saved, - * or is not existing. + * Returns the absolute filepath to the internal copy of the attachment. Null is returned, if the attachment is + * only externally saved, or is not existing. * * @param Attachment $attachment The attachment for which the filepath should be determined */ - public function toAbsoluteFilePath(Attachment $attachment): ?string + public function toAbsoluteInternalFilePath(Attachment $attachment): ?string { - if ($attachment->getPath() === '') { + if (!$attachment->hasInternal()){ return null; } - if ($attachment->isExternal()) { - return null; - } - - $path = $this->pathResolver->placeholderToRealPath($attachment->getPath()); + $path = $this->pathResolver->placeholderToRealPath($attachment->getInternalPath()); //realpath does not work with null as argument if (null === $path) { @@ -89,8 +85,8 @@ class AttachmentManager } /** - * Checks if the file in this attachement is existing. This works for files on the HDD, and for URLs - * (it's not checked if the ressource behind the URL is really existing, so for every external attachment true is returned). + * Checks if the file in this attachment is existing. This works for files on the HDD, and for URLs + * (it's not checked if the resource behind the URL is really existing, so for every external attachment true is returned). * * @param Attachment $attachment The attachment for which the existence should be checked * @@ -98,15 +94,23 @@ class AttachmentManager */ public function isFileExisting(Attachment $attachment): bool { - if ($attachment->getPath() === '') { - return false; - } - - if ($attachment->isExternal()) { + if($attachment->hasExternal()){ return true; } + return $this->isInternalFileExisting($attachment); + } - $absolute_path = $this->toAbsoluteFilePath($attachment); + /** + * Checks if the internal file in this attachment is existing. Returns false if the attachment doesn't have an + * internal file. + * + * @param Attachment $attachment The attachment for which the existence should be checked + * + * @return bool true if the file is existing + */ + public function isInternalFileExisting(Attachment $attachment): bool + { + $absolute_path = $this->toAbsoluteInternalFilePath($attachment); if (null === $absolute_path) { return false; @@ -117,21 +121,17 @@ class AttachmentManager /** * Returns the filesize of the attachments in bytes. - * For external attachments or not existing attachments, null is returned. + * For purely external attachments or inexistent attachments, null is returned. * * @param Attachment $attachment the filesize for which the filesize should be calculated */ public function getFileSize(Attachment $attachment): ?int { - if ($attachment->isExternal()) { + if (!$this->isInternalFileExisting($attachment)) { return null; } - if (!$this->isFileExisting($attachment)) { - return null; - } - - $tmp = filesize($this->toAbsoluteFilePath($attachment)); + $tmp = filesize($this->toAbsoluteInternalFilePath($attachment)); return false !== $tmp ? $tmp : null; } diff --git a/src/Services/Attachments/AttachmentReverseSearch.php b/src/Services/Attachments/AttachmentReverseSearch.php index 5f4f86de..e05192d0 100644 --- a/src/Services/Attachments/AttachmentReverseSearch.php +++ b/src/Services/Attachments/AttachmentReverseSearch.php @@ -55,7 +55,7 @@ class AttachmentReverseSearch $repo = $this->em->getRepository(Attachment::class); return $repo->findBy([ - 'path' => [$relative_path_new, $relative_path_old], + 'internal_path' => [$relative_path_new, $relative_path_old], ]); } diff --git a/src/Services/Attachments/AttachmentSubmitHandler.php b/src/Services/Attachments/AttachmentSubmitHandler.php index d9b2a380..d079644e 100644 --- a/src/Services/Attachments/AttachmentSubmitHandler.php +++ b/src/Services/Attachments/AttachmentSubmitHandler.php @@ -207,7 +207,7 @@ class AttachmentSubmitHandler if ($file instanceof UploadedFile) { $this->upload($attachment, $file, $secure_attachment); - } elseif ($upload->downloadUrl && $attachment->isExternal()) { + } elseif ($upload->downloadUrl && $attachment->hasExternal()) { $this->downloadURL($attachment, $secure_attachment); } @@ -244,12 +244,12 @@ class AttachmentSubmitHandler protected function renameBlacklistedExtensions(Attachment $attachment): Attachment { //We can not do anything on builtins or external ressources - if ($attachment->isBuiltIn() || $attachment->isExternal()) { + if ($attachment->isBuiltIn() || !$attachment->hasInternal()) { return $attachment; } //Determine the old filepath - $old_path = $this->pathResolver->placeholderToRealPath($attachment->getPath()); + $old_path = $this->pathResolver->placeholderToRealPath($attachment->getInternalPath()); if ($old_path === null || $old_path === '' || !file_exists($old_path)) { return $attachment; } @@ -267,7 +267,7 @@ class AttachmentSubmitHandler $fs->rename($old_path, $new_path); //Update the attachment - $attachment->setPath($this->pathResolver->realPathToPlaceholder($new_path)); + $attachment->setInternalPath($this->pathResolver->realPathToPlaceholder($new_path)); } @@ -275,17 +275,17 @@ class AttachmentSubmitHandler } /** - * Move the given attachment to secure location (or back to public folder) if needed. + * Move the internal copy of the given attachment to a secure location (or back to public folder) if needed. * * @param Attachment $attachment the attachment for which the file should be moved * @param bool $secure_location this value determines, if the attachment is moved to the secure or public folder * - * @return Attachment The attachment with the updated filepath + * @return Attachment The attachment with the updated internal filepath */ protected function moveFile(Attachment $attachment, bool $secure_location): Attachment { //We can not do anything on builtins or external ressources - if ($attachment->isBuiltIn() || $attachment->isExternal()) { + if ($attachment->isBuiltIn() || !$attachment->hasInternal()) { return $attachment; } @@ -295,7 +295,7 @@ class AttachmentSubmitHandler } //Determine the old filepath - $old_path = $this->pathResolver->placeholderToRealPath($attachment->getPath()); + $old_path = $this->pathResolver->placeholderToRealPath($attachment->getInternalPath()); if (!file_exists($old_path)) { return $attachment; } @@ -319,7 +319,7 @@ class AttachmentSubmitHandler //Save info to attachment entity $new_path = $this->pathResolver->realPathToPlaceholder($new_path); - $attachment->setPath($new_path); + $attachment->setInternalPath($new_path); return $attachment; } @@ -329,7 +329,7 @@ class AttachmentSubmitHandler * * @param bool $secureAttachment True if the file should be moved to the secure attachment storage * - * @return Attachment The attachment with the new filepath + * @return Attachment The attachment with the downloaded copy */ protected function downloadURL(Attachment $attachment, bool $secureAttachment): Attachment { @@ -338,7 +338,7 @@ class AttachmentSubmitHandler throw new RuntimeException('Download of attachments is not allowed!'); } - $url = $attachment->getURL(); + $url = $attachment->getExternalPath(); $fs = new Filesystem(); $attachment_folder = $this->generateAttachmentPath($attachment, $secureAttachment); @@ -399,7 +399,7 @@ class AttachmentSubmitHandler //Make our file path relative to %BASE% $new_path = $this->pathResolver->realPathToPlaceholder($new_path); //Save the path to the attachment - $attachment->setPath($new_path); + $attachment->setInternalPath($new_path); } catch (TransportExceptionInterface) { throw new AttachmentDownloadException('Transport error!'); } @@ -427,7 +427,9 @@ class AttachmentSubmitHandler //Make our file path relative to %BASE% $file_path = $this->pathResolver->realPathToPlaceholder($file_path); //Save the path to the attachment - $attachment->setPath($file_path); + $attachment->setInternalPath($file_path); + //reset any external paths the attachment might have had + $attachment->setExternalPath(''); //And save original filename $attachment->setFilename($file->getClientOriginalName()); diff --git a/src/Services/Attachments/AttachmentURLGenerator.php b/src/Services/Attachments/AttachmentURLGenerator.php index d28a8d65..c22cefe4 100644 --- a/src/Services/Attachments/AttachmentURLGenerator.php +++ b/src/Services/Attachments/AttachmentURLGenerator.php @@ -92,9 +92,9 @@ class AttachmentURLGenerator * Returns a URL under which the attachment file can be viewed. * @return string|null The URL or null if the attachment file is not existing */ - public function getViewURL(Attachment $attachment): ?string + public function getInternalViewURL(Attachment $attachment): ?string { - $absolute_path = $this->attachmentHelper->toAbsoluteFilePath($attachment); + $absolute_path = $this->attachmentHelper->toAbsoluteInternalFilePath($attachment); if (null === $absolute_path) { return null; } @@ -111,6 +111,7 @@ class AttachmentURLGenerator /** * Returns a URL to a thumbnail of the attachment file. + * For external files the original URL is returned. * @return string|null The URL or null if the attachment file is not existing */ public function getThumbnailURL(Attachment $attachment, string $filter_name = 'thumbnail_sm'): ?string @@ -119,11 +120,14 @@ class AttachmentURLGenerator throw new InvalidArgumentException('Thumbnail creation only works for picture attachments!'); } - if ($attachment->isExternal() && ($attachment->getURL() !== null && $attachment->getURL() !== '')) { - return $attachment->getURL(); + if (!$attachment->hasInternal()){ + if($attachment->hasExternal()) { + return $attachment->getExternalPath(); + } + return null; } - $absolute_path = $this->attachmentHelper->toAbsoluteFilePath($attachment); + $absolute_path = $this->attachmentHelper->toAbsoluteInternalFilePath($attachment); if (null === $absolute_path) { return null; } @@ -137,7 +141,7 @@ class AttachmentURLGenerator //GD can not work with SVG, so serve it directly... //We can not use getExtension here, because it uses the original filename and not the real extension //Instead we use the logic, which is also used to determine if the attachment is a picture - $extension = pathinfo(parse_url($attachment->getPath(), PHP_URL_PATH) ?? '', PATHINFO_EXTENSION); + $extension = pathinfo(parse_url($attachment->getInternalPath(), PHP_URL_PATH) ?? '', PATHINFO_EXTENSION); if ('svg' === $extension) { return $this->assets->getUrl($asset_path); } @@ -157,7 +161,7 @@ class AttachmentURLGenerator /** * Returns a download link to the file associated with the attachment. */ - public function getDownloadURL(Attachment $attachment): string + public function getInternalDownloadURL(Attachment $attachment): string { //Redirect always to download controller, which sets the correct headers for downloading: return $this->urlGenerator->generate('attachment_download', ['id' => $attachment->getID()]); diff --git a/src/Services/EntityMergers/Mergers/EntityMergerHelperTrait.php b/src/Services/EntityMergers/Mergers/EntityMergerHelperTrait.php index a5c9a5fa..64c952a9 100644 --- a/src/Services/EntityMergers/Mergers/EntityMergerHelperTrait.php +++ b/src/Services/EntityMergers/Mergers/EntityMergerHelperTrait.php @@ -247,7 +247,8 @@ trait EntityMergerHelperTrait { return $this->mergeCollections($target, $other, 'attachments', fn(Attachment $t, Attachment $o): bool => $t->getName() === $o->getName() && $t->getAttachmentType() === $o->getAttachmentType() - && $t->getPath() === $o->getPath()); + && $t->getExternalPath() === $o->getExternalPath() + && $t->getInternalPath() === $o->getInternalPath()); } /** diff --git a/src/Services/EntityURLGenerator.php b/src/Services/EntityURLGenerator.php index 5718daec..c66b5fd9 100644 --- a/src/Services/EntityURLGenerator.php +++ b/src/Services/EntityURLGenerator.php @@ -156,25 +156,32 @@ class EntityURLGenerator public function viewURL(Attachment $entity): string { - if ($entity->isExternal()) { //For external attachments, return the link to external path - return $entity->getURL() ?? throw new \RuntimeException('External attachment has no URL!'); + if ($entity->hasInternal()) { + return $this->attachmentURLGenerator->getInternalViewURL($entity); } - //return $this->urlGenerator->generate('attachment_view', ['id' => $entity->getID()]); - return $this->attachmentURLGenerator->getViewURL($entity) ?? ''; + + if($entity->hasExternal()) { + return $entity->getExternalPath(); + } + + throw new \RuntimeException('Attachment has no internal nor external path!'); } public function downloadURL($entity): string { - if ($entity instanceof Attachment) { - if ($entity->isExternal()) { //For external attachments, return the link to external path - return $entity->getURL() ?? throw new \RuntimeException('External attachment has no URL!'); - } - - return $this->attachmentURLGenerator->getDownloadURL($entity); + if (!($entity instanceof Attachment)) { + throw new EntityNotSupportedException(sprintf('The given entity is not supported yet! Passed class type: %s', $entity::class)); } - //Otherwise throw an error - throw new EntityNotSupportedException(sprintf('The given entity is not supported yet! Passed class type: %s', $entity::class)); + if ($entity->hasInternal()) { + return $this->attachmentURLGenerator->getInternalDownloadURL($entity); + } + + if($entity->hasExternal()) { + return $entity->getExternalPath(); + } + + throw new \RuntimeException('Attachment has not internal or external path!'); } /** diff --git a/src/Services/ImportExportSystem/PartKeeprImporter/PKImportHelperTrait.php b/src/Services/ImportExportSystem/PartKeeprImporter/PKImportHelperTrait.php index 5489fc8c..1e4cd3ba 100644 --- a/src/Services/ImportExportSystem/PartKeeprImporter/PKImportHelperTrait.php +++ b/src/Services/ImportExportSystem/PartKeeprImporter/PKImportHelperTrait.php @@ -105,7 +105,7 @@ trait PKImportHelperTrait //Next comes the filename plus extension $path .= '/'.$attachment_row['filename'].'.'.$attachment_row['extension']; - $attachment->setPath($path); + $attachment->setInternalPath($path); return $attachment; } diff --git a/src/Services/LabelSystem/SandboxedTwigFactory.php b/src/Services/LabelSystem/SandboxedTwigFactory.php index cdf0594f..d6ea6968 100644 --- a/src/Services/LabelSystem/SandboxedTwigFactory.php +++ b/src/Services/LabelSystem/SandboxedTwigFactory.php @@ -122,8 +122,8 @@ final class SandboxedTwigFactory 'getFullPath', 'getPathArray', 'getSubelements', 'getChildren', 'isNotSelectable', ], AbstractCompany::class => ['getAddress', 'getPhoneNumber', 'getFaxNumber', 'getEmailAddress', 'getWebsite', 'getAutoProductUrl'], AttachmentContainingDBElement::class => ['getAttachments', 'getMasterPictureAttachment'], - Attachment::class => ['isPicture', 'is3DModel', 'isExternal', 'isSecure', 'isBuiltIn', 'getExtension', - 'getElement', 'getURL', 'getHost', 'getFilename', 'getAttachmentType', 'getShowInTable', ], + Attachment::class => ['isPicture', 'is3DModel', 'hasExternal', 'hasInternal', 'isSecure', 'isBuiltIn', 'getExtension', + 'getElement', 'getExternalPath', 'getHost', 'getFilename', 'getAttachmentType', 'getShowInTable'], AbstractParameter::class => ['getFormattedValue', 'getGroup', 'getSymbol', 'getValueMin', 'getValueMax', 'getValueTypical', 'getUnit', 'getValueText', ], MeasurementUnit::class => ['getUnit', 'isInteger', 'useSIPrefix'], diff --git a/templates/parts/edit/edit_form_styles.html.twig b/templates/parts/edit/edit_form_styles.html.twig index cf9a40b9..084c073e 100644 --- a/templates/parts/edit/edit_form_styles.html.twig +++ b/templates/parts/edit/edit_form_styles.html.twig @@ -154,26 +154,24 @@ {% set attach = form.vars.value %} {% if attach is not null %} - {% if attachment_manager.fileExisting(attach) %} - {% if not attach.external %} -

-
- - {{ attach.filename }} - -
- - {{ attachment_manager.humanFileSize(attach) }} - -
- {% else %} -

-
+ {% if not attach.hasInternal() %} +

+
{% trans %}attachment.external{% endtrans %} -
- {% endif %} + + {% elseif attachment_manager.isInternalFileExisting(attach) %} +

+
+ + {{ attach.filename }} + +
+ + {{ attachment_manager.humanFileSize(attach) }} + +
{% if attach.secure %}
diff --git a/templates/parts/info/_attachments_info.html.twig b/templates/parts/info/_attachments_info.html.twig index 995c69eb..cc3ede5f 100644 --- a/templates/parts/info/_attachments_info.html.twig +++ b/templates/parts/info/_attachments_info.html.twig @@ -24,18 +24,16 @@ {{ attachment.name }} {{ attachment.attachmentType.fullPath }} - {% if attachment.external %} - {{ attachment.host }} - {% else %} + {% if attachment.hasInternal() %} {{ attachment.filename }} {% endif %} - {% if attachment.external %} + {% if not attachment.hasInternal() %} {% trans %}attachment.external{% endtrans %} - {% elseif attachment_manager.fileExisting(attachment) %} + {% elseif attachment_manager.internalFileExisting(attachment) %} {{ attachment_manager.humanFileSize(attachment) }} @@ -58,13 +56,18 @@
- {# TODO: translate #} + + + - diff --git a/tests/API/Endpoints/AttachmentsEndpointTest.php b/tests/API/Endpoints/AttachmentsEndpointTest.php index 8084db5c..8f4d7e77 100644 --- a/tests/API/Endpoints/AttachmentsEndpointTest.php +++ b/tests/API/Endpoints/AttachmentsEndpointTest.php @@ -91,7 +91,7 @@ class AttachmentsEndpointTest extends AuthenticatedApiTestCase //Attachment must be set (not null) $array = json_decode($response->getContent(), true); - self::assertNotNull($array['media_url']); + self::assertNotNull($array['internal_path']); //Attachment must be private self::assertJsonContains([ diff --git a/tests/Entity/Attachments/AttachmentTest.php b/tests/Entity/Attachments/AttachmentTest.php index a2179e53..1c21abb0 100644 --- a/tests/Entity/Attachments/AttachmentTest.php +++ b/tests/Entity/Attachments/AttachmentTest.php @@ -59,14 +59,15 @@ class AttachmentTest extends TestCase $this->assertNull($attachment->getAttachmentType()); $this->assertFalse($attachment->isPicture()); - $this->assertFalse($attachment->isExternal()); + $this->assertFalse($attachment->hasExternal()); + $this->assertFalse($attachment->hasInternal()); $this->assertFalse($attachment->isSecure()); $this->assertFalse($attachment->isBuiltIn()); $this->assertFalse($attachment->is3DModel()); $this->assertFalse($attachment->getShowInTable()); - $this->assertEmpty($attachment->getPath()); + $this->assertEmpty($attachment->getInternalPath()); + $this->assertEmpty($attachment->getExternalPath()); $this->assertEmpty($attachment->getName()); - $this->assertEmpty($attachment->getURL()); $this->assertEmpty($attachment->getExtension()); $this->assertNull($attachment->getElement()); $this->assertEmpty($attachment->getFilename()); @@ -119,76 +120,56 @@ class AttachmentTest extends TestCase $attachment->setElement($element); } - public function externalDataProvider(): \Iterator - { - yield ['', false]; - yield ['%MEDIA%/foo/bar.txt', false]; - yield ['%BASE%/foo/bar.jpg', false]; - yield ['%FOOTPRINTS%/foo/bar.jpg', false]; - yield ['%FOOTPRINTS3D%/foo/bar.jpg', false]; - yield ['%SECURE%/test.txt', false]; - yield ['%test%/foo/bar.ghp', true]; - yield ['foo%MEDIA%/foo.jpg', true]; - yield ['foo%MEDIA%/%BASE%foo.jpg', true]; - } - - /** - * @dataProvider externalDataProvider - */ - public function testIsExternal($path, $expected): void - { - $attachment = new PartAttachment(); - $this->setProtectedProperty($attachment, 'path', $path); - $this->assertSame($expected, $attachment->isExternal()); - } public function extensionDataProvider(): \Iterator { - yield ['%MEDIA%/foo/bar.txt', null, 'txt']; - yield ['%MEDIA%/foo/bar.JPeg', null, 'jpeg']; - yield ['%MEDIA%/foo/bar.JPeg', 'test.txt', 'txt']; - yield ['%MEDIA%/foo/bar', null, '']; - yield ['%MEDIA%/foo.bar', 'bar', '']; - yield ['http://google.de', null, null]; - yield ['https://foo.bar', null, null]; - yield ['https://foo.bar/test.jpeg', null, null]; - yield ['test', null, null]; - yield ['test.txt', null, null]; + yield ['%MEDIA%/foo/bar.txt', 'http://google.de', null, 'txt']; + yield ['%MEDIA%/foo/bar.JPeg', 'https://foo.bar', null, 'jpeg']; + yield ['%MEDIA%/foo/bar.JPeg', '', 'test.txt', 'txt']; + yield ['%MEDIA%/foo/bar', 'https://foo.bar/test.jpeg', null, '']; + yield ['%MEDIA%/foo.bar', 'test.txt', 'bar', '']; + yield ['', 'http://google.de', null, null]; + yield ['', 'https://foo.bar', null, null]; + yield ['', ',https://foo.bar/test.jpeg', null, null]; + yield ['', 'test', null, null]; + yield ['', 'test.txt', null, null]; } /** * @dataProvider extensionDataProvider */ - public function testGetExtension($path, $originalFilename, $expected): void + public function testGetExtension($internal_path, $external_path, $originalFilename, $expected): void { $attachment = new PartAttachment(); - $this->setProtectedProperty($attachment, 'path', $path); + $this->setProtectedProperty($attachment, 'internal_path', $internal_path); + $this->setProtectedProperty($attachment, 'external_path', $external_path); $this->setProtectedProperty($attachment, 'original_filename', $originalFilename); $this->assertSame($expected, $attachment->getExtension()); } public function pictureDataProvider(): \Iterator { - yield ['%MEDIA%/foo/bar.txt', false]; - yield ['https://test.de/picture.jpeg', true]; - yield ['https://test.de/picture.png?test=fdsj&width=34', true]; - yield ['https://invalid.invalid/file.txt', false]; - yield ['http://infsf.inda/file.zip?test', false]; - yield ['https://test.de', true]; - yield ['https://invalid.com/invalid/pic', true]; - yield ['%MEDIA%/foo/bar.jpeg', true]; - yield ['%MEDIA%/foo/bar.webp', true]; - yield ['%MEDIA%/foo', false]; - yield ['%SECURE%/foo.txt/test', false]; + yield ['', '%MEDIA%/foo/bar.txt', false]; + yield ['', 'https://test.de/picture.jpeg', true]; + yield ['', 'https://test.de/picture.png?test=fdsj&width=34', true]; + yield ['', 'https://invalid.invalid/file.txt', false]; + yield ['', 'http://infsf.inda/file.zip?test', false]; + yield ['', 'https://test.de', true]; + yield ['', 'https://invalid.com/invalid/pic', true]; + yield ['%MEDIA%/foo/bar.jpeg', 'https://invalid.invalid/file.txt', true]; + yield ['%MEDIA%/foo/bar.webp', '', true]; + yield ['%MEDIA%/foo', '', false]; + yield ['%SECURE%/foo.txt/test', 'https://test.de/picture.jpeg', false]; } /** * @dataProvider pictureDataProvider */ - public function testIsPicture($path, $expected): void + public function testIsPicture($internal_path, $external_path, $expected): void { $attachment = new PartAttachment(); - $this->setProtectedProperty($attachment, 'path', $path); + $this->setProtectedProperty($attachment, 'internal_path', $internal_path); + $this->setProtectedProperty($attachment, 'external_path', $external_path); $this->assertSame($expected, $attachment->isPicture()); } @@ -208,7 +189,7 @@ class AttachmentTest extends TestCase public function testIsBuiltIn($path, $expected): void { $attachment = new PartAttachment(); - $this->setProtectedProperty($attachment, 'path', $path); + $this->setProtectedProperty($attachment, 'internal_path', $path); $this->assertSame($expected, $attachment->isBuiltIn()); } @@ -225,52 +206,56 @@ class AttachmentTest extends TestCase public function testGetHost($path, $expected): void { $attachment = new PartAttachment(); - $this->setProtectedProperty($attachment, 'path', $path); + $this->setProtectedProperty($attachment, 'external_path', $path); $this->assertSame($expected, $attachment->getHost()); } public function filenameProvider(): \Iterator { - yield ['%MEDIA%/foo/bar.txt', null, 'bar.txt']; - yield ['%MEDIA%/foo/bar.JPeg', 'test.txt', 'test.txt']; - yield ['https://www.google.de/test.txt', null, null]; + yield ['%MEDIA%/foo/bar.txt', 'https://www.google.de/test.txt', null, 'bar.txt']; + yield ['%MEDIA%/foo/bar.JPeg', 'https://www.google.de/foo.txt', 'test.txt', 'test.txt']; + yield ['', 'https://www.google.de/test.txt', null, null]; } /** * @dataProvider filenameProvider */ - public function testGetFilename($path, $original_filename, $expected): void + public function testGetFilename($internal_path, $external_path, $original_filename, $expected): void { $attachment = new PartAttachment(); - $this->setProtectedProperty($attachment, 'path', $path); + $this->setProtectedProperty($attachment, 'internal_path', $internal_path); + $this->setProtectedProperty($attachment, 'external_path', $external_path); $this->setProtectedProperty($attachment, 'original_filename', $original_filename); $this->assertSame($expected, $attachment->getFilename()); } - public function testSetURL(): void + public function testSetExternalPath(): void { $attachment = new PartAttachment(); //Set URL - $attachment->setURL('https://google.de'); - $this->assertSame('https://google.de', $attachment->getURL()); + $attachment->setExternalPath('https://google.de'); + $this->assertSame('https://google.de', $attachment->getExternalPath()); - //Ensure that an empty url does not overwrite the existing one - $attachment->setPath('%MEDIA%/foo/bar.txt'); - $attachment->setURL(' '); - $this->assertSame('%MEDIA%/foo/bar.txt', $attachment->getPath()); + //Ensure that changing the external path does reset the internal one + $attachment->setInternalPath('%MEDIA%/foo/bar.txt'); + $attachment->setExternalPath('https://example.de'); + $this->assertSame('', $attachment->getInternalPath()); + + //Ensure that setting the same value to the external path again doesn't reset the internal one + $attachment->setExternalPath('https://google.de'); + $attachment->setInternalPath('%MEDIA%/foo/bar.txt'); + $attachment->setExternalPath('https://google.de'); + $this->assertSame('%MEDIA%/foo/bar.txt', $attachment->getInternalPath()); + + //Ensure that resetting the external path doesn't reset the internal one + $attachment->setInternalPath('%MEDIA%/foo/bar.txt'); + $attachment->setExternalPath(''); + $this->assertSame('%MEDIA%/foo/bar.txt', $attachment->getInternalPath()); //Ensure that spaces get replaced by %20 - $attachment->setURL('https://google.de/test file.txt'); - $this->assertSame('https://google.de/test%20file.txt', $attachment->getURL()); - } - - public function testSetURLForbiddenURL(): void - { - $attachment = new PartAttachment(); - - $this->expectException(InvalidArgumentException::class); - $attachment->setURL('%MEDIA%/foo/bar.txt'); + $attachment->setExternalPath('https://example.de/test file.txt'); + $this->assertSame('https://example.de/test%20file.txt', $attachment->getExternalPath()); } public function testIsURL(): void diff --git a/translations/messages.en.xlf b/translations/messages.en.xlf index 77dd22da..e1c19d7c 100644 --- a/translations/messages.en.xlf +++ b/translations/messages.en.xlf @@ -791,7 +791,7 @@ The user will have to set up all two-factor authentication methods again and pri attachment.external - External + External only @@ -817,7 +817,7 @@ The user will have to set up all two-factor authentication methods again and pri attachment.view - View + View Local Copy @@ -2126,7 +2126,7 @@ Sub elements will be moved upwards. attachment.download - Download + Download Local Copy