create no-embbed option for exporting issues #4799

Godoy0722 · 2025-04-01T11:22:14Z

Description: I added a --no-embbed option for the CLI and the system interface. It reduces the memory usage, as for faster responses and lighter XML files. It solves the issue #9769.

plugins/importexport/native/NativeImportExportPlugin.inc.php

jonasraoni

@Godoy0722 I've left some comments.

One very important thing that's missing is the way back. I mean, it should be possible to import this new format.

Please, also take a look if there are no more <embed>s in the other applications (OPS and OMP).

plugins/importexport/native/NativeImportExportPlugin.inc.php

plugins/importexport/native/templates/index.tpl

jonasraoni · 2025-04-01T11:45:39Z

plugins/importexport/native/templates/index.tpl

+				{/fbvFormSection}
+
+				{fbvFormSection list="true" title="plugins.importexport.native.exportOptions"}
+					{fbvElement type="checkbox" id="no-embed" name="no-embed" label="plugins.importexport.native.noEmbedOption" checked=$noEmbed|default:false}


Great that you've included the option on the UI, then it makes sense to include the other alternative, it should be a radio (embed | relative | URL).

jonasraoni · 2025-04-01T11:51:44Z

plugins/importexport/native/native.xsd

@@ -135,7 +135,7 @@
 			<element name="original_file_name" type="string" minOccurs="1" maxOccurs="1" />
 			<element name="date_uploaded" type="date" minOccurs="1" maxOccurs="1" />
 			<element name="date_modified" type="date" minOccurs="1" maxOccurs="1" />
-			<element name="embed" type="pkp:embed" />
+			<element name="embed" type="pkp:embed" minOccurs="0" maxOccurs="1" />


Include the href element (min 0/max 1), with the attributes href and mime_type.

The schema should specify that there should be one <href> OR one <embed>

p.s.: There's another occurrence of this update.

jonasraoni · 2025-04-01T13:04:05Z

plugins/importexport/native/filter/IssueGalleyNativeXmlFilter.inc.php

-			$embedNode = $doc->createElementNS($deployment->getNamespace(), 'embed', base64_encode(file_get_contents($filePath)));
-			$embedNode->setAttribute('encoding', 'base64');
-			$issueFileNode->appendChild($embedNode);
+			if (empty($this->opts['no-embed'])) {


Wherever you added support for the no-embed, there should be also the URL (I didn't check, but I assume it's possible), just to keep the standard.

jonasraoni · 2025-04-01T13:08:49Z

plugins/importexport/native/filter/NativeFilterHelper.inc.php

+					$request = Application::get()->getRequest();
+					$baseUrl = $request->getBaseUrl();
+					$context = $deployment->getContext();
+					$fileUrl = $baseUrl . '/' . $context->getPath() . '/public/' . $coverImage;


This public path isn't standard, you need to use the class PublicFileManager.

Co-authored-by: Jonas Raoni Soares da Silva <[email protected]>

Godoy0722 · 2025-04-03T17:48:21Z

@jonasraoni I just made the requested updates. I'll also take a look at OMP and OPS and will open PR forms for them if it's possible to change it there. I also see that my validation was not used anywhere - I forgot to implement the validation, but it's not required, so I removed it.

jonasraoni · 2025-04-11T09:21:14Z

plugins/importexport/native/NativeImportExportPlugin.inc.php

+		$imageOption = $request->getUserVar('image-option');
+		switch ($imageOption) {


It's not just about images, so it's better to use a generic name

Suggested change

$imageOption = $request->getUserVar('image-option');

switch ($imageOption) {

$serializationMode = $request->getUserVar('serializationMode');

jonasraoni · 2025-04-11T09:37:03Z

plugins/importexport/native/filter/IssueGalleyNativeXmlFilter.inc.php

+			$issueFileNode->appendChild($node = $doc->createElementNS($deployment->getNamespace(), 'date_uploaded', $dateUploaded));
+			$issueFileNode->appendChild($node = $doc->createElementNS($deployment->getNamespace(), 'date_modified', $dateModified));


I understand these changes were motivated because the strftime is deprecated. But as there's a polyfill, it's better to revert. I mean, we are trying to decrease the amount of updates on stable branches.

Also, this whole branch is going to die in 1 year after the 3.5 is released, so it's not even worth to fix minor problems (e.g. /* @var type */ instead of /** @var type */).

Suggested change

$issueFileNode->appendChild($node = $doc->createElementNS($deployment->getNamespace(), 'date_uploaded', $dateUploaded));

$issueFileNode->appendChild($node = $doc->createElementNS($deployment->getNamespace(), 'date_modified', $dateModified));

$issueFileNode->appendChild($node = $doc->createElementNS($deployment->getNamespace(), 'date_uploaded', strftime('%Y-%m-%d', strtotime($issueFile->getDateUploaded()))));

$issueFileNode->appendChild($node = $doc->createElementNS($deployment->getNamespace(), 'date_modified', strftime('%Y-%m-%d', strtotime($issueFile->getDateModified()))));

jonasraoni · 2025-04-11T09:37:27Z

plugins/importexport/native/filter/IssueGalleyNativeXmlFilter.inc.php

+			$dateUploaded = $issueFile->getDateUploaded();
+			if (is_string($dateUploaded)) {
+				$dateUploaded = substr($dateUploaded, 0, 10); // Extract YYYY-MM-DD
+			}
+
+			$dateModified = $issueFile->getDateModified();
+			if (is_string($dateModified)) {
+				$dateModified = substr($dateModified, 0, 10); // Extract YYYY-MM-DD
+			}


Suggested change

$dateUploaded = $issueFile->getDateUploaded();

if (is_string($dateUploaded)) {

$dateUploaded = substr($dateUploaded, 0, 10); // Extract YYYY-MM-DD

}

$dateModified = $issueFile->getDateModified();

if (is_string($dateModified)) {

$dateModified = substr($dateModified, 0, 10); // Extract YYYY-MM-DD

}

jonasraoni · 2025-04-11T10:03:33Z

plugins/importexport/native/NativeImportExportPlugin.inc.php

@@ -327,7 +357,7 @@ function usage($scriptName) {
 	 * @see PKPImportExportPlugin::executeCLI()
 	 */
 	function executeCLI($scriptName, &$args) {
-		$opts = $this->parseOpts($args, ['no-embed', 'use-file-urls']);
+		$opts = $this->parseOpts($args, ['no-embed', 'use-file-urls', 'use-absolute-urls', 'image-option:']);


This is a bit confuse, what should happen if I add use-file-urls and use-absolute-urls? I think it's better to just add the serializationMode, and on the main branch we can get rid of the other arguments (or mark them as deprecated).

Suggested change

$opts = $this->parseOpts($args, ['no-embed', 'use-file-urls', 'use-absolute-urls', 'image-option:']);

$opts = $this->parseOpts($args, ['no-embed', 'use-file-urls', 'serializationMode:']);

jonasraoni · 2025-04-11T11:16:02Z

plugins/importexport/native/NativeImportExportPlugin.inc.php

+			case 'embed':
+				break;
+			case 'relative':
+				$opts['use-file-urls'] = true;
+				break;
+			case 'url':
+				$opts['use-file-urls'] = true;
+				$opts['use-absolute-urls'] = true;
+				break;
+			default:
+				if ($request->getUserVar('no-embed')) {
+					$opts['no-embed'] = true;
+				}


Instead of adding all these options, we can have just one, the serializationMode

Suggested change

case 'embed':

break;

case 'relative':

$opts['use-file-urls'] = true;

break;

case 'url':

$opts['use-file-urls'] = true;

$opts['use-absolute-urls'] = true;

break;

default:

if ($request->getUserVar('no-embed')) {

$opts['no-embed'] = true;

}

jonasraoni · 2025-04-11T14:28:42Z

plugins/importexport/native/filter/NativeFilterHelper.inc.php

+					case 'file_url':
+						if (!isset($coverImage['uploadName'])) {
+							$deployment->addWarning(ASSOC_TYPE_PUBLICATION, $object->getId(), __('plugins.importexport.common.error.coverImageNameUnspecified'));
+							break;
+						}
+
+						import('classes.file.PublicFileManager');
+						$publicFileManager = new PublicFileManager();
+						$filePath = $publicFileManager->getContextFilesPath($context->getId()) . '/' . $coverImage['uploadName'];
+						$allowedFileTypes = ['gif', 'jpg', 'png', 'webp'];
+						$extension = pathinfo(strtolower($filePath), PATHINFO_EXTENSION);
+						if (!in_array($extension, $allowedFileTypes)) {
+							$deployment->addWarning(ASSOC_TYPE_PUBLICATION, $object->getId(), __('plugins.importexport.common.error.invalidFileExtension'));
+							break;
+						}
+
+						$imageUrl = $n->textContent;
+						if (empty($imageUrl)) {
+							$deployment->addWarning(ASSOC_TYPE_PUBLICATION, $object->getId(), __('plugins.importexport.common.error.missingFileUrl'));
+							break;
+						}
+
+						$fileContents = file_get_contents($imageUrl);
+						if ($fileContents === false) {
+							$deployment->addWarning(ASSOC_TYPE_PUBLICATION, $object->getId(), __('plugins.importexport.common.error.failedDownloadingFile', array('url' => $imageUrl)));
+							break;
+						}
+
+						file_put_contents($filePath, $fileContents);
+						break;


This whole piece can be removed, once you copy the implementation for the <href>

Suggested change

case 'file_url':

if (!isset($coverImage['uploadName'])) {

$deployment->addWarning(ASSOC_TYPE_PUBLICATION, $object->getId(), __('plugins.importexport.common.error.coverImageNameUnspecified'));

break;

}

import('classes.file.PublicFileManager');

$publicFileManager = new PublicFileManager();

$filePath = $publicFileManager->getContextFilesPath($context->getId()) . '/' . $coverImage['uploadName'];

$allowedFileTypes = ['gif', 'jpg', 'png', 'webp'];

$extension = pathinfo(strtolower($filePath), PATHINFO_EXTENSION);

if (!in_array($extension, $allowedFileTypes)) {

$deployment->addWarning(ASSOC_TYPE_PUBLICATION, $object->getId(), __('plugins.importexport.common.error.invalidFileExtension'));

break;

}

$imageUrl = $n->textContent;

if (empty($imageUrl)) {

$deployment->addWarning(ASSOC_TYPE_PUBLICATION, $object->getId(), __('plugins.importexport.common.error.missingFileUrl'));

break;

}

$fileContents = file_get_contents($imageUrl);

if ($fileContents === false) {

$deployment->addWarning(ASSOC_TYPE_PUBLICATION, $object->getId(), __('plugins.importexport.common.error.failedDownloadingFile', array('url' => $imageUrl)));

break;

}

file_put_contents($filePath, $fileContents);

break;

jonasraoni · 2025-04-11T14:40:34Z

plugins/importexport/native/filter/NativeFilterHelper.inc.php

@@ -284,11 +378,93 @@ function parseIssueCover($filter, $node, $object) {

 						file_put_contents($filePath, base64_decode($n->textContent));
 						break;
+					case 'href':


This is similar to the previous modified code block, so the same comments can be applied

jonasraoni · 2025-04-11T14:42:28Z

plugins/importexport/native/filter/NativeFilterHelper.inc.php

+	/**
+	 * Get MIME type based on file extension
+	 * @param $fileName string
+	 * @return string MIME type
+	 */
+	protected function _getMimeTypeFromFileName($fileName) {
+		$extension = strtolower(pathinfo($fileName, PATHINFO_EXTENSION));
+		switch ($extension) {
+			case 'jpg':
+			case 'jpeg':
+				return 'image/jpeg';
+			case 'png':
+				return 'image/png';
+			case 'gif':
+				return 'image/gif';
+			case 'webp':
+				return 'image/webp';
+			default:
+				return 'application/octet-stream';
+		}
+	}
+


We have such method already in the codebase, but before using it, check if the application doesn't provide it through a getMimeType() (or something like that) method.

Suggested change

/**

* Get MIME type based on file extension

* @param $fileName string

* @return string MIME type

*/

protected function _getMimeTypeFromFileName($fileName) {

$extension = strtolower(pathinfo($fileName, PATHINFO_EXTENSION));

switch ($extension) {

case 'jpg':

case 'jpeg':

return 'image/jpeg';

case 'png':

return 'image/png';

case 'gif':

return 'image/gif';

case 'webp':

return 'image/webp';

default:

return 'application/octet-stream';

}

}

jonasraoni · 2025-04-11T14:43:19Z

plugins/importexport/native/locale/en_US/locale.po

+msgid "plugins.importexport.common.error.missingFileUrl"
+msgstr "The file_url element does not contain a URL."
+
+msgid "plugins.importexport.common.error.failedDownloadingFile"
+msgstr "Failed to download file from URL: {$url}"
+


This can be removed once you copy the other implementation.

Suggested change

msgid "plugins.importexport.common.error.missingFileUrl"

msgstr "The file_url element does not contain a URL."

msgid "plugins.importexport.common.error.failedDownloadingFile"

msgstr "Failed to download file from URL: {$url}"

jonasraoni · 2025-04-11T14:49:33Z

plugins/importexport/native/locale/en_US/locale.po

@@ -125,3 +134,27 @@ msgid "plugins.importexport.native.import.error.publishedDateMissing"
 msgstr ""
 "The article \"{$articleTitle}\" is contained within an issue, but has no "
 "published date."
+
+msgid "plugins.importexport.common.export"
+msgstr "Export"


Generic notes for the locale keys:

Check if generic texts already exist on the localization files and re-use them, this one does exist for example as common.export.

In general, try to not create new locale keys on old branches, these are probably not going to be translated (due to problems in how we manage the translations across all the branches, the old branches are currently left to rot).

If the locale key is going to be re-used in other applications, create them on the pkp-lib submodule, then their translation will be shared.

create no-embbed option for exporting issues

74051f0

jonasraoni linked an issue Apr 1, 2025 that may be closed by this pull request

Native XML consumes too much memory pkp/pkp-lib#9769

Open

jonasraoni reviewed Apr 1, 2025

View reviewed changes

plugins/importexport/native/NativeImportExportPlugin.inc.php Outdated Show resolved Hide resolved

jonasraoni requested changes Apr 1, 2025

View reviewed changes

Godoy0722 and others added 4 commits April 3, 2025 15:43

Changed by Jonas and commited by Guilherme

8ff8475

Co-authored-by: Jonas Raoni Soares da Silva <[email protected]>

Changed by Jonas and commited by Guilherme

f704bb7

Co-authored-by: Jonas Raoni Soares da Silva <[email protected]>

Changed by Jonas and commited by Guilherme

641f11a

Co-authored-by: Jonas Raoni Soares da Silva <[email protected]>

Improve behavior so the user can chose tif the file is embed or an url

95c5b87

jonasraoni requested changes Apr 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create no-embbed option for exporting issues #4799

create no-embbed option for exporting issues #4799

Godoy0722 commented Apr 1, 2025

jonasraoni left a comment

jonasraoni Apr 1, 2025

jonasraoni Apr 1, 2025

jonasraoni Apr 1, 2025

jonasraoni Apr 1, 2025

Godoy0722 commented Apr 3, 2025

jonasraoni Apr 11, 2025

jonasraoni Apr 11, 2025

jonasraoni Apr 11, 2025

jonasraoni Apr 11, 2025

jonasraoni Apr 11, 2025

jonasraoni Apr 11, 2025

jonasraoni Apr 11, 2025

jonasraoni Apr 11, 2025

jonasraoni Apr 11, 2025

jonasraoni Apr 11, 2025

		$imageOption = $request->getUserVar('image-option');
		switch ($imageOption) {

	$imageOption = $request->getUserVar('image-option');
	switch ($imageOption) {
	$serializationMode = $request->getUserVar('serializationMode');

		$issueFileNode->appendChild($node = $doc->createElementNS($deployment->getNamespace(), 'date_uploaded', $dateUploaded));
		$issueFileNode->appendChild($node = $doc->createElementNS($deployment->getNamespace(), 'date_modified', $dateModified));

	$opts = $this->parseOpts($args, ['no-embed', 'use-file-urls', 'use-absolute-urls', 'image-option:']);
	$opts = $this->parseOpts($args, ['no-embed', 'use-file-urls', 'serializationMode:']);

create no-embbed option for exporting issues #4799

Are you sure you want to change the base?

create no-embbed option for exporting issues #4799

Conversation

Godoy0722 commented Apr 1, 2025

jonasraoni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Godoy0722 commented Apr 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment