PHP Fatal error: Cannot redeclare form_options_from_text

[findlabnet]

Describe the bug:*

1. Attempt to upgrade core from 1.11.3 (contrib "Options element" v 1.x-1.0 is enabled for using with "Webform" module) to 1.12.0:
   PHP Fatal error: Cannot redeclare form_options_from_text() (previously declared in /DOC_ROOT/core/modules/field/modules/options/options.module:567) in /DOC_ROOT/modules/contrib/options_element/options_element.module on line 173
2. Attempt to uninstall previously disabled contrib module fire the same fatal error again.
   Workaround:
   Disable contrib "Options element" - upgrade going as expected, but module cannot be uninstalled, disabled only.
   Expected behavior:
   Check if this contrib exists, then disable/uninstall it before going to core upgrade.
   Additional information:

• Operating System and its version: Debian GNU/Linux 9.6 as is
• Web server and its version: Apache/PHP/MariaDB from Debian distribution
• Browser(s) and their versions: any

---

PR by @klonos: backdrop/backdrop#2564

[jerry-hudgins]

Yep, seeing this here as well. Upgrade from 1.11.3 to 1.11.4 is OK.

[docwilmot]

Should you uninstall options element before upgrade?

[quicksketch]

Thanks @findlabnet, we should have realized that would happen. A temporary work-around you can do to fix the problem:

• Execute this MySQL query: DELETE FROM system WHERE name = 'options_element';
• Delete options_element module directory from your file system.

The question is: can we do this same thing in options.install in Backdrop core without it causing a fatal error? I'm not sure if options.module is going to be loaded at the time update.php is run. If it is, we may need to move any conflicting function names into a new .inc file that is conditionally loaded.

[quicksketch]

Looks like we can't simply add an update hook, because the entire options.module is loaded when visiting update.php (or any other page for that matter).

[herbdool]

Yeah, missed this. I was also working on an update to Webform so it could work with core options_element.

[quicksketch]

@laryn helpfully made a new release of Options Element that adds core version checking: https://github.com/backdrop-contrib/options_element/releases/tag/1.x-1.0.1

Though I don't think it can disable itself after updating, at least it will keep anyone from turning it on in 1.12 and higher.

[herbdool]

@quicksketch, @serundeputy had mentioned we could put a warning message if Options Element is present after 1.12.x. We can at as well perhaps.

[herbdool]

Perhaps we can also rename the conflicting functions. I don't think they're used for anything else.

[herbdool]

I guess my last idea won't help since theme_options can't be renamed and is present in both.

@findlabnet does another module require options_element on your site? The only one I can think of is webform_civicrm. I've made an update for webform_civicrm so it doesn't require it anymore.

[findlabnet]

@herbdool, I'm fine with current core implementation. Haven't try CiviCRM, so can't add anything.

[herbdool]

@findlabnet I didn't word it well: I'm wondering why you're unable to fully uninstall options_element. Maybe because there's a dependency, but not clear what module.

[findlabnet]

@herbdool this was because same named functions have been exists twice: within core directory (just replaced by me from 1.11.3 to 1.12.0 ) and in module "Options element" - no caused dependency, moreover module haven't settings file.
After removing module from DB and FS - no more problem.

[jenlampton]

no PR here yet, bumping milestone.

[jenlampton]

Can we add an if(!function_exists('form_options_from_text')) check around the definition of the function?

[klonos]

A bit of a more generic question... "back in the days", we used to use backdrop_merged_modules() when merging modules into core. Should we be doing that here as well?

Returns an array of modules that have been merged into Backdrop core.

Modules that have been merged into core may not be enabled on newer installations of Backdrop. Note that some modules have been merged into core but kept the same name as they did previously. For example "redirect" module existed both as a contrib module and then later as a core module. Because the core module shares the same name, it's not included in this list. Including it would prevent the core module from being enabled.

[herbdool]

I didn't know about that function. I agree it should be added to the list.

[klonos]

...PR updated.

Unfortunately, because of all the reasons I mentioned in my previous comment, we need to do a direct db query to delete the module entry from the database, instead of using "native" functions.

I have tested running db update when upgrading from 1.11.x to 1.12.x and confirmed that is removes the contrib module options_element, without any php errors.

[herbdool]

I think your PR makes an incorrect assumption. I'll need to look at it later. You can just run DELETE FROM system WHERE name = 'options_element'; without a check.

we could execute a db_query() to disable the Options Element module, and then it would be disabled from being re-enabled by the modules UI due to being in this list.

Though maybe Nate means to change status to zero rather than delete entry?

[klonos]

You can just run DELETE FROM system WHERE name = 'options_element'; without a check.

True. Although we will not be able to notify the user about this change then (the message returned by the update hook). So basically, if we blindly delete without checking if the module exists or not, it will work, but we'd need to decide whether we should a) be providing a message after db update regardless of whether the module existed in the site before or not; or b) to assume that the site admin/owner follows the Backdrop changelog (and not provide any message at all after db update).

Though maybe Nate means to change status to zero rather than delete entry?

Tried that. Does not remove the module from /admin/modules/uninstall unless we also set schema_version to -1. Delete does both things in a single go.

[herbdool]

I tweaked your PR @klonos. I think tests will pass now.

[quicksketch]

The PR looks great. And as options element does not provide any config or settings, there doesn't seem to be anything else left to clean up or migrate.

[klonos]

I have updated the PR with the following:

• Removed the duplicate message returned by the update hook function. No need for it since backdrop_set_message() works. Noting that I initially copied the implementation from another hook - not sure which one though; just mentioning this because I like to have as much consistency as possible.

• Changed the direct db_query()s to db_select()/db_delete() instead. As a novice developer, I don't speak MySQL, so these make more sense to me.

[herbdool]

@klonos the code is full of db_query so it's a good thing to learn. For example, in system.install: db_query("DELETE FROM {system} WHERE name = 'token' AND type = 'module'"); From what I understand the idea was to change all Backdrop queries back to that older format in 2.x because people figured it was easier to understand. That's probably on hold now.

I can't tell if your query works or not. I did find this example which doesn't include a null in the fields:

db_update('system') ->fields(array('status' => 1)) ->condition('type', 'theme') ->condition('name', 'stark') ->execute();

[herbdool]

@klonos I was drawing from system_update_1046 which outputs two messages.

[klonos]

I can't tell if your query works or not.

It works 😄 ...I have tested it on my local.

I did find this example which doesn't include a null in the fields

If I use ->fields(array('status' => 1)) (or rather various variations of it, like ->fields(array('name'))), I get PDO exceptions.

I could use aliases, like db_select('system', 's') instead of just db_select('system'). This would allow doing ->fields('s', array('name')), but again, as a novice, I'd get confused by that 's'.

If used without a specific alias, db_select('system') produces a 'system' alias by default. So I could do ->fields('system', array('name')). That would produce this:

SELECT system.name AS name FROM system system WHERE ...

...whereas ->fields(NULL, array('name')) produces this:

SELECT name AS name FROM system system WHERE ...

Both have the same result, so does not really matter. In other words, I can use any of the below, and the end result after ->fetchField(); would be options_element (that is, if the module exists):

db_select('system', 's')->fields('s', array('name'))
db_select('system')->fields('system', array('name'))
db_select('system')->fields(NULL, array('name'))

I have updated the PR to use the second variation, since it seems to be the least confusing of the 3.

I was drawing from system_update_1046 which outputs two messages.

Yeah, I know. I have seen various implementation of this, and it might make sense if two different messages need to be shown. The simple return t(); renders the message styled as a "log entry", which I find less user-friendly. I have changed the PR to show such a message if the Options Element module is not installed...

If Options Element was found:

If not found:

PS: I have also added a comment with an explanation as to why we are doing the db_delete() in the hope that it will be useful in other similar cases in the future.

[klonos]

Based on the recent discussions in #3655 and #61, I have reverted the update hook back to using db_query().

[herbdool]

@klonos ideally we should put it back to needs review if code changes because who knows, there could be little errors introduced. I checked and it looks ok.

[klonos]

...sorry, indeed we should. Noted for next time.

And thanks 👍

[quicksketch]

Merged backdrop/backdrop#2564 into 1.x and 1.12.x. Great job! Thanks @herbdool and @klonos for the fix. Thanks @findlabnet and @jerry-hudgins for reporting this issue!