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

[MAINTENANCE] Format XML output of OAI-PMH #1473

Merged
merged 2 commits into from
Feb 5, 2025
Merged

Conversation

stweil
Copy link
Member

@stweil stweil commented Feb 5, 2025

The current XML output is generated with a Fluid template. Therefore it is badly formatted with lots of empty lines and unnecessary whitespace.

@stweil
Copy link
Member Author

stweil commented Feb 5, 2025

Here are some numbers for http://HOST/oai?verb=ListIdentifiers&metadataPrefix=mets"|wc:

# minimized XML
       3      13    1766
# formatted XML (new code)
      49      59    2046
# unformatted XML (old code)
     172      59    6172

So this pull request reduces the size of the answer for this specific OAI-PMH request from 6172 bytes to 2046 bytes.
A minimized XML would reduce the size a little bit more and could be added as a configuration option in the future.

Signed-off-by: Stefan Weil <[email protected]>
$xmlOutput = $this->view->render();

// Format the XML.
$dom = new \DOMDocument();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$dom = new \DOMDocument();
$dom = new DOMDocument();

Copy link
Member Author

@stweil stweil Feb 5, 2025

Choose a reason for hiding this comment

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

Note that the code already uses new \DOMDocument(); in function getMetsData. This is more safe and efficient than new DOMDocument(); which should only be used if this function were overridden in the current namespace.

Copy link
Member

Choose a reason for hiding this comment

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

Codacy complained because you declared use DOMDocument; at the top of the file, but then added the namespace in this line anyways. The use-statement already makes clear that there is no such function/object in current namespace and the global version should be used.
The reason why getMetsData() uses new \DOMDocument(); is, that before your changes we didn't have an use-statement for DOMDocument. Strictly speaking we should remove the backslash from getMetsData as well, now.

Copy link
Member Author

@stweil stweil Feb 5, 2025

Choose a reason for hiding this comment

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

Codacy complained about my new code before I added the use statement. I added it to fix the Codacy warning.

Copy link
Member

Choose a reason for hiding this comment

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

Both is true: Codacy generally discourages using any namespaces in the code in favor of declaring all used namespaced functions and objects per use-statement.
So Codacy first complained about the missing use-statement and then complained about a namespace in code for which a use-statement exists.

@sebastian-meyer sebastian-meyer added the 🛠 maintenance A task to keep the code up-to-date and manageable. label Feb 5, 2025
@sebastian-meyer sebastian-meyer changed the title Format XML output of OAI-PMH [MAINTENANCE] Format XML output of OAI-PMH Feb 5, 2025
@sebastian-meyer sebastian-meyer merged commit df4bcbe into kitodo:main Feb 5, 2025
8 checks passed
Copy link

codecov bot commented Feb 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (411bfb8) to head (298c5cf).
Report is 1 commits behind head on main.

Additional details and impacted files
@@     Coverage Diff      @@
##   main   #1473   +/-   ##
============================
============================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stweil stweil deleted the oai branch February 5, 2025 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🛠 maintenance A task to keep the code up-to-date and manageable.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants