Opened 4 years ago

Closed 4 years ago

#12148 closed Bug (fixed)

Element#getChild modifies passed array

Reported by: Piotrek Koszuliński Owned by: Artur Delura
Priority: Normal Milestone: CKEditor 4.5.0 Beta
Component: General Version:
Keywords: Cc:

Description

Using shift() on a passed argument is a very bad practice.

Setting milestone to 4.5 because unfortunately fix will affect code using getChild().

Change History (9)

comment:1 Changed 4 years ago by Jakub Ś

Status: newconfirmed

comment:2 Changed 4 years ago by Artur Delura

Shouldn't we create parallel method which doesn't modify passed argument, and mark existing one as a deprecated? I suppose that there might be various 3rd party plugins, extensions etc. which use this method and are designed to work just with current implementation. I mean that migration can be difficult, and some plugins might stop working.

comment:3 Changed 4 years ago by Marek Lewandowski

I think we can't affort to make a duplicates in api.

As for this particular change I can't imagine any reasonable usage of the fact that getChild() consumes your array.

Eventually for some "hc optimization enthusiast" who considers every new array a harmful loss of performance.

eg.

var paragraphPath = [ 0, 0, 1 ],
	paragraphChild;
	(...)
	paragraphChild.getChild( paragraphPath );
	
	// Now paragraphPath path will store customer orders! We don't create an
	// extra var for that, because this is a waste of resources!
	// Use paragraphPath as customer orders!
	app.getCustomerOrders( paragraphPath );
	
	(...)
	app.view.displayOrders( paragraphPath );

I don't think that we have lot of such "hardcore" developers out there :)

Summary:

  • I wouldn't expect array parameter to be used. This method is most useful with number.
  • there's no reasonable (API) UC that would be broken if we fix the method
  • as mentioned, typically programmer will expect his input params not to be modified (if it's not mentioned in docs)

Still lets wait what other our devs have to say about that.

comment:4 Changed 4 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

Okay, after some talks we decided that we shouldn't create parallel solution because current behaviour is just a bug and it's not documented as well.

Last edited 4 years ago by Artur Delura (previous) (diff)

comment:5 Changed 4 years ago by Artur Delura

Status: assignedreview

Changes and tests in branch:t/12148.

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

This branch should be based on major, not master. I'll rebase it.

comment:7 in reply to:  6 Changed 4 years ago by Artur Delura

Replying to Reinmar:

This branch should be based on major, not master. I'll rebase it.

Sorry for that, I get used to master.

comment:8 Changed 4 years ago by Artur Delura

I rebased it to major.

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

Resolution: fixed
Status: reviewclosed

Fixed on major with git:8969f97.

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