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

Add field_formatter_settings module to backdrop_merged_modules() #3742

Closed
quicksketch opened this issue May 2, 2019 · 10 comments
Closed

Add field_formatter_settings module to backdrop_merged_modules() #3742

quicksketch opened this issue May 2, 2019 · 10 comments

Comments

@quicksketch
Copy link
Member

quicksketch commented May 2, 2019

Description of the bug
Follow-up to #1376, where Field Formatter Settings was merged into core.

We need to uninstall the contrib version of Field Formatter Settings when upgrading to Backdrop 1.13.0

Steps To Reproduce
To reproduce the behavior:

  1. Install Backdrop 1.12.6
  2. Install https://github.com/backdrop-contrib/field_formatter_settings via UI or manually.
  3. Upgrade to Backdrop 1.x latest (or Backdrop 1.13.0-preview), be sure to run update.php
  4. Manage fields on a content types.

Actual behavior

  • You may get function already defined error for field_formatter_settings_get_instance_display_settings().

Expected behavior

  • Field Formatter Settings module should be uninstalled during the update and should not be possible to re-enable in the UI.

We should also add @since to all the new functions added in https://github.com/backdrop/backdrop/pull/2493/files. Sometimes that difficult to do during development because we don't know when the feature will be merged, so let's use this opportunity to include @since 1.13.0 now that we know it's version.


PR: backdrop/backdrop#2644

@opi
Copy link

opi commented May 2, 2019

Should we copy/paste https://github.com/backdrop/backdrop/pull/2564/files (Issue #3485: Add options_element in backdrop_merged_modules()) ?

I left a comment on this merged PR: https://github.com/backdrop/backdrop/pull/2564/files#r280312750

@quicksketch
Copy link
Member Author

Yeah, just copying what we have before is probably what we want here. As with Options Element, this should probably go in field.install.

@opi opi changed the title Add field_formatter_settings() to backdrop_merged_modules() Add field_formatter_settings module to backdrop_merged_modules() May 3, 2019
@opi
Copy link

opi commented May 3, 2019

PR backdrop/backdrop#2644 roughly tested with following steps

cd path/to/backdrop
git checkout 1.12.6
drush dl field_formatter_settings && drush en -y field_formatter_settings
git checkout opi/3742-add-field_formatter_settings-to-backdrop_merged_modules
drush updb
drush pm-list | grep field_formatter_settings

The last drush command shows that field_formatter_settings contrib module is now disabled.

Note the this PR has a separated commit with documentation update, about adding @since info to new hooks and function.

@herbdool
Copy link

herbdool commented May 3, 2019

RTBC

@laryn
Copy link
Contributor

laryn commented May 3, 2019

FYI I've made a new release of the contrib version that has a dependency on system being less than 1.13. (dependencies[] = system (<1.13))

@opi
Copy link

opi commented May 3, 2019

Thank you both ❤️

@klonos
Copy link
Member

klonos commented May 3, 2019

...wondering if it's worth moving the code we are reusing into a backdrop_remove_merged_modules() function, and then calling that from the update hooks (we are going to be merging more modules as time goes by - and repetition of the very exact code hurts my eyes 😅).

@quicksketch
Copy link
Member Author

That's a great idea @klonos! We have a set of utility-like functions in includes/update.inc that includes things like update_variable_get() and update_module_enable(). Creating an update_module_uninstall() or even update_module_uninstall_merged_module() could get rid of redundancy.

@quicksketch
Copy link
Member Author

I don't want to hold this up, so we can consolidate those similar places in a follow-up issue.

I've merged backdrop/backdrop#2644 into 1.x for 1.13.0 (and the preview release). Thanks @opi, @herbdool, @laryn, and @klonos!

@klonos
Copy link
Member

klonos commented May 8, 2019

That's a great idea @klonos! ...I don't want to hold this up

#3760 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants