Problem/Motivation

#3070880: One of methods documented by Drupal core to create a CKEditor build does not work anymore: update docs improved the documentation in core/assets/vendor/ckeditor/build-config.js. @bkosborne did that, and it definitely helped more people be able to build CKEditor! 🥳

One of the things that stood out was this, from #3070880-6: One of methods documented by Drupal core to create a CKEditor build does not work anymore: update docs:

  1. +++ 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.

To which the response was:

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

But … this is wrong, they must've made a mistake in manual testing. @lauriii was following the updated instructions and couldn't verify #3072382: Update CKEditor to version 4.13 was correct. He asked me to help him figure this out. Turns out this is the problem.

I also manually tested it and verified that indeed the build-config.js file needs to be copied over into CKEditor's dev/builder/ directory.

Proposed resolution

Update the docs to instruct to cp core/assets/vendor/ckeditor/build-config.js ./dev/builder

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

None.

CommentFileSizeAuthor
#3 3088525-3.patch763 byteswim leers
#2 3088525-2.patch755 byteswim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new 3088525-2.patch755 bytes
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new 3088525-3.patch763 bytes

TIL git is stupid: https://github.com/wimleers/dotfiles/commit/8ef617c43db2d41d43b736c11523...

twod’s picture

twod
Primary language Swedish
Location Sweden
commented
Status: Needs review » Reviewed & tested by the community

#3 does indeed work - and applies with git apply ;).

We could be more verbose and tell the user to either mark the build script as executable or prefix it with bash, but I think it's safe to assume that anyone doing builds are at least somewhat familiar with running scripts in their own environment.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 8.9.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed eca31d3dfb to 9.0.x and a7bb1afbd3 to 8.9.x and 5abda73d8c to 8.8.x. Thanks!

Backported to 8.8.x as this is an important docs fix.

There's a pattern of using /PATH/TO and the bit you need to replace all in caps. See core/UPDATE.txt - fixed that on commit.

diff --git a/core/assets/vendor/ckeditor/build-config.js b/core/assets/vendor/ckeditor/build-config.js
index 8583bc353d..071da538ac 100644
--- a/core/assets/vendor/ckeditor/build-config.js
+++ b/core/assets/vendor/ckeditor/build-config.js
@@ -4,7 +4,7 @@
  * To re-create this build:
  * 1. Clone the development repo of CKEditor to your machine
  * 2. Check out the version you'd like to build
- * 3. Copy /drupal/core/assets/vendor/ckeditor/build-config.js ./dev/builder/
+ * 3. Copy PATH/TO/DRUPAL/core/assets/vendor/ckeditor/build-config.js ./dev/builder/
  * 4. Run ./dev/builder/build.sh -s
  *
  * Then, replace this directory (core/assets/vendor/ckeditor) with your build.

Yes the "comment" is longer than 80 chars but it's more important to be clear.

  • alexpott committed eca31d3 on 9.0.x 
    Issue #3088525 by Wim Leers: Follow-up for #3070880: fix instructions...

  • alexpott committed a7bb1af on 8.9.x 
    Issue #3088525 by Wim Leers: Follow-up for #3070880: fix instructions...

  • alexpott committed 5abda73 on 8.8.x 
    Issue #3088525 by Wim Leers: Follow-up for #3070880: fix instructions...

Status: Fixed » Closed (fixed)

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