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

Add hook for sharing and migration function #214

Merged
merged 6 commits into from May 3, 2019

Conversation

bondjimbond
Copy link
Contributor

JIRA Ticket: (https://jira.duraspace.org/browse/ISLANDORA-2410)

What does this Pull Request do?

Creates a new hook, hook_islandora_basic_collection_share_migrate, which provides the object being migrated and its new parent to whatever module is calling it.

What's new?

New hook exists. It can be called.

How should this be tested?

Enable devel

Call the hook in some other module. For example, in your module "islandora_my_module", add the function:

function islandora_my_module_islandora_basic_collection_share_migrate($object, $newparent) {
  dd($object);
  dd($newparent);
}

Migrate an object to another collection.

Checking /tmp/drupal_debug.txt should show the two parameters have been passed to your function.

Interested parties

@Islandora/7-x-1-x-committers

includes/utilities.inc Outdated Show resolved Hide resolved
@bondjimbond
Copy link
Contributor Author

@whikloj Thanks for the feedback! I think I've fixed it all; we should be good to go now.

Before this last commit, the new hook would fire twice every time you viewed a collection where the hook could be called. I printed the value of $new_member-id and got "fake:pid" both times... a strange feature in the module that I don't really understand, but apparently fake:pid is passed around in various places.

Excluding fake:pid before triggering the hook solves the problem.

includes/utilities.inc Outdated Show resolved Hide resolved
@bondjimbond
Copy link
Contributor Author

Looks like I mistook the file where the constant was defined. Now it's fixed, and all is well... It works just fine.

Any other issues for your review, @whikloj?

@bondjimbond
Copy link
Contributor Author

Note that the Travis failure is for only one job - php 5.3.3 - which is the build itself failing and not any syntax errors. It looks like this is happening across all or most of our modules.

@bondjimbond
Copy link
Contributor Author

@whikloj Following up again on this -- anything else before it's approvable? The Travis failure (only happening on the very last php 5.3.3 build) looks like a problem with Travis.yml rather than my code:

islandora_basic_collection: Did not install any objects. Could not [error]
connect to the repository. Please check the settings on the Islandora
configuration page and install the required objects manually on the
Solution Pack admin page.

@whikloj
Copy link
Member

whikloj commented Apr 23, 2019

I'll try to get to actually testing this this week.

@bondjimbond
Copy link
Contributor Author

Forgot to mention in testing instructions that devel should be enabled if taking the dd() approach (but I'm guessing you know that @whikloj).

@bondjimbond
Copy link
Contributor Author

Just a bump - still needs testing. Would much appreciate some eyes on it!

@mjordan
Copy link
Contributor

mjordan commented Apr 30, 2019

@bondjimbond I can test this today.

Copy link
Contributor

@mjordan mjordan left a comment

Choose a reason for hiding this comment

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

Works as advertised.

@bondjimbond
Copy link
Contributor Author

Thanks @mjordan! Once this is merged, islandora_westvault will be just about ready for a serious test.

@mjordan
Copy link
Contributor

mjordan commented Apr 30, 2019

Travis is failing on the PHP 5.3.3 / Fedora 3.8.1 combination with:

[Composer\Exception \NoSslException The openssl extension is required for SSL/TLS protection but is not available. If you can not enable the openssl extension, you can disable this error, at your own risk, by setting the 'disable-tls' option to true.

Anyone know how we work around this?

@whikloj
Copy link
Member

whikloj commented Apr 30, 2019

I'm sorry @bondjimbond I got swamped, thank you @mjordan

@bondjimbond
Copy link
Contributor Author

@mjordan This has been happening for a lot of modules lately. People have been discussing whether to simply drop that test, since 5.3.3 is EOL anyway. I believe we're merging PRs that fail that check now; anyone able to confirm/deny that?

@whikloj
Copy link
Member

whikloj commented Apr 30, 2019

#216

@mjordan
Copy link
Contributor

mjordan commented Apr 30, 2019

Thanks @whikloj I'll add this to my recent Islandora Bagit PRs as well.

@bondjimbond
Copy link
Contributor Author

@mjordan So are we good to merge this then? Since #216 isn't merged yet, I'd rather get this in first than wait for it and rebase.

@mjordan
Copy link
Contributor

mjordan commented May 1, 2019 via email

@bondjimbond
Copy link
Contributor Author

@mjordan
Copy link
Contributor

mjordan commented May 2, 2019

@bondjimbond shouldn't #216 get merged before this one, so this one passes? I can merge #216 now if that's the case, then merge this one.

@bondjimbond
Copy link
Contributor Author

I don't think the order really matters, but it's up to you. If you merge #216, I'll pull it down and add the commit here to get the pass.

@mjordan
Copy link
Contributor

mjordan commented May 2, 2019

OK, I'll merge #216 now.

@mjordan
Copy link
Contributor

mjordan commented May 2, 2019

Correction, I just approved #216, so I'll merge in 24 hours.

@bondjimbond
Copy link
Contributor Author

Woohoo, finally passed!

@mjordan mjordan merged commit 200dd0d into Islandora:7.x May 3, 2019
@mjordan
Copy link
Contributor

mjordan commented May 3, 2019

@bondjimbond can you close the JIRA ticket?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants