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 tokens 1171 #66

Merged
merged 14 commits into from
Jul 22, 2021

Conversation

wgilling
Copy link
Contributor

GitHub Issue: Islandora/documentation#1171

  • Other Relevant Links (Google Groups discussion, related pull requests,
    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 and metatag_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

@elizoller
Copy link
Member

this looks like it would supercede #30

@dannylamb
Copy link
Contributor

Beautiful. This will be useful to soooooooo many people.

* @return mixed
* \Drupal\file\FileInterface on success, NULL otherwise.
*/
function controlled_access_terms_image_from_media($media) {
Copy link
Contributor

@seth-shaw-unlv seth-shaw-unlv Apr 29, 2021

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).

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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)?

@wgilling
Copy link
Contributor Author

wgilling commented May 3, 2021

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 and move it into islandora.

@dannylamb
Copy link
Contributor

🙇


case 'media-thumbnail-image:url':
case 'media_thumbnail_image:url':
$term = $islandoraUtils->getTermForUri('http://pcdm.org/use#ThumbnailImage');
Copy link
Contributor

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');

$fid[0]['target_id'] : NULL;
if (!is_null($fid_value)) {
$file = File::load($fid_value);
$url = $file->getFileUri();
Copy link
Contributor

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.

@elizoller
Copy link
Member

@wgilling
Copy link
Contributor Author

wgilling commented Jul 6, 2021

this should now have all of the phpcs updates and be ready to merge

'name' => t("PDF Url"),
'description' => t('URL to related media file if "Original file" is a PDF file'),
];
$node['media-thumbnail-image:url'] = [
Copy link
Contributor

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.

'name' => t("Publication date"),
'description' => t('Show the "Date Created" into YYYY/MM/DD format (handles EDTF format)'),
];
$node['pdf_url'] = [
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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).

@wgilling
Copy link
Contributor Author

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.
To test the functionality as it will be once the metadata patch is included in the playbook (separate PR that @elizoller is making to the playbook, but it looks like the lines pasted below), this patch will need to be applied. This can be done by editing the /var/www/html/drupal/composer.json file to add a section for this patch:

+            },
+            "drupal/metatag": {
+                "fix for delimiters": "https://git.drupalcode.org/project/metatag/-/merge_requests/20.diff"
            }

... and then run composer update drupal/metadata

Be sure that the previous islandora_tokens module is not installed -- and that the current islandora branch is up to date.

@kayakr
Copy link
Contributor

kayakr commented Jul 20, 2021

@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.

Copy link
Contributor Author

@wgilling wgilling left a 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.

* @return mixed
* \Drupal\file\FileInterface on success, NULL otherwise.
*/
function controlled_access_terms_image_from_media($media) {
Copy link
Contributor Author

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.

* @return mixed
* \Drupal\file\FileInterface on success, NULL otherwise.
*/
function controlled_access_terms_image_from_media($media) {
Copy link
Contributor Author

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)?

'name' => t("Publication date"),
'description' => t('Show the "Date Created" into YYYY/MM/DD format (handles EDTF format)'),
];
$node['pdf_url'] = [
Copy link
Contributor Author

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).

Copy link
Contributor

@dannylamb dannylamb left a comment

Choose a reason for hiding this comment

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

👍

I've tested all these tokens with metatag_google_scholar and they're all working, including using multiple values 🚀

image

@dannylamb dannylamb merged commit 79b4694 into Islandora:8.x-1.x Jul 22, 2021
@dannylamb
Copy link
Contributor

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 islandora I can test and merge real quick.

I'm going to go ahead and slice the version for this module for the release!

Merci beaucoup encore 🙇‍♂️

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