backdrop-contrib / video_filter

A Backdrop CMS port of drupal.org/project/video_filter, a highly flexible and easily extendable filter module to embed any type of video in your site using a simple token.
GNU General Public License v2.0
2 stars 2 forks source link

Merge in tinymce_video_filter #20

Closed herbdool closed 5 months ago

herbdool commented 5 months ago

See https://github.com/backdrop-contrib/tinymce_video_filter/issues/3

herbdool commented 5 months ago

@indigoxela I've got a PR. I took the approach that core does by putting a message in the update hook. Doesn't seem like any config needs to change.

indigoxela commented 5 months ago

Just tested locally and... it breaks the editor (Tiny won't load anymore), if the tinymce_video_filter module's still installed when opening a node form.

The update hook... doesn't seem to make much sense, either. It doesn't do anything, weird that one get's prompted to run a noop update, just to see a message. But then still things break.

Some brainstorming:

Skip out of the hook, if the initial module still exists, like:

/**
 * Implements hook_tinymce_external_plugins().
 */
function video_filter_tinymce_external_plugins($format) {
  if (module_exists('tinymce_video_filter')) {
    return;
  }
  $module_url = base_path() . backdrop_get_path('module', 'video_filter');
  // Rest of the code...

^^ This prevents the editor from breaking on the duplicate plugin.

Instead of a noop update hook, rather display something on the status page. Messages in update hooks are easily overlooked.

There's no urgent need to turn the obsolete module off, if we catch that in video_filter_tinymce_external_plugins.

@herbdool what do you think?

herbdool commented 5 months ago

I've incorporated your suggestions. It all works for me now. Thanks @indigoxela

indigoxela commented 5 months ago

And already released. :wink: Time to update the other readme(s).