Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Same as contextual.js & co.
(the js is confusing to read)
See related issues.
Beta phase evaluation
Issue category | Task neither a bug nor a new feature; solely refactoring. |
---|---|
Unfrozen changes | Unfrozen because it doesn't change anything, it only does internal refactoring: it splits up existing JS across multiple files. No contrib/custom JS would ever want to override this. |
Prioritized changes | The main goal of this issue is unblocking #2182153: [Meta] Document Drupal JavaScript using JSDoc, which is a documentation issue, and hence has long-term DX consequences for front-end developers using D8. |
Disruption | No disruption. |
Proposed resolution
Split up ckeditor.admin.js
Remaining tasks
- Evaluate if allowed at this point in the beta release.
User interface changes
No.
API changes
No.
Comment | File | Size | Author |
---|---|---|---|
#30 | 2174589-30.patch | 110.39 KB | Wim Leers |
Comments
Comment #1
Wim LeersComment #2
Wim LeersComment #3
el7cosmosComment #4
nod_There is an extra
,
in ckeditor.admin.js but more importantly there are references toregisterButtonMove
and a couple other functions in the split files. Those functions are local to the ckeditor.admin.js scope. They can't work from an other file. Those functions needs to be added to theDrupal.ckeditor
object so they can be used elsewhere.Comment #5
el7cosmosAnything else i missed?
Comment #10
setvik CreditAttribution: setvik commentedHi Wim,
I'm at the PNW Drupal Summit core sprint and was just about to try re-rolling this patch, but noticed you just requeued for testing about 40 min. ago. and wanted to make sure i wasn't duplicating work. If you working on it now, let me know and i'll tackle something else.
Comment #11
setvik CreditAttribution: setvik commentedRerolled against HEAD. No other changes.
Comment #12
Wim LeersI don't know how you rerolled, but unfortunately the reroll is wrong. The changes introduced by the last commit, for example, are lost.
When rerolling a patch that is splitting up a file into multiple files, you must make sure to include any new changes in the additions, rather than only updating the deletions to allow the patch to be applied.
Since this is a JS-only patch, it doesn't matter what testbot says: we have no JS test coverage, hence a green patch doesn't mean anything, unfortunately.
Could you reroll this again? Thanks!
Comment #13
Wim LeersComment #14
ikeigenwijs CreditAttribution: ikeigenwijs commentedComment #15
ikeigenwijs CreditAttribution: ikeigenwijs commentedComment #16
Wim LeersregisterGroupMove()
andopenGroupNameDialog()
still need the same treatment asregisterButtonMove()
already received: they need to be moved onto theDrupal.ckeditor
object. Then this should be good to go! :)Comment #17
ikeigenwijs CreditAttribution: ikeigenwijs commentedComment #18
ikeigenwijs CreditAttribution: ikeigenwijs commentedregisterGroupMove() and openGroupNameDialog()
changes added to patch
interdiff.txt between patch of comment 15 en 18
Comment #19
ikeigenwijs CreditAttribution: ikeigenwijs commentedComment #20
Wim LeersWorks perfectly.
But:
Model.js
, there was no empty line. So, added that.Comment #21
YesCT CreditAttribution: YesCT commentedHere is the result of git diff -w from 18 to 20.
Comment #22
YesCT CreditAttribution: YesCT commentedEvaluating this normal task per https://www.drupal.org/contribute/core/beta-changes
Updating the issue summary.
Note, according to the allowed changes handbook, this maybe should be postponed till 8.1.x
Could it reduce fragility? Or maybe improve performance (maybe it loads only the js that is needed)?
----
I read the related issues.. in #2162407: Split up toolbar.js the motivation is:
it is confusing to read.
I'm not sure I see the point of this issue. At least if this issue motivation is more than that, the summary should be updated.
Comment #23
nod_It's needed for us to be able to document our JS (JSDoc has problems when documenting all this stuff in one file for some reason, it's a JSDoc issue but it doesn't mean all this should be in a single file).
It's still the plan to do #2182153: [Meta] Document Drupal JavaScript using JSDoc It's just been a while since I could block another 12 hours to reroll that patch.
Comment #24
ikeigenwijs CreditAttribution: ikeigenwijs commentedthx Wim
So will the patch be submitted?
Comment #25
Wim Leers#24: You've already submitted/posted the patch. The patch is now marked as "Reviewed & tested by the community", which means that the next step is for a Drupal 8 core committer to do a final review and if no problems are found, commit it.
Comment #26
alexpottThe problem with this patch is the whilst I can see the benefits - I'm not sure they outweigh the risks - we have no test coverage of javascript patches. It would be good to see evidence of manual testing. I think it is worth trying to move to a javascript documentation standard and if this unblocks #2182153: [Meta] Document Drupal JavaScript using JSDoc then ok - but test evidence please.
Comment #27
Wim LeersI'm sorry, I didn't mention it, but I did extensive manual testing prior to RTBC'ing in #20.
Together with Jesse Beach, I was responsible for getting this into Drupal 8, and precisely because it is the result of *so* many hours of development, to make the UX and accessibility of configuring CKEditor awesome, I would never RTBC a patch like this if I weren't certain it didn't cause any problems. I thoroughly tested it manually.
Comment #28
Wim LeersAdded beta evaluation.
Comment #29
alexpottComment #30
Wim LeersDarn, I really need to write a
drupallint
script to run the various coding standards linters to prevent this.Manually tested again.
Comment #31
alexpottCommitted b61edd7 and pushed to 8.0.x. Thanks!
Comment #33
Wim Leers