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

Remove typehint to allow false, if parent object not ingested yet #719

Closed
wants to merge 2 commits into from
Closed

Remove typehint to allow false, if parent object not ingested yet #719

wants to merge 2 commits into from

Conversation

IAMlKeno
Copy link

ISLANDORA-2418: (https://jira.duraspace.org/browse/ISLANDORA-2418)

What does this Pull Request do?

This pull request will prevent the following PHP error caused when access to an object not yet created is requested.
Error produced:
Argument 2 passed to islandora_object_manage_access_callback() must be an instance of AbstractObject, boolean given... defined in islandora_object_manage_access_callback() (line 930 of /var/www/drupal7/sites/all/modules/islandora/islandora.module).

What's new?

  • The PR removes the typehint restricting the type that can be passing to the function islandora_object_manage_access_callback as FALSE can also be passed if the object does not exist.

How should this be tested?

--- Before merging code ---

  • Create a batch ingest that creates at least one child compound object with a parent that does not exist.
    ** A module like the Islandora Spreadsheet Ingest module can be used.
  • Once ingested, navigate to the object and the PHP error, "Argument 2 passed to islandora_object_manage_access_callback() must be an instance of AbstractObject..." should be produced.

--- After merging the code from this PR ---

  • The error will not be seen and the object should load with no issues.

Interested parties

@Islandora/7-x-1-x-committers
@DiegoPino
@jonathangreen
@jordandukart

@DiegoPino
Copy link

@IAMlKeno i don't think removing the signature requirement is the solution. They do exist for a reason. Issue is the compound module that tries to invoke something on a non existing object. It would be wiser, IMHO to avoid the call at all if the object does not exist. That makes more sense that opening the function to anything and then allowing the function to actually file in other non expected situations.

@jonathangreen
Copy link

I'm okay with this fix. #714 probably added some type hints that weren't totally accurate or necessary, walking them back is probably a good idea when we find them. Does that sound reasonable to you @DiegoPino?

@IAMlKeno this does need to fix some coding standards before we can consider merging it though. See travis log here: https://travis-ci.org/Islandora/islandora/jobs/521807965#L874

@jonathangreen
Copy link

This is still failing, but it looks like its not specifically due to this PR.

I made a ticket for the the coding standards changes here:
https://jira.duraspace.org/browse/ISLANDORA-2420

@Islandora/7-x-1-x-committers any thoughts on how we want to handle this? Maybe we just update things when we notice they are failing?

@DiegoPino
Copy link

@jonathangreen i agree, update when we see things fail. About this particular pull, i'm still not convinced. Reason is that we are totally dependent on implementation flavors here when removing the type hint. I guess issue here is really islandora_object_load that returns either NULL, FALSE an error or the actual Object...which means assuming we can pass the return of this function directly to the function being touched here, makes little sense without an if() on the return first. (and funny, same functions says..:

// Assuming access denied in all other cases for now.
  return NULL;

If course rewriting islandora_object_load is totally breaking stuff, so yeah, lets avoid that.

Also, why are we calling if (!$object && !islandora_describe_repository())
I mean, do we need another check to see if the repository is there?

Anyway, i will submit myself to the greater good, i still think the issue here is also the code in compound that just calls this without checking anything. Thanks!

@IAMlKeno
Copy link
Author

IAMlKeno commented Apr 23, 2019

I believe, I understand the hesitation @DiegoPino. I have submitted a pull request to the compound solution pack here Islandora/islandora_solution_pack_compound#157 (https://jira.duraspace.org/browse/ISLANDORA-2418)

From what I can see, compound seems to be the only core islandora module, that can create this use case. Whereas, devs creating custom code that call this function would need to be cautious about parameters being passed.

@DiegoPino
Copy link

@IAMlKeno do we still need this one? Now that Islandora/islandora_solution_pack_compound@4100bed got merged?

@IAMlKeno
Copy link
Author

IAMlKeno commented May 8, 2019

consider it closed

@IAMlKeno IAMlKeno closed this May 8, 2019
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.

3 participants