Islandora / islandora

Drupal modules for browsing and managing digital repositories.
http://islandora.ca/
GNU General Public License v2.0
152 stars 118 forks source link

Allow for value to be absent #1047

Closed adam-vessey closed 3 months ago

adam-vessey commented 3 months ago

GitHub Issue: (link)

https://github.com/Islandora/documentation/issues/2338

What does this Pull Request do?

A brief description of what the intended result of the PR will be and/or what problem it solves.

What's new?

A in-depth description of the changes made by this PR. Technical details and possible side effects.

How should this be tested?

A description of what steps someone could take to:

Documentation Status

Additional Notes:

Any additional information that you think would be helpful when reviewing this PR.

Interested parties

Tag (@ mention) interested parties or, if unsure, @Islandora/committers

xurizaemon commented 3 months ago

I do not think the change here is correct. \Drupal\islandora\IslandoraUtils::getTermForUri accepts a string per its phpdoc comment, so should not be passed NULL. It doesn't yet have a type declaration on the function argument but may in future.

Regardless, it doesn't make sense to pass NULL to an entityquery (which getTermForURI() will run) to obtain a NULL, that just moves the issue up the chain.

I propose this instead: https://github.com/Islandora/islandora/pull/1048

Not yet tested, but will be able to in a few hours.

adam-vessey commented 3 months ago

@xurizaemon: I'm not seeing where in my code it would be possible for NULL to be passed to IslandoraUtils::getTermForUri()? NULL would be false-y, so it would just memoize and return NULL?

-      $this->structuredTextTerm = $this->utils->getTermForUri($this->options['structured_text_term_uri']);
+      $uri = $this->options['structured_text_term_uri'] ?? NULL;
+      $this->structuredTextTerm = $uri ? $this->utils->getTermForUri($uri) : NULL;

If it's not in the ->options array, $uri would be NULL, if $uri is NULL (or otherwise false-y), it would assign NULL directly ->structuredTextTerm, which already nullable, as the return from ::getTermFormUri() is nullable.

xurizaemon commented 3 months ago

You're right, I'd misread that on my mobile. Apologies and carry on! I will test your fix in a few hours, since I can repro this.

xurizaemon commented 3 months ago

Tested locally, this works perfectly well and as @adam-vessey indicated above. Thanks!