Problem/Motivation

Given that \Drupal\Core\Plugin\DefaultSingleLazyPluginCollection uses $this->instanceId it should set it in \Drupal\Core\Plugin\DefaultSingleLazyPluginCollection::addInstanceId

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#7 2726837-plugin-7-PASS.patch4.41 KBtim.plunkett
#7 2726837-plugin-7-FAIL.patch3.38 KBtim.plunkett
#5 set_instance_id-2726837-5.patch672 bytesdipakmdhrm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett as a volunteer commented
Title: \Drupal\Core\Plugin\DefaultSingleLazyPluginCollection::addInstanceId should set $this->instanceID » \Drupal\Core\Plugin\DefaultSingleLazyPluginCollection::addInstanceId should set $this->instanceId
Issue summary: View changes

Fixing capitalization

Vinay15’s picture

Vinay15
he/him
Primary language Marathi
Location Navi Mumbai
CreditAttribution: Vinay15 at Blisstering Solutions commented
Assigned: Unassigned » Vinay15
Vinay15’s picture

Vinay15
he/him
Primary language Marathi
Location Navi Mumbai
CreditAttribution: Vinay15 at Blisstering Solutions commented
Assigned: Vinay15 » Unassigned
dipakmdhrm’s picture

dipakmdhrm CreditAttribution: dipakmdhrm as a volunteer commented
Status: Active » Needs review
FileSize
set_instance_id-2726837-5.patch672 bytes

More context for this would be helpful.

$this->instanceId is already being set in the constructor and addInstanceId() adds an instanceId to list of available ids.
Without any context I am not sure how it would be helpful to set the instance id here.

Attaching a patch in any case.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett as a volunteer commented

Working on tests.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett as a volunteer commented
FileSize
2726837-plugin-7-FAIL.patch3.38 KB
2726837-plugin-7-PASS.patch4.41 KB

Okay. There were actually two bugs here, since setConfiguration() was changing $this->configuration AFTER the get() call, and it needs to happen before.

The last submitted patch, 7: 2726837-plugin-7-FAIL.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Nice work!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed 4e05f84 and pushed to 8.1.x and 8.2.x. Thanks!

I've committed to 8.1.x as this is a BC compatible bug fix.

  • alexpott committed 7c0adf9 on 8.2.x 
    Issue #2726837 by tim.plunkett, dipakmdhrm, dawehner: \Drupal\Core\...

  • alexpott committed 4e05f84 on 8.1.x 
    Issue #2726837 by tim.plunkett, dipakmdhrm, dawehner: \Drupal\Core\...

Status: Fixed » Closed (fixed)

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