Problem/Motivation

Currently DefaultLazyPluginCollection instantiates the plugins in order to sort the instances by the plugin ID. This is unnecessarily non-lazy since DefaultLazyPluginCollection already provides a mechanism to find the plugin ID from the configuration using the pluginKey property.

Steps to reproduce

Proposed resolution

Be more lazy, get the plugin id from config if not already instantiated.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#14 3520997-nr-bot_yex6gtt7.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3520997

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

jonathanshaw created an issue. See original summary.

jonathanshaw opened merge request !11934

jonathanshaw changed the visibility of the branch 3520997-2 to hidden.

jonathanshaw changed the visibility of the branch 3520997-defaultlazyplugincollection-unnecessarily-instantiates to hidden.

jonathanshaw changed the visibility of the branch 11.x to hidden.

jonathanshaw changed the visibility of the branch 3520997- to hidden.

jonathanshaw opened merge request !11935

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented
Version: 11.1.x-dev » 11.x-dev
jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented
Status: Active » Needs review
mxh’s picture

mxh commented

Good catch. Took a brief look into this and left two minor side notes.

mxh’s picture

mxh commented

Added another small note regards the array lookup. Other than that the code changes look good. Unfortunately I have no time to actually test it on a site.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Appears to be 1 open threads around the return. If that changes though do wonder if it will need test coverage that the exception is correctly thrown.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented
Status: Needs work » Needs review

I'm not sure what's going on with the tests, I think the fails are unrelated.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3520997-nr-bot_yex6gtt7.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented
Status: Needs work » Needs review

Tests pass now, test coverage requested in #12 added, no outstanding feedback. I think this is ready for RTBC.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Left small comments on the MR.

Wonder if this needs a CR?

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented
Status: Needs work » Needs review

I don't believe it needs a CR as it is only a minor performance optimisation, is fully BC, and it will be very rare that a class extending DefaultLazyPluginCollection will need or want to do anything to make use of this optimisation. Any child class that doesn't customise the sortHelper will get this optimisation effortlessly, and any class that doesn't won't need it.

dcam’s picture

dcam commented
Status: Needs review » Needs work

I left a comment on the MR. I'm setting the status to Needs Work for that to be handled. I think it's close to being ready.

I agree that this doesn't need a change record.

shalini_jha made their first commit to this issue’s fork.

shalini_jha’s picture

shalini_jha commented
Status: Needs work » Needs review

Rebased the MR, fixed conflict, and addressed the feedback. Moving this back to NR.

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

The change seems sensible to me. I don't have any additional feedback.

dcam’s picture

dcam commented
Issue tags: +Performance

jonathanshaw changed the visibility of the branch 3520997-3 to hidden.

  • catch committed 041315ba on 11.x 
    performance: #3520997 DefaultLazyPluginCollection unnecessarily...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !11935

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.