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

Fix PHP warnings when indexing in solr #114

Merged
merged 2 commits into from
Apr 24, 2024
Merged

Fix PHP warnings when indexing in solr #114

merged 2 commits into from
Apr 24, 2024

Conversation

joecorall
Copy link
Member

@joecorall joecorall commented Feb 16, 2024

GitHub Issue: (link)

  • Other Relevant Links (Google Groups discussion, related pull requests,
    Release pull requests, etc.)

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
Copy link
Member

rosiel commented Feb 20, 2024

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
Copy link
Member Author

Yeah it was a misconfigured edtf year processor.

@rosiel rosiel self-assigned this Mar 6, 2024
@rosiel
Copy link
Member

rosiel commented Mar 12, 2024

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 joecorall closed this Mar 12, 2024
@joecorall joecorall deleted the joecorall-patch-1 branch March 12, 2024 22:20
@joecorall joecorall restored the joecorall-patch-1 branch April 15, 2024 18:35
@joecorall joecorall reopened this Apr 15, 2024
@joecorall
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor

@aOelschlager aOelschlager left a comment

Choose a reason for hiding this comment

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

Based off of Josh's comment, I feel like it's been tested and works.

@aOelschlager aOelschlager merged commit d9573c1 into 2.x Apr 24, 2024
8 checks passed
@joecorall joecorall deleted the joecorall-patch-1 branch April 24, 2024 17:40
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.

4 participants