search

Islandora / controlled_access_terms

Drupal module for subject and agents
GNU General Public License v2.0
7 stars 28 forks source link

Fix PHP warnings when indexing in solr #114

Closed joecorall closed 2 months ago

joecorall commented 4 months ago

GitHub Issue: (link)

What does this Pull Request do?

Continues iterating over the loop if an EDTF value isn't defined

Was getting these warnings in my watchdog when indexing nodes in solr:

Warning: Undefined array key 2 in Drupal\controlled_access_terms\Plugin\search_api\processor\EDTFYear->addFieldValues() (line 142 of /var/www/drupal/web/modules/contrib/controlled_access_terms/src/Plugin/search_api/processor/EDTFYear.php)
#0 /var/www/drupal/web/core/includes/bootstrap.inc(164): _drupal_error_handler_real()
#1 /var/www/drupal/web/modules/contrib/controlled_access_terms/src/Plugin/search_api/processor/EDTFYear.php(142): _drupal_error_handler()
Warning: Undefined variable $edtf in Drupal\controlled_access_terms\Plugin\search_api\processor\EDTFYear->addFieldValues() (line 154 of /var/www/drupal/web/modules/contrib/controlled_access_terms/src/Plugin/search_api/processor/EDTFYear.php)
#0 /var/www/drupal/web/core/includes/bootstrap.inc(164): _drupal_error_handler_real()
#1 /var/www/drupal/web/modules/contrib/controlled_access_terms/src/Plugin/search_api/processor/EDTFYear.php(154): _drupal_error_handler()

What's new?

Just declaring some variables that cause warning if the assumptions in the code aren't met.

How should this be tested?

I tested by just making the code changes and seeing the watchdog messages go away.

Additional Notes:

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

Interested parties

@Islandora/committers

rosiel commented 4 months ago

I'm curious what case would cause $field to not be made up of an entity type, bundle, and field name? This seems to cause them to not be indexed. Is that just a misconfiguration in the EDTF Year processor?

joecorall commented 4 months ago

Yeah it was a misconfigured edtf year processor.

rosiel commented 3 months ago

If it only happens when misconfigured (bad config load? manual shenanigans?) then shouldn't we leave that error there, so that the site admin can see something is wrong and fix it? Failing silently (and failing to index the content) seems like it'll make it hard to find out about and correct your site.

joecorall commented 2 months ago

This is still an issue if a node bundle is being indexed that does not have an EDTF field set in the processor settings. See https://islandora.slack.com/archives/CM5PPAV28/p1713205364774709?thread_ts=1712939510.602939&cid=CM5PPAV28 for an example.

joshdentremont commented 2 months ago

In my case this happened because I have multiple content types that each use field_edtf_date_issued. The code as it is written now will loop through all of them, but the $bundle variable only matches on one, so the other ones all throw the $edtf doesn't exist error.

You should be able to reproduce the errors in a fresh install by adding a new content type that uses field_edtf_date_issued and then trying to index some objects in Solr.

joshdentremont commented 2 months ago

Replacing my EDTFYear.php with the one in this PR allowed me to reindex solr without those errors, so I would say this works as advertised.