Opened 7 years ago

Closed 7 years ago

#9636 closed Bug (fixed)

Do not extend the toolbarGroups config object

Reported by: Frederico Caldeira Knabben Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.0
Component: UI : Toolbar Version: 4.0
Keywords: Cc:

Description

Change the appendto.html sample with the following:

var globalConfig = {
	height: 50,
	toolbarGroups : [ { name: 'basicstyles', groups: [ 'basicstyles' ] } ]
};

CKEDITOR.appendTo( 'section1', globalConfig );
CKEDITOR.appendTo( 'section1', globalConfig );
CKEDITOR.appendTo( 'section1', globalConfig );

Check the results :|

It seems that we're touching the original toolbarGroups configuration object internally. This brings troubles when there is a global configuration option in the application, which is reused by several editors.

If we want to use the original toolbarGroups object internally, we should make a depth copy of it instead.

Attachments (1)

9636b.html (668 bytes) - added by Piotrek Koszuliński 7 years ago.

Download all attachments as: .zip

Change History (6)

comment:1 Changed 7 years ago by Piotrek Koszuliński

Owner: set to Piotrek Koszuliński
Status: newassigned

comment:2 Changed 7 years ago by Piotrek Koszuliński

Status: assignedreview

Pushed t/9636 on dev and tests.

comment:3 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

To be bulletproof on other object based configuration as well, let's have it fixed it in the editor creation instead.

Changed 7 years ago by Piotrek Koszuliński

Attachment: 9636b.html added

comment:4 Changed 7 years ago by Piotrek Koszuliński

Status: review_failedreview

Sample 9636b.html shows that t/9636b solves only problem with sharing instance configs - global config is still shared. Thus t/9636 is still required to fix this completely. So we should use both patches. Original one to fix this exact issue and t/9636b to prevent some of the future issue with instance config sharing. But this still won't be enough to ensure 100% safety which can be achieved only by deep cloning of the entire config object.

comment:5 Changed 7 years ago by Piotrek Koszuliński

Resolution: fixed
Status: reviewclosed
Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy