backdrop-issues icon indicating copy to clipboard operation
backdrop-issues copied to clipboard

Fix typos in hook_ckeditor_PLUGIN_plugin_check documentation

Open bennybobw opened this issue 11 months ago • 6 comments

Typos and errors in hook_ckeditor_PLUGIN_plugin_check documentation

in site/web/core/modules/ckeditor/ckeditor.api.php

function hook_ckeditor_PLUGIN_plugin_check($format, $plugin_name) {
  $toolbars = $format->editor_settings['toolbar'];
  foreach ($toolbars as $toolbar['items'] as $row) {

    if (in_array('Underline', $row)) {
      return TRUE;
    }
  }
}

The foreach loop is completely wrong and also there are multiple $toolbars available. Additionally, using $row is confusing. $toolbar[$x] is the row but then there are button groups defined inside those, so I'd suggest renaming it to $group

Pull request incoming.

bennybobw avatar Jan 10 '25 19:01 bennybobw

Actually, the code in core/modules/ckeditor/ckeditor.api.php is the following one.

function hook_ckeditor_PLUGIN_plugin_check($format, $plugin_name) {
  // Automatically enable this plugin if the Underline button is enabled.
  foreach ($format->editor_settings['toolbar']['buttons'] as $row) {
    if (in_array('Underline', $row)) {
      return TRUE;
    }
  }
}

It does not work because in_array() checks the array values, not the array keys. in_array('Linux', $values) works when $values is array('Linux', 'macOS', 'Windows'), not array('Linux' => 1, 'macOS' => 2, 'Windows' => 3).

The code used in the PR would not work for the same reason.

function hook_ckeditor_PLUGIN_plugin_check($format, $plugin_name) {
  $toolbars = $format->editor_settings['toolbar'];
  foreach ($toolbars as $toolbar) {
    foreach ($toolbar as $group) {
      if (in_array('Underline', $group['items'])) {
        return TRUE;
      }
    }
  }
}

avpaderno avatar Jan 10 '25 23:01 avpaderno

Also, core/modules/ckeditor5/ckeditor5.api.php contains the same code, for a similar hook (hook_ckeditor5_PLUGIN_plugin_check()).

The files that need to be changed are two.

avpaderno avatar Jan 10 '25 23:01 avpaderno

The following code would work.

function hook_ckeditor_PLUGIN_plugin_check($format, $plugin_name) {
  // Automatically enable this plugin if the Underline button is enabled.
  foreach ($format->editor_settings['toolbar']['buttons'] as $row) {
    if (in_array('Underline', array_keys($row))) {
      return TRUE;
    }
  }
}

That is how I would write the code, if I wanted to make the minimum change to fix it.

(I assume $format->editor_settings['toolbar']['buttons'] is correct to access the toolbar buttons.)

avpaderno avatar Jan 10 '25 23:01 avpaderno

Basing on the code used by ckeditor_get_settings(), I guess the correct code should be the following one.

function hook_ckeditor_PLUGIN_plugin_check($format, $plugin_name) {
  // Automatically enable this plugin if the Underline button is enabled.
  foreach (foreach ($format->editor_settings['toolbar'] as $row) {
    foreach ($row as $button_group) {
      if (in_array('Underline', array_keys($button_group['items']))) {
        return TRUE;
      }
    }
  }
}

avpaderno avatar Jan 11 '25 16:01 avpaderno

@bennybobw Could you take a look at incorporating @avpaderno's feedback?

quicksketch avatar Feb 20 '25 17:02 quicksketch

Basing on the code used by ckeditor_get_settings(), I guess the correct code should be the following one.

function hook_ckeditor_PLUGIN_plugin_check($format, $plugin_name) { // Automatically enable this plugin if the Underline button is enabled. foreach (foreach ($format->editor_settings['toolbar'] as $row) { foreach ($row as $button_group) { if (in_array('Underline', array_keys($button_group['items']))) { return TRUE; } } } }

Hm. I'm a little confused by this. When I dd() group in the following code, I get numerically keyed arrays, so I don't think the array_keys code would work. Here's my test code:

function mymodule_ckeditor_test_plugin_check($format, $plugin_name) {
 $toolbars = $format->editor_settings['toolbar'];
  foreach ($toolbars as $toolbar) {
    foreach ($toolbar as $group) {
      dd($group['items']);
      if (in_array('Underline', $group['items'])) {
        return TRUE;
      }
    }
  }
}
// Output is arrays like:
Array
(
    [0] => Bold
    [1] => Italic
    [2] => Blockquote
    [3] => Styles
)

Am I missing something about how this stuff works? I'm on Backdrop 1.28.2 so I guess it's possible it's changed in a couple of versions....

bennybobw avatar Feb 20 '25 19:02 bennybobw