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

ISLANDORA-1555 #216

Merged
merged 2 commits into from
Dec 1, 2015
Merged

ISLANDORA-1555 #216

merged 2 commits into from
Dec 1, 2015

Conversation

matthewperry
Copy link
Contributor

Jira: https://jira.duraspace.org/browse/ISLANDORA-1555

What does this Pull Request do?

It's an improvement to the DOI to allow for the full DOI URL to be used when getting the DOI data.

How should this be tested?

Ingest some objects using the DOI importer.
Old formats should continue to work:
DOI:10.1016/j.jorganchem.2011.11.018
10.1016/eva.12339

In addition to the old formats you should now be able to enter the records using full DOI urls:
http://dx.doi.org/10.1111/eva.12339
http://doi.org/10.1111/eva.12340

Additional Notes:

  • Could this change impact execution of existing code? There is no impact on existing code or functions using islandora_doi_perform_request as it maintains backwards compatibility with passing/processing just the CrossRef ID.

Tagging: @qadan, @ruebot, @DiegoPino, @mjordan


Matthew Perry
Developer
discoverygarden inc. | Managing Digital Content

@qadan
Copy link

qadan commented Nov 26, 2015

I've checked and run this code locally. Okay with the update as the component manager; would like some confirmation slash someone to merge.

@DiegoPino
Copy link
Contributor

@qadan, looks fine for me, it's a simple but very effective change. 👍 Thanks!

$doi_url = parse_url(trim($url));
// Remove "/" from the path if it's the first or last character in the path.
$doi_url['path'] = rtrim($doi_url['path'], '/');
$doi_url['path'] = ltrim($doi_url['path'], '/');
Copy link

Choose a reason for hiding this comment

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

A thought: could just use regular trim()? To ... haha ... trim down the number of lines here?

( •_•)
( •_•)>⌐■-■
(⌐■_■)

pinging @adam-vessey for pun approval

Copy link
Member

Choose a reason for hiding this comment

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

😆

Copy link
Contributor

Choose a reason for hiding this comment

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

😸 ✂️

@qadan
Copy link

qadan commented Nov 26, 2015

Changes look okay to me; I'd say this is ready to get merged in.

@ruebot
Copy link
Member

ruebot commented Nov 26, 2015

👍

I'm happy to hit the merge button if everybody is cool with it.

@mitchmac
Copy link

mitchmac commented Dec 1, 2015

With no comments to the contrary, it seems like this can be merged?

@DiegoPino
Copy link
Contributor

👍 to merge

ruebot added a commit that referenced this pull request Dec 1, 2015
@ruebot ruebot merged commit 59e943c into Islandora:7.x Dec 1, 2015
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.

5 participants