Add custom skin support

Patch looks good to me. And it seems good to also have the option to set a skins folder. I was looking for the same thing...
Patch doesn't pass because: ERROR: No valid tests were specified.

This patch is AWESOME guys, great work.

It also fixed a skin problem for me when using a local copy of ckeditor 4.6.2 , for some reason my skin was not working correctly until I used this patch.

I haven't looked at the code, but the results are amazing! Great work guys!

Without this, the skin functionality is broken, so definately, this patch is needed asap! Especially if you are not using the CDN, so as far as the above reports say, this patch is good for everyone!. Great work.

1. +++ b/includes/ckeditor.admin.inc
   @@ -327,6 +327,48 @@ function ckeditor_admin_global_profile_form($form, $form_state, $mode = 'add') {
   +          '!library' => '<code>' . $drupal_library_path . '

   ',
   

   Is this safe against XSS? I think you better use @placeholders and move the inline HTML into the t().

2. +++ b/includes/ckeditor.admin.inc
   @@ -327,6 +327,48 @@ function ckeditor_admin_global_profile_form($form, $form_state, $mode = 'add') {
   +    '#maxlength' => 128,
   +    '#description' => t(
   +        'The path to the local directory (on the server) that points to the path defined above. Enter either an absolute server path or a path relative to the !indexphp file. If left empty, the CKEditor module will try to find the right path.', array(
   +          '!indexphp' => '<code>index.php

   '
   + )
   

   No line breaks, please.

3. +++ b/includes/ckeditor.admin.inc
   @@ -458,7 +500,21 @@ function ckeditor_admin_global_profile_form($form, $form_state, $mode = 'add') {
   +      $err_msg = t('When using the CKEditor CDN and a skin other than Kama or Moono, the');
   +      $err_msg .= ' <strong>skin.js</strong> ';
   

   Such line breaks are also not allowed in translatable strings. THe full code block is incorrect here.

4. +++ b/includes/ckeditor.admin.inc
   @@ -458,7 +500,21 @@ function ckeditor_admin_global_profile_form($form, $form_state, $mode = 'add') {
   +        '!link' => l('CKEditor Addons', 'http://ckeditor.com/addons/skins/all', array('external' => TRUE))));
   

   This is not allowed. Never use l().

Aside of this I have successfully compiled custom skins on cckeditor website and just used it in Drupal. A setting is not required for this.

The patch in #5 is the recommended way to install CKEditor alongside Media module in this script: https://www.drupal.org/node/2843391

#11 seems address #5 but we need a proper review.

***EDIT*** thanks for the recent release
now , we need to get this patch in.

patch 11 no longer works with the latest release 7.x-1.18
it needs some work

ALTHOUGH, I was testing with ckeditor 4.6.2

haven't yet tested with ckeditor 4.7.1

actually, it could be that I need to download some additional skins, I see some messages in my watchdog warning me of missing skin files.

confirm: Patch #11 applies cleanly to CKEditor 7.x-1.18.
Great patch/addition! Will probably use this in production.
would be great to see this in the next stable release

This is excellent - I struggled with getting a new theme applied and this did the trick, applied against 7.x-1.x-dev.

I'm having the issue where it no longer discovers non-default skins.
I have the following setup:
Latest dev version of this module (currently rev: 9ea879baf9e52a4a2e3b61addadb8c4ce8b233b0)
I'm using the latest stable version of the library specifically this: https://download.cksource.com/CKEditor/CKEditor/CKEditor%204.11.1/ckedit...

That library comes with just 1 skin (the new default): moono-lisa.
I add the moono skin (https://download.ckeditor.com/moono/releases/moono_4.11.1.zip)
At [libraries]/ckeditor/skins/moono

When using the #11 patch I only see the default moono-lisa skin.
When NOT using the #11 patch I see both the moono and moono-lisa skin.

It is possible that the patch from #2947109: New default CKEditor skin not recognized interferes with the #11 patch. I've not looked further into this issue I must confess.

The bigger issue is why this patch has oscillated between "RTBC" and "Needs work" for almost 2 years without it ever being committed. On a long enough timeline, every patch will conflict with something and never be committed.

It has been oscillated between RTBC and NW because there was never a review for it. "It works" is not really a review of a patch. Security question was not really answered.

We've been using various versions of this patch for almost three years with no problems. The patch in the other issue has only existed since last year.

Just because some patch is older does not influence which issue should be moved to "needs work".
Since the other issue is already committed, any issues should be resolved in the non-committed issue, like this one.

If there is a possible conflict with #2947109, someone needs to test it rather than just assuming this patch doesn't work.

I've tested it, albeit only functionally. The result was a problem with this patch so I've moved it to needs work. As we need to either confirm there is no bug with this patch OR resolve the problem, before this patch should be committed. Even if the bug is caused merely by passed time, bugs are bugs and should be resolved.

But I do recognize that it sucks that an issue is taking this long and that it can be frustrating for you as a contributor. However that is the reality of open source development. You are either dependent on the maintainer (and reviewers), or you have to be the maintainer and put in the work of resolving issues.

the security question was answered in #2671806-11: Add custom skin support

ron_s is using the same approach as is used elsewhere in the module. This is consistent, so if there is an issue it should be addressed in a new issue, not this one. I'd say the patch is good to commit.

and , if there's a problem with the consistent patterns in this module it should be addressed in one felt swoop in a separate issue rather than block a fix.

ron_s addressed the issues in patch 11. No one had issue directly with his comments and I suggest if there is an issue that it be dealt with in a new issue rather than stall this one indefinitely.

I recommend to the maintainer to push in patch# 11

Then deal with related concerns in the followup issue
#3025136: code style and security hardenning for ckeditor.

Setting NR for #30.

@ron_s I've tested a bit the patch and here is what I found

1. Skins are listed from the local directory even when no paths for custom skin is set. When such skin is chosen and settings are saved. This causes a 404 error. There is a message of invalid configuration, but still.
So those plugins should not be listed until the path is set. There should be no default path returned when custom (local) skin directory is not set.

2. The default skin path when there is no config set yet points to /skins of the CKEditor module. This may cause some inconvenience, as there is a dedicated folder /ckeditor in which there is a file COPY_HERE.txt. So it would be a better place, as users would put their local CKEditor there - and folders would match (plugins from CKEditor would have the same location).

I've used your patch and extended it with my proposals. Please do a review.

BTW. I have added fieldsets to better group those settings, have fun :-)

@vokiel

in case you're wondering why your patch didn't apply, check the documentation

Thank you @ron_s for your comments.

I'll just add my point of view regarding this:

This patch is a copy/paste of identical code elsewhere in the module. If the goal is to make the changes you mentioned, then a full audit should be done to make changes everywhere they exist.

I noticed that I was thinking about ignoring it but the possible error with invalid skin setting made me to do those changes. This is a new code (even it's copy/paste) so it should be clean. Refactoring all the code does not make sense for me here. It could be done as a separate task, although Drupal 7 will reach end-of-life in November of 2021, there is Drupal 8 and Drupal 9 coming soon. So there is less and less ROI in investing time in refactoring.

As for the paths, it's a bit different in the case of CKEditor itself. When a user has the module enabled it should find any working CKEditor even if paths are not manually set yet. Plugins are defined within the module so the case is different as well. Setting the default path (if skins are found on the disk) may cause Editor to crash.

As for the validation:

// (1)
  //strip slash from the end
  if (empty($edit['ckeditor_skins_path'])) {
    $edit['ckeditor_skins_path'] = '';
  }

// (2)
  $edit['ckeditor_skins_path'] = trim(rtrim($edit['ckeditor_skins_path'], "/"));
  if ($edit['ckeditor_skins_path'] && 0 !== strpos($edit['ckeditor_skins_path'], "/") && 0 !== strpos($edit['ckeditor_skins_path'], "%")) {
    //ensure that slash is at the beginning
    $edit['ckeditor_skins_path'] = "/" . $edit['ckeditor_skins_path'];
  }

// (3)
  //no slash at the end
  $edit['ckeditor_skins_path'] = trim(rtrim($edit['ckeditor_skins_path'], "/"));

1. It does not strip slash from the end, it basically does nothing because if the path is empty, then it's an empty string.
2. Here we have removal of the trailing slash, so the comment from 1 should be here, and point 1 can be removed.
3. Again, trailing slash removal, which repeats the step from 2. I see now that it was just a typo, there should be $edit['ckeditor_skins_local_path'] not $edit['ckeditor_skins_path'].

@joseph.olstad picky exporting from PHPStrom didn't work :-(

I'm attaching a patch with one fix (added extra trim()), which is based on the latest dev.

No other patches should be applied. The one form ticket #3120705 is a different issue so it should be treated separately.

Fixed with 308c58d