Remove from CSSList via key

[bytestream]

If you're looping over getContents() and want to remove items from the CSSList it would be more performant to remove items by the key given it's already known. The current method uses array_search which is wasteful in the specified scenario.

[westonruter]

Shouldn't you rather than use the splice() method? The remove() method wasn't intended to accept an array index.

Aside: it seems remove() needs to re-index the numeric keys to make them all sequential.

[bytestream]

Shouldn't you rather than use the splice() method?

unset is marginally faster than array_splice on large arrays. It's also easier to work with, array_splice keeps reindexing so it's difficult to keep track of the offset to remove in a loop

it seems remove() needs to re-index the numeric keys to make them all sequential.

How come? It seems to work OK as it is.

[oliverklee]

I strongly recommend not mixing types likes this in a parameter. Instead, I propose a new, separate method removeByKey(int $key).

[westonruter]

Agreed.

[bytestream]

Happy to make the change but are the points raised in #224 (comment) still a concern with this PR?

[westonruter]

Actually, the the problem I had was specifically with the replace method. The explanation I provided with my workaround in ampproject/amp-wp@d7c565c was:

This is being used instead of CSSList::splice() because it uses array_splice() which does not work properly
if the array keys are not sequentially indexed from 0, which happens when CSSList::remove() is employed.

I did the following to maintain a 0-indexed array:

$contents = array_values( $css_list->getContents() ); // Required to obtain the offset instead of the index.
$offset   = array_search( $old_item, $contents, true );
if ( false !== $offset ) {
	array_splice( $contents, $offset, 1, $new_items );
	$css_list->setContents( $contents );
	return true;
}
return false;