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

Initial work on #482. #487

Merged
merged 2 commits into from
Sep 14, 2018
Merged

Initial work on #482. #487

merged 2 commits into from
Sep 14, 2018

Conversation

mjordan
Copy link
Collaborator

@mjordan mjordan commented Aug 29, 2018

Github issue: (#482)

  • Other Relevant Links (Google Groups discussion, related pull requests, etc.)

What does this Pull Request do?

Allows use of CONTENTdm metadata field "nicknames" in MIK mappings files and Twig-based metadata manipulators.

What's new?

The nicknames are used internally in MIK, but we go out of our way to allow the use of the corresponding human-readable field labels in mappings files. This change effectively bypasses that conversion.

How should this be tested?

@bondjimbond has already tested this thoroughly, but checking out this branch, then following the documentation in how to configure this option in https://github.com/MarcusBarnes/mik/wiki/Cookbook:-Using-CONTENTdm-field-nicknames-instead-of-labels should be sufficient.

Additional Notes

  • Does this change require documentation to be updated?

https://github.com/MarcusBarnes/mik/wiki/Cookbook:-Using-CONTENTdm-field-nicknames-instead-of-labels created, and can be linked from the Cdm toolchain wiki pages and https://github.com/MarcusBarnes/mik/wiki/Metadata-manipulator:-InsertXmlFromTemplate.

  • Does this change add any new dependencies?

No.

  • Could this change impact execution of existing code?

Yes.

@mjordan
Copy link
Collaborator Author

mjordan commented Aug 29, 2018

Failing on whitespace in one of the test files.... which is strange since I didn't touch that file. I'll investigate.

@bondjimbond
Copy link
Collaborator

Anything beyond the work I've already been doing that you think should be tested? Any places where you see potential problems/conflicts, etc?

@bondjimbond
Copy link
Collaborator

NormalizeDate: Can't confirm, because it doesn't work for me regardless of which field I choose...

[2018-08-30 18:33:27] ErrorException.ERROR: ErrorException {"message":"problem with metadataparser","record_key":661,"details":"[object] (mik\exceptions\MikErrorException(code: 0): at /Users/brandon/sfuvault/mik/mik:105)"} []
[2018-08-30 18:33:27] ErrorException.ERROR: ErrorException {"message":"DOMDocument::loadXML(): Start tag expected, '<' not found in Entity, line: 1","code":{"input":"null4","dom":"[object] (DOMDocument: {})"},"severity":2,"file":"/Users/brandon/sfuvault/mik/src/metadatamanipulators/NormalizeDate.php","line":59}

For the others: well, nothing crashed... SimpleReplace doesn't seem to do anything, and I don't really have the appropriate data for SplitRepeatedValues. I'm not sure that the data I have available is useful for testing those manipulators.

@mjordan
Copy link
Collaborator Author

mjordan commented Aug 30, 2018

Let's not worry about those plugins now, we can address later as needed.

@bondjimbond
Copy link
Collaborator

@MarcusBarnes Any objection to me merging this then?

@MarcusBarnes
Copy link
Owner

@bondjimbond I'm assuming that if use_nicknames = false (i.e., current default behaviour), you don't get the NormalizeDate metadata manipulator issue you mentioned in #487 (comment)? If this is the case, please merge and another issue can be created noting the issue with NormalizeDate and use_nicknames = false.

@bondjimbond
Copy link
Collaborator

@MarcusBarnes If I remove the use_nicknames option, I still get the NormalizeDate problems. I haven't tried with tagging it "false"

@MarcusBarnes
Copy link
Owner

Do you get those NormalizeDates problems if you use the default branch (not the changes in the PR)?

@bondjimbond
Copy link
Collaborator

@MarcusBarnes Yeah, NormalizeDates is still a problem for me. But that's kind of normal; these INI files are rather finicky.

If I hear no objections from you, I'll merge this soon; I'd like to get it into the main branch ASAP.

@MarcusBarnes
Copy link
Owner

@bondjimbond Please go ahead and merged this. Also please create a separate issue regarding the details of how NormalizeDate is failing for your use cases so that the issue is documented. Thanks so much!

@bondjimbond bondjimbond merged commit 2201280 into master Sep 14, 2018
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.

None yet

3 participants