Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove from CSSList via key #224

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bytestream
Copy link

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
Copy link
Contributor

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
Copy link
Author

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.

@@ -246,11 +246,18 @@ public function splice($iOffset, $iLength = null, $mReplacement = null)

/**
* Removes an item from the CSS list.
* @param RuleSet|Import|Charset|CSSList $oItemToRemove May be a RuleSet (most likely a DeclarationBlock), a Import, a Charset or another CSSList (most likely a MediaQuery)
*
* @param RuleSet|Import|Charset|CSSList|int $oItemToRemove May be a RuleSet (most likely a DeclarationBlock), a Import, a Charset or another CSSList (most likely a MediaQuery)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

@oliverklee oliverklee deleted the branch MyIntervals:main February 7, 2024 11:36
@oliverklee oliverklee closed this Feb 7, 2024
@oliverklee oliverklee reopened this Feb 7, 2024
@oliverklee oliverklee changed the base branch from master to main February 7, 2024 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants