Define workflow for patching CKEditor

[wincent]

This issue is to provide a place for us to discuss how we want to approach the question of applying patches to the upstream CKEditor given our new build system. For context, the old and the new build system are described in #7.

Ways to modify CKEditor

• Submit a patch upstream: This is the ideal scenario for bug fixes of a general nature, because it frees us from having to maintain a fork. On the flip side however, waiting for patches to be published in a released version adds a delay that we don't necessarily control, and not all patches may be accepted.
• Use runtime configuration to modify behavior: We can (and do) use plug-ins to extend or modify the behavior of the editor without modifying its source code.
• Use runtime monkey-patching to modify behavior: This is a more aggressive version of the above, and involves mutating or circumventing the normal functioning of the editor to produce a different result. This is sometimes a cheap and effective measure, but it can be quite difficult to pull off, may result in shipping useless code to the client, and can easily break as we move between versions.
• Maintain a fork: This is basically what we used to do in this repo: the commit graph of the upstream project is in the repo, and then we add commits on top of it; because it's a Git repo, we can use whatever internal structure (branches, tags, directories etc) or tools (cherry-picking, merging etc) that we see fit.
• Maintain a patchset: This is a variation of the fork approach. Instead of adding commits directly to the commit graph, we keep patches (for example, it could be patches created with git-format-patch) in a directory, and apply them as part of the build process. In this approach there are two main ways to get the base version of the code upon which to apply the patches: one is from a submodule (ie. what we currently have in this repo) and the other would be to keep it on a branch (ie. you'd add the CKEditor upstream as a remote and just create a local tracking branch to follow it here).

Observations:

• All these approaches have one thing in common: it is painful to maintain a long-lived set of modifications on top of a third-party project in motion.
• Because all the approaches suck in different ways, I don't think we should obsess over finding a perfect "silver bullet" solution, but rather, find an adequate solution and then provide clear documentation and workflows for applying it.

Proposals

To get the ball rolling on the discussion here, I'll detail a proposal for what I think would be a decent solution.

Proposal 1: Submodule plus explicit patch-list with supporting scripts

• Our default policy for actual bugs should be to try and get an upstream fix accepted whenever possible so that we don't have to maintain a long-lived patch that keeps us divergent from the upstream project.
• Likewise, for changes that are straightforwardly augmentative or restrictive in nature, we should prefer using configuration (eg. turning things off) or add plug-ins to achieve our ends.
• For those cases were we really must maintain a patch against the CKEditor codebase (ie. because we need to cover the lag between our patch to upstream making it into an official release, or because the nature of the patch means that it will probably never get accepted into the upstream), we need a solution for tracking and applying those patches over time.
• All else being equal, we should seek to minimize the total number of patches that we need to manage at any given time.
• We keep the submodule because that provides us with a clear separation between our build tooling and patches and the pristine version of the upstream project.
• Patches live in "patches" directory at the top-level of the project. Patches are created with git format-patch, and applied with git am.
• To make preparing and updating these patches easy, we provide some helper scripts to make it easy to create a branch inside the submodule from those patches, which can then be rebased, edited, added to etc. The utility of the submodule here is that it provides a sandbox in which changes can be experimented with without messing with the superproject. Likewise, it becomes possible to edit the superproject files and the submodule files without having to switch branches.

Variant to this approach:

• A very similar workflow can be attained by using git-worktree to create a directory with a checkout of a branch from the upstream repo. To the developer, the structure looks the same (ie. the upstream code will be at "ckeditor-dev/") and the contents are a commit graph, so you can do all the things that you can usually do with Git (use git-am, git-format-patch, git-rebase, git-merge etc). The only difference is that one is a submodule and the other is a worktree. Advantages of this approach: removes friction related to submodules. Disadvantages: we no longer have a the guarantee that the submodule provides us by pointing at a specific commit of the upstream project, so we'd have to right some supporting code in the build process to force, insofar as it is possible, that the worktree is based on a specific commit from the upstream repo.

Both of these variants would involve what most would consider to be "advanced" Git usage, so my sense is that we'll need to provide some scaffolding in the form of clear documentation and helper scripts to carry out the workflow. My inclination is to give the submodule + patches approach a try first, and then evaluate whether it's working well enough, or whether we should explore an alternative. In any case, we might be able to defer all this for quite some time, as we don't actually need to apply any patches until we actually need to apply any patches... 😂

[wincent]

Mentioning @julien and @jbalsas for input and to add other proposals.

[blzaugg]

👍 submodule + patches approach

[julien]

I'm also in favor of trying the submodule + patch approach and also agree that this should be well documented and possibly automated

[julien]

For anyone interested, this is currently being implemented in #17

[wincent]

Closed by #17 and #19.