Uploaded image for project: 'Islandora'
  1. Islandora
  2. ISLANDORA-1885

islandora_invoke_hook_list can clash during refinements



    • Type: Documentation
    • Status: Open
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Labels:


      This is such a ridiculously edge-y edge case but here goes

      In short: https://github.com/Islandora/islandora/blob/0bb04444145c620bc2f841bb2dd441797852698f/includes/utilities.inc#L197 can cause problems based on rare module/datastream ID naming clashes.

      In long:

      The behaviour of array_merge_recursive() is, if it encounters duplicates, to convert each recursive item to an array.

      In an ideal world we wouldn't have duplicates, BUT, since we're tacking refinements onto the inside of our hook list, we could potentially have some based on module naming conventions; for example:

      1. Person A makes a module - let's say islandora_book_batch, to give a real-world example
      2. Person B makes their own module called, let's say, islandora_book_batch_document - independent of the functionality of islandora_book_batch, mind you; just a module that batches books from various kinds of documents (this isn't something I'm making but I can see how these two modules could potentially exist in the real world)
      3. Person B implements a hook that can be refined on DSIDs - let's say islandora_book_batch_document_islandora_datastream_alter(); this isn't a great example but I'm running with it
      4. Person C has an Islandora installation that has both islandora_book_batch and islandora_book_batch_document enabled
      5. Person C also implements a solution pack that creates DOCUMENT datastreams
      6. Person C ingests an object with a DOCUMENT datastream
      7. Person C alters the DOCUMENT datastream, and two copies of islandora_book_batch_document_islandora_datastream_alter are added to the hook list - one as the explicitly defined version, and one as the 'DOCUMENT' refinement of islandora_book_batch_islandora_datastream_alter - a non-existent function that nevertheless is picked up by module_invoke_all
      8. The fake refinement hook is recursively merged with the hook list containing the previous copy, converting its keys to indices
      9. We now have a hook in the hook list that contains indexed keys where once there were associative keys
      10. Hang on - the datastream alter hook implementation is expecting specific keys ... which were actually correctly implemented by Person B ... but have now been recurse-merged into oblivion simply due to a specific installation profile by Person C ...
      11. Oh god

      I've listed this as 'Task' because I'm really not sure what to do with this. It seems obvious in retrospect but tripped me up for a while. And while this can be documented, I have no idea where to document it - and it doesn't help the theoretical Person C, nor does it seem like Person A or B's fault in particular - it's just a quirk of how we build hooks, so it's kind of unclear which person should fix their module (the person who made the latter module? Should everyone just be cognizant at all times of all theoretical DSID/module name clashes?).

      If it's acceptable to fix this in code, we could either change array_merge_recursive() to array_replace_recursive() to squash potential duplicates, or use numeric keys when we encounter duplicates (dangerous? Do we use the keys for anything post-hook-invocation?), or use some other strategy, I dunno.




            • Assignee:
              daitken Daniel Aitken
              daitken Daniel Aitken
            • Votes:
              0 Vote for this issue
              2 Start watching this issue


              • Created: