If the current profile has 'allow users to choose default' (state) and the user select disabled as the default on his profile page, it has no effect. Pages still load with editor enabled by default.

The problem is wysiwyg_editor_user() uses boolean true/false for the relevent select form element. However you can not use booleans in a form element(*) so if you look at the source of the user settings page, you'll notice that the forms API has set available values of the this dropdown to 1 or 0.

When a page is being loaded wysiwyg_editor_load() tries to set the editor's JS settings and uses "status = 0" instead of "status = false" which appears to cause the javascript to assume the editor should be disabled.

This patch is a quick and painless fix to the problem, but the best solution would probably be to not use true/false anywhere in a form element.

(*)I can't seem to find the documentation for this anywhere, perhaps someone else knows the relevant background info as to why booleans can't be used in a form. I think it has to do with not being to recognize whether (-1) should represent boolean false or the integer -1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Status: Needs review » Postponed (maintainer needs more info)

Well, 1 is TRUE and 0 is FALSE, and both JavaScript and PHP implement an automatic type conversion for int/bool. That would be different if we would check the values type-agnostic (===), but we do not do that.

However, I feel you have found a bug, but unfortunately, you did not include the high-level steps to replicate your bug report. And I also like your patch, so please provide some more information to get this properly fixed.

AltaVida’s picture

To replicate:

  1. In your current wysiwyg profile options, enable "Allow users to choose default" under "Basic setup"
  2. In your user profile under "Wysiwyg Editor settings" select disabled for "Default state"
  3. Proceed to edit a node... the Wysiwyg editor is enabled when the page loads (it shouldn't be)

Currently the value status = 0 gets passed to the editor script via this code in wysiwyg_editor_load():

drupal_add_js(array('wysiwygEditor' => array(
      'configs' => array(),
      'showToggle' => $profile->settings['show_toggle'],
      'disable' => $disable,
      'enable' => $enable,
      'noWysiwyg' => $no_wysiwyg,
      'status' => $status,
      // If JS compression is enabled, at least TinyMCE is unable to determine
      // its own base path and exec mode since it can't find the script name.
      'editorBasePath' => base_path() . $path_editor,
      'execMode' => $exec_mode,
    )), 'setting');

However, passing a value of 0 for 'status' does not disable the editor, but passing boolean false does. Which is why the patch changes this:

      'status' => $status,

To this:

      'status' => $status ? true : false,

I know that PHP performs automatic type conversion, but not sure about Javascript (total js rookie here, sorry). All I know is that passing 0 to the js doesn't work and passing boolean false does work. Looking at the generated page I see the following for when status = 0:

<script type="text/javascript">jQuery.extend(Drupal.settings, {[..snip..], "status": "0", [..snip..]} });</script>

And for status = false I see:

<script type="text/javascript">jQuery.extend(Drupal.settings, {[..snip..], "status": false, [..snip..]} });</script>

So it doesn't appear that whatever Js script that reads the status var is taking "0" to mean false. (Perhaps because the quotes make it a string not an int? Like I said I'm a js noob).

What do you think?

sun’s picture

Title: A user's setting for 'default state' has no effect. » JavaScript interprets "0" as FALSE
Status: Postponed (maintainer needs more info) » Needs work

status var is taking "0" to mean false

That's possible and might screw up a lot more configuration values. Does a simple type-conversion to integer fix this issue? I.e. 'status' => (int)$status? If it does, we need the same fix for all configuration values in wysiwyg_editor_load_config().

AltaVida’s picture

You're right, and there are a whole other bunch of config options in wysiwyg_editor_config() that need to be fixed as well.

The problem is that when the settings page for a profile is saved, the boolean settings are saved as strings "1" or "0", and then the following logic from wysiwyg_editor_config() doesn't work correctly:

  $init['verify_html'] = $settings['verify_html'] ? $settings['verify_html'] : TRUE;
  $init['preformatted'] = $settings['preformatted'] ? $settings['preformatted'] : FALSE;
  $init['convert_fonts_to_spans'] = $settings['convert_fonts_to_spans'] ? $settings['convert_fonts_to_spans'] : TRUE;
  $init['remove_linebreaks'] = $settings['remove_linebreaks'] ? $settings['remove_linebreaks'] : TRUE;
  $init['apply_source_formatting'] = $settings['apply_source_formatting'] ? $settings['apply_source_formatting'] : FALSE;

Because the string "0" evaluates to TRUE.

I'd recommend changing these all to:

$init['something'] = isset($settings['something']) ? (int) $settings['something'] : TRUE;

Or perhaps the default values aren't needed at all? Then we could just use:

$init['something'] = (int) $settings['something'];

That would be simplest. What do you think?

I will test the latter and get back to you.

AltaVida’s picture

Title: JavaScript interprets "0" as FALSE » JavaScript interprets the string "0" as true
Status: Needs work » Needs review
FileSize
2.74 KB

Here is a patch. Everything is converted to (int). It works fine.

The "Safari warning" option appears to have been removed from TinyMCE, at least according to:

http://drupal.org/node/125267

I'll post another issue for that.

Cheers,
Mark

sun’s picture

Assigned: AltaVida » Unassigned
Status: Needs review » Needs work
FileSize
1.37 KB

Attached patch does a type conversion for all booleans to integers.

I'm struggling a bit with the direction of this issue/patch. According to http://api.drupal.org/api/function/drupal_to_js/5, boolean values should be properly converted to JavaScript booleans.

However, after thoroughly reading the whole issue again, the real cause seems to be that we are not passing boolean values to JavaScript, although they are stored as booleans...

...which is a wrong assumption after having a look into the serialized profile configuration in the database:

a:25:{...s:7:"default";s:1:"1";s:11:"user_choose";s:1:"0";s:11:"show_toggle";s:1:"1";...}

This means that profile configuration form values (hint: '#options' => array(FALSE => t('Disabled'), TRUE => t('Enabled'))) are neither submitted as booleans, nor as integers - they are converted to strings instead, which is finally the real cause for this issue.

sun’s picture

Assigned: Unassigned » sun
sun’s picture

Status: Needs work » Needs review
FileSize
6.5 KB
6.52 KB

So, let's fix the cause - with the wonderful side benefit of increasing usability by converting all affected profile configuration select fields into checkboxes finally! :)

AltaVida’s picture

the real cause seems to be that we are not passing boolean values to JavaScript, although they are stored as booleans.....which is a wrong assumption after having a look into the serialized profile configuration in the database

Yes I know. See #4:

The problem is that when the settings page for a profile is saved, the boolean settings are saved as strings "1" or "0"...

Anyhow, good idea on converting the form elements to checkboxes.

I got a bit confused on the patches you've provided. I applied "wysiwyg.bool_.patch" then also had to manually apply (because I'm on D6) the patch from #6 to get "status" working correctly. With just "wysiwyg.bool_.patch" applied it doesn't work correctly. I think if these 2 patches are combined we should have a winner.

Cheers.

sun’s picture

Status: Needs review » Needs work

Hm. I thought that type conversion was correctly working with the patch in #8, because admin configuration values are submitted/stored as integers then (hint: manual profile update required, resp. an needs also an module update to fix existing profiles). I'll test this once more.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
11.9 KB
11.88 KB

Well, I have tested the latest patch once again and everything worked as it should, even the various ways to override the default status setting (global/user/default). Did you update your existing profile(s) after applying the patch?

Attached patch fixes some stale form item descriptions and also converts the form item in the user account settings form into a checkbox.

I don't know whether it's worth to also develop a module update for this...

sun’s picture

Any updates? I'd like to create a new official release shortly...

AltaVida’s picture

Yeah, the patches work for me. Thanks Sun.

toniw’s picture

These patches are for two different Drupal versions, right?
So for D5 I only apply patch wysiwyg-DRUPAL-5.bool_.patch ?
If that assumption is correct then it does not work for me. After applying the patch TinyMCE is not loading at all anymore.

sun’s picture

@toniw: Did you update your Wysiwyg editor profile after applying the patch? This patch changes the value types of all configuration properties.

toniw’s picture

Yes, I did.
This was on the test-sibling of a production site however, which might have some TinyMCE-module history laying around, if that is at all possible.
Will retry this later today on a fresh D5 sandbox.

jackbravo’s picture

The patch works for me also.

toniw’s picture

Just installed a fresh D5.9 with only wysiwyg-5.x-1.x-dev.tar.gz and tinymce_2_1_3.tgz
confirmed the existing problem
installed patch wysiwyg-DRUPAL-5.bool__1.patch
confirmed that this patch fixes the problem

In other words: patch is ok for D5
(my problem must be caused by something else)

sun’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, committed.

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.