I'm trying to debug #3065546: CKEditor's "merge cells" feature for tables does not work in when using Firefox, but I'm running into road block after road block when trying to set up a CKEditor locally unminified. There is a build-config.js file in the core/assets/vendor/ckeditor/ folder which file comments:

/**
* This is a Drupal-optimized build of CKEditor.
*
* You may re-use it at any time at http://ckeditor.com/builder to build
* CKEditor again. Alternatively, use the "build.sh" script to build it locally.
* If you do so, be sure to pass it the "-s" flag. So: "sh build.sh -s".
*
* If you are developing or debugging CKEditor plugins, you may want to work
* against an unoptimized (unminified) CKEditor build. To do so, you have two
* options:
* 1. Upload build-config.js to http://ckeditor.com/builder and choose the
* "Source (Big N'Slow)" option when downloading.
* 2. Use the "build.sh" script to build it locally, with one additional flag:
* "sh build.sh -s --leave-js-unminified".
* Then, replace this directory (core/assets/vendor/ckeditor) with your build.
*
* NOTE:
* This file is not used by CKEditor, you may remove it.
* Changing this file will not change your CKEditor configuration.
*/

I'm following step 1 and it does not work. When I download the replacement and replace the directory, I get error TypeError: CKEDITOR.skinName is undefined in skin.js.

I hacked something in so that that var gets defined, but then I get an error about missing the Image2 plugin, despite it being there.

Anyone know of better steps to follow here?

CommentFileSizeAuthor
#7 3070880-ckeditor-build-docs-7.patch1.58 KBbkosborne
#5 3070880-ckeditor-build-docs.patch1.58 KBbkosborne
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bkosborne created an issue. See original summary.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
CreditAttribution: bkosborne at Princeton University commented
Related issues: +#2480333: Document how to use an unminified CKEditor build (for developing/debugging)
bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
CreditAttribution: bkosborne at Princeton University commented

Okay, so the second set of instructions seems to work (doesn't produce a broken build). So I think we need to remove the first option since it doesn't work, and then improve the instructions for the 2nd option since they are vague. It's not clear if the "build.sh" file is part of core or CKEditor for example, or that you need to pass in the build-config.js file from core that that script.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Category: Bug report » Task
Priority: Normal » Minor

Yeah it seems that the CKEditor team broke the online builder along the way. But … that used the second option under the hood anyway. That second option is exactly how the CKEditor team themselves do releases.

Could you create a patch that removes that first option from the docs? :)

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
CreditAttribution: bkosborne at Princeton University commented
Status: Active » Needs review
FileSize
3070880-ckeditor-build-docs.patch1.58 KB

How's this?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Looking good!

Just a few thoughts:

  1. +++ b/core/assets/vendor/ckeditor/build-config.js
@@ -1,19 +1,17 @@
+ * 2. Checkout the version you'd like to build

    Nit: s/Checkout/Check out/ 

  2. +++ b/core/assets/vendor/ckeditor/build-config.js
@@ -1,19 +1,17 @@
+ * 3. Run ./dev/builder/build.sh -s /path/to/build-config.js

    Does this actually work? It used to be the case that you first needed to cd dev/builder and only then would build.sh work as expected.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
CreditAttribution: bkosborne at Princeton University commented
FileSize
3070880-ckeditor-build-docs-7.patch1.58 KB

Yes, it works. I tested both methods and compared the output and it's identical.

Fixed the grammar issue. No interdiff due to small patch size. Thank you for review.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: Debugging CKEditor does not work following core's instructions » One of methods documented by Drupal core to create a CKEditor build does not work anymore: update docs
Status: Needs review » Reviewed & tested by the community

Excellent! Thank you very much, @bkosborne! 👍

  • webchick committed 9ea3db3 on 8.8.x 
    Issue #3070880 by bkosborne: One of methods documented by Drupal core to...
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Great catch! Thanks for the updated docs!

Committed and pushed to 8.8.x, and backported to 8.7.x. Thanks!

  • webchick committed e2f9847 on 8.7.x 
    Issue #3070880 by bkosborne: One of methods documented by Drupal core to...
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#3088525: Follow-up for #3070880: fix instructions for how to direct CKEditor's build system to use Drupal's build-config.js

Turns out that #6.2 was a problem, unlike what #7 says. See #3088525: Follow-up for #3070880: fix instructions for how to direct CKEditor's build system to use Drupal's build-config.js.