This is to implement step 1 as per sun's comment #164 of #746524-164: No Font Styles for CKeditor.

A definitive solution of #746524: No Font Styles for CKeditor seems to be hanging on editor specific code being brought into the wysiwyg_profile_form function in the wysiwyg.admin.inc file. This patch introduces the "settings form callback" mechanism that will allow the above mentioned issue to be solved in a way that is acceptable for the maintainers.

Marking critical as the other issue is already running for over 2 years and has more than 120 followers

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Status: Needs review » Needs work

Thank you for taking this on properly! :)

+++ b/editors/ckeditor.inc
@@ -120,6 +121,19 @@ function wysiwyg_ckeditor_themes($editor, $profile) {
+ *
+ * @param array $form
+ * @param array $form_state

Let's add short descriptions for these params. form.inc or also system.api.php in Drupal core contain some standard/default descriptions for them, which we can copy.

+++ b/editors/ckeditor.inc
@@ -120,6 +121,19 @@ function wysiwyg_ckeditor_themes($editor, $profile) {
+function wysiwyg_ckeditor_settings_form(&$form, &$form_state) {

This should rather go into the API documentation in wysiwyg.api.php. We can do an actual implementation for ckeditor in the issue you mentioned.

+++ b/wysiwyg.admin.inc
@@ -299,6 +299,12 @@ function wysiwyg_profile_form($form, &$form_state, $profile) {
+  if (function_exists($editor['settings form callback'])) {

Let's add an &&ed first condition to check for whether there is any callback set before checking whether the callback function exists.

+++ b/wysiwyg.admin.inc
@@ -299,6 +299,12 @@ function wysiwyg_profile_form($form, &$form_state, $profile) {
+    call_user_func_array($editor['settings form callback'], array(&$form, &$form_state));

The cufa() is not necessary here. Just invoke the defined callback function directly and pass $form and $form_state as arguments.

Note that you need to remove the & from the arguments, since pass-by-reference is deprecated and throws a PHP warning. Arguments can only be taken by reference, but not passed by reference.


Aside from these issues, this looks very good to me.

fietserwin’s picture

Thanks for your reaction. Will make the changes later, but have some questions regarding your remarks:

- function_exists() accepts NULL as parameter and returns FALSE without any warning (and default callback is set to NULL). So I removed the test on isset/!empty (it was there before I did some tests). But I will be happy to reintroduce it for clarity.
- I was looking for a place to document the function, but as wysiwyg.api.php does not document hook_editor(), I could not find the correct place to do so!?
- cuf() cannot be used when arguments are taken by reference and I never seen the $func() syntax on variable function invocations in Drupal code. That is why I used cufa(). I am not sure though on the necessity of using & in the array creation. would need to test that.

fietserwin’s picture

Status: Needs work » Needs review
FileSize
937 bytes

New patch:
- removed empty "implementation" from ckeditor
- no documentation, as there is no place to document hook_editor array entries (wywiwyg.api.php focuses on plugins)
- removed cufa() as per #1. May be difficult to read if you are not used to this syntax, but within an if that does a function_exists(0 it should not be too hard.
- no condition to check !empty first: not needed (as per #2)
- when writing a patch for the other issue, I will document the parameters as per #1

sun’s picture

Status: Needs review » Fixed
FileSize
3.89 KB

Sorry, that's not what I meant.

Anyway, to unblock the other issue, I've committed attached patch.

Thanks for reporting, reviewing, and testing! Committed to 7.x-2.x.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

If we want/need to backport this to D6, feel free to re-open this issue.

fietserwin’s picture

OK, I will go write a patch for the other issue. I saw you prefer to fail very visibly instead of silently (not checking if the function exists, just calling it). That is normally indeed better and the function should just exists if you define it in a hook.

sun’s picture

Status: Fixed » Needs review
FileSize
1.51 KB

Quick follow-up, since I didn't like the duplication of retrieving contextual values that actually already exist at all.

fietserwin’s picture

I have also been thinking about whether to use this format or go for similarity with hook_form_alter list. Differences are minor, so pick your preferred one.

sun’s picture

Status: Needs review » Fixed

Committed and pushed to all 2.x branches.

TwoD’s picture

I like not having to fetch those context objects again, but we could also have baked them into $form_state to keep the function signature simpler. Or am I missing something?

sun’s picture

Status: Fixed » Needs review
FileSize
2.04 KB

LOL! You're absolutely right. I can't believe I did this ;)

Also, sorry for the quick commit - I assumed this would be simple enough, but alas... ;)

sun’s picture

Any objections? :)

TwoD’s picture

Status: Needs review » Reviewed & tested by the community

Nope. =)

sun’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to all 2.x branches.

Status: Fixed » Closed (fixed)

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