Skip to content
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

[OMP Native Import/Export Plugin v3.2 #775

Merged
merged 4 commits into from
Feb 28, 2020

Conversation

defstat
Copy link
Contributor

@defstat defstat commented Feb 27, 2020

No description provided.

* Constructor
* @param $filterGroup FilterGroup
*/
function __construct($filterGroup) {
Copy link
Member

Choose a reason for hiding this comment

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

(Another dead constructor)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

* @param $document DOMDocument|string
* @return array Array of imported documents
*/
function &process(&$document) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be removed...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

* Constructor
* @param $filterGroup FilterGroup
*/
function __construct($filterGroup) {
Copy link
Member

Choose a reason for hiding this comment

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

(Another dead constructor)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

}
// if ($publication->getData('subtitle')) {
// $titleElementNode->appendChild($this->_buildTextNode($doc, 'Subtitle', $publication->getData('subtitle')));
// }
Copy link
Member

Choose a reason for hiding this comment

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

Is this commented for a reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$publication->getData('subtitle') is an array (localized), and _buildTextNode expects string. I was looking for alternatives here

Copy link
Member

Choose a reason for hiding this comment

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

ONIX is not multilingual-capable, so I would suggest using the values for the submission's locale. (Generally we should also be avoiding getLocalizedData because it gets the content based on the current user's locale -- exports should not depend on the user's language, so getData('subtitle', $publication->getData('locale')) is preferred.)

* Get the publication node name
* @return string
*/
function getPublicationNodeName() {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this abstraction adds anything, since we're using publication (and publications) throughout -- I think this pattern comes from cases where we need different element names (article in OJS, monograph in OMP). So I'd suggest removing it.

Copy link
Member

Choose a reason for hiding this comment

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

[removed from OJS and pkp-lib, can remove here without anything more]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@asmecher asmecher merged commit 0f19682 into pkp:master Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants