-
Notifications
You must be signed in to change notification settings - Fork 31
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 tokens 1171 #66
Islandora tokens 1171 #66
Conversation
…unt and ASU distinct implements of islandora_tokens modules for #1171
this looks like it would supercede #30 |
controlled_access_terms.tokens.inc
Outdated
* @return mixed | ||
* \Drupal\file\FileInterface on success, NULL otherwise. | ||
*/ | ||
function controlled_access_terms_image_from_media($media) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't rely on Islandora services because this module may be used independently of it.
I would crib from Islandora's Media Source Service for a Drupal solution that supports multiple Media Types (See both https://github.com/Islandora/islandora/blob/8.x-1.x/src/MediaSource/MediaSourceService.php#L121-L133 and https://github.com/Islandora/islandora/blob/8.x-1.x/src/MediaSource/MediaSourceService.php#L97-L107).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't write this part, but I think I can swap out that fixed field-name with the bundle config media type fieldname without breaking it. fingers-crossed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe move the chunks for getting files from Media to islandora
? Hacking out the particular files seems very Islandora specific. Presumably if you were running controlled_access_terms without Islandora, you wouldn't need to jump through the same hoops to get your pdf or thumbnail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok - this makes sense... I can make those changes... so, I infer that there would need to be synchronized pull requests after this so that islandora
as well as controlled_access_terms
are both updated with the whole set of tokens functionality.
Should I just make note of this in both pull requests' descriptions (that there is a related pull request on the other module)?
Ok -- a new branch with the same exact name was made in our fork of the islandora code repository and a similar pull request has been made for that to split out the Media calls from here |
🙇 |
controlled_access_terms.tokens.inc
Outdated
|
||
case 'media-thumbnail-image:url': | ||
case 'media_thumbnail_image:url': | ||
$term = $islandoraUtils->getTermForUri('http://pcdm.org/use#ThumbnailImage'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$islandoraUtils doesn't exist here. Needs $islandoraUtils = \Drupal::service('islandora.utils');
controlled_access_terms.tokens.inc
Outdated
$fid[0]['target_id'] : NULL; | ||
if (!is_null($fid_value)) { | ||
$file = File::load($fid_value); | ||
$url = $file->getFileUri(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$url returned here is fedora://filepath. Needs to include overall site domain.
…x to islandoraUtils object variable for #1171
PHPCS is unhappy on this one https://github.com/Islandora/controlled_access_terms/pull/66/checks?check_run_id=2777626305#step:12:13 @wgilling |
this should now have all of the phpcs updates and be ready to merge |
controlled_access_terms.tokens.inc
Outdated
'name' => t("PDF Url"), | ||
'description' => t('URL to related media file if "Original file" is a PDF file'), | ||
]; | ||
$node['media-thumbnail-image:url'] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
media-thumbnail-image:url
and media-thumbnail-alt
as well as their underscored counterparts can be removed now that they're in the islandora module.
controlled_access_terms.tokens.inc
Outdated
'name' => t("Publication date"), | ||
'description' => t('Show the "Date Created" into YYYY/MM/DD format (handles EDTF format)'), | ||
]; | ||
$node['pdf_url'] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fella ought to be moved into the islandora module since it does the whole 'media lookup by media_use term' dealio.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This guy still needs to get moved over into the islandora module, or you put a conditional around it to check for the islandora module. At least then it'll be a soft dependency instead of an undeclared hard dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok -- this token has now been moved over to the islandora_tokens_1171
branch of the islandora module https://github.com/asulibraries/islandora/tree/islandora_tokens_1171 (as well as renaming the helper function from "controlled_access_terms_*" to lookup the media by mime type).
…ndora tokens file for #1171
…lop values when the delimiter is not a comma for #1171
Ok -- it may be worth it to test out the. code before the metadata patch is appllied... in the event that the configuration for the metadata separation character does not exist, the updated code now assumes to use a comma.
... and then run Be sure that the previous islandora_tokens module is not installed -- and that the current islandora branch is up to date. |
@wgilling Just tried this out on some real data and got an error:
https://github.com/Islandora/controlled_access_terms/pull/66/files#diff-ecff50b0a1ba42fe05498c9dac67f1f04ae584715dad824497951a82c2d045ecR263 assumes File has loaded correctly but in my case it looks like a file has been deleted but the media still (somehow) references it (or the file has failed to load for other reasons). In any case, it would be good to test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wgilling Just tried this out on some real data and got an error:
The website encountered an unexpected error. Please try again later.
Error: Call to a member function getFileUri() on null in controlled_access_terms_url_to_service_file_media_by_mimetype() (line 263 of modules/contrib/controlled_access_terms/controlled_access_terms.tokens.inc).https://github.com/Islandora/controlled_access_terms/pull/66/files#diff-ecff50b0a1ba42fe05498c9dac67f1f04ae584715dad824497951a82c2d045ecR263 assumes File has loaded correctly but in my case it looks like a file has been deleted but the media still (somehow) references it (or the file has failed to load for other reasons). In any case, it would be good to test
is_object($file)
before getting URL.
This token has been moved from controlled_access_terms to the islandora module (https://github.com/asulibraries/islandora/tree/islandora_tokens_1171) and the added logic to test whether or not there is a file has been added.
To test, this may be a problem since the previous instance of this token lived in controlled_access_terms and now that logic all exists in islandora (by the same exact token name [islandoratokens:pdf_url], but I believe that clearing the cache will be sufficient to get the code to execute from the new islandora module location.
controlled_access_terms.tokens.inc
Outdated
* @return mixed | ||
* \Drupal\file\FileInterface on success, NULL otherwise. | ||
*/ | ||
function controlled_access_terms_image_from_media($media) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't write this part, but I think I can swap out that fixed field-name with the bundle config media type fieldname without breaking it. fingers-crossed.
controlled_access_terms.tokens.inc
Outdated
* @return mixed | ||
* \Drupal\file\FileInterface on success, NULL otherwise. | ||
*/ | ||
function controlled_access_terms_image_from_media($media) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok - this makes sense... I can make those changes... so, I infer that there would need to be synchronized pull requests after this so that islandora
as well as controlled_access_terms
are both updated with the whole set of tokens functionality.
Should I just make note of this in both pull requests' descriptions (that there is a related pull request on the other module)?
controlled_access_terms.tokens.inc
Outdated
'name' => t("Publication date"), | ||
'description' => t('Show the "Date Created" into YYYY/MM/DD format (handles EDTF format)'), | ||
]; | ||
$node['pdf_url'] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok -- this token has now been moved over to the islandora_tokens_1171
branch of the islandora module https://github.com/asulibraries/islandora/tree/islandora_tokens_1171 (as well as renaming the helper function from "controlled_access_terms_*" to lookup the media by mime type).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it was a long and winding road, but we got there @wgilling . If you wanna toss up the PR for the pdf file on I'm going to go ahead and slice the version for this module for the release! Merci beaucoup encore 🙇♂️ |
GitHub Issue: Islandora/documentation#1171
Release pull requests, etc.)
What does this Pull Request do?
Consolidates the separate works from ASU and Jonathan Hunt to provide some islandora_tokens.
What's new?
The code from https://github.com/kayakr/islandora_tokens and https://github.com/asulibraries/islandora-repo/tree/develop/web/modules/custom/islandora_tokens were merged into the the controlled_access_terms module.
Any tokens that were created with a previous install should still be supported by the new code (after legacy module is uninstalled and this is installed, for example, a token for [islandoratokens:agent_contributor] would still output the same value afterwards).
How should this be tested?
After pulling in this branch and the cache is cleared, ANY place where you can use tokens should support these tokens, but we used it in the Metatag: Google Scholar in order to provide values for the various scholar meta tags. To use this, first install
metatag
andmetatag_google_scholar
and configure the "Repository item" content type admin/config/search/metatag/node__islandora_object to define several of the fields output to use some of the tokens. These should pop up when you click the "Browse available tokens." link -- and are all in the "Islandora Tokens" section.Interested parties
@Islandora/8-x-committers, @dannylamb, @kayakr