search

bondjimbond / islandora_metadata_extras

Islandora utility module that provides some options for customizing metadata display and generation.
GNU General Public License v3.0
6 stars 2 forks source link

Fix offset error #34

Closed bondjimbond closed 4 years ago

bondjimbond commented 4 years ago

Error noted in #32:

Noted an issue here. Every time there's a search, this appears in the logs:

Notice: Undefined offset: 1 in islandora_metadata_extras_facet_convert() (line 68 of /var/www/html/drupal7/sites/all/modules/islandora_metadata_extras/includes/utilities.inc).

This happens only in my production site; not in Vagrant.

Is it possible there might be an out of date module creating this conflict? Do you get similar results?

What does this Pull Request do?

Currently, if user forgets to include the | character in the list of replacements, result is an Undefined Offset error which is written to the logs. Given how the module works, this means hundreds of Undefined Offset errors every hour on a busy site.

This PR checks each replacement string to make sure the | is actually present, before it tries to explode it.

What's new?

In the two Replacement fields, code is introduced to check for the presence of the | before attempting to explode and replace. If the | is not present in that particular string, the replacement process is skipped.

How should this be tested?

Configure your text replacements as normal, in a way that should generate results. Run a search and confirm that replacements are happening.

In one of your replacements, remove the | and re-run your search. Check the Drupal log and see the Undefined Offset error.

Check out this PR and re-run the search. No error reported.

Question: should we introduce some lines to report to the administrator what the problem is? Is this a watchdog error that runs during the replacement process, or is it something we can catch when saving your config via the admin form?

Interested parties

@mjordan

mjordan commented 4 years ago

@bondjimbond Your fix works as you describe, with nothing left in the log. WRT your question:

Question: should we introduce some lines to report to the administrator what the problem is? Is this a watchdog error that runs during the replacement process, or is it something we can catch when saving your config via the admin form?

I would prefer to validate the values entered in the form. I think all we'd need to do is make sure each one contains a |.

bondjimbond commented 4 years ago

Agreed. I have added form validation, which I've tested and is working. I left the original fix in, as it deals with any legacy values that might already exist. Does this satisfy?

mjordan commented 4 years ago

Yup, tested and it works. Merging.

bondjimbond commented 4 years ago

Huzzah