In \Drupal\Core\State\State, parent::delete($key) is called twice [#2884223]

API page: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21State%21S...

State::delete() calls parent::delete(), and then State::deleteMultiple(), which then calls parent::delete() for each of the received keys.

State::delete() should just calls State::deleteMultiple(), and the code would become the following.

public function delete($key) {
  $this->deleteMultiple([$key]);
}

Comment	File	Size	Author
#14	remove-parent-delete-2884223-14.patch	425 bytes	apaderno
#14	8.3.x: PHP 5.6 & MySQL 5.5 21,827 pass
#8	remove-parent-delete-2884223-8.patch	425 bytes	apaderno
#8	8.4.x: PHP 5.6 & MySQL 5.5 21,992 pass
#6	remove-parent-delete-2884223-6.patch	397 bytes	apaderno
#6	8.4.x: PHP 5.6 & MySQL 5.5 22,067 pass
#4	remove-parent-delete-2884223-4.patch	406 bytes	apaderno
#4	8.4.x: PHP 5.6 & MySQL 5.5 Patch Failed to Apply
#2	remove-parent-delete-2884223-2.patch	404 bytes	apaderno
#2	8.4.x: PHP 5.6 & MySQL 5.5 Patch Failed to Apply

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

6 June 2017 at 21:55

kiamlaluno created an issue. See original summary.

Comment #2

apaderno

he/him

Italian

Brescia, 🇮🇹 🇪🇺

CreditAttribution: apaderno as a volunteer commented 6 June 2017 at 22:10

Status:

Active

» Needs review

File	Size
remove-parent-delete-2884223-2.patch	404 bytes
8.4.x: PHP 5.6 & MySQL 5.5 Patch Failed to Apply

Comment #3

6 June 2017 at 22:12

Status:

Needs review

» Needs work

The last submitted patch, 2: remove-parent-delete-2884223-2.patch, failed testing. View results

Comment #4

apaderno

he/him

Italian

Brescia, 🇮🇹 🇪🇺

CreditAttribution: apaderno as a volunteer commented 7 June 2017 at 08:07

Status:

Needs work

» Needs review

File	Size
remove-parent-delete-2884223-4.patch	406 bytes
8.4.x: PHP 5.6 & MySQL 5.5 Patch Failed to Apply

Moving the file from Ubuntu, I lost a new line.

Comment #5

7 June 2017 at 08:09

Status:

Needs review

» Needs work

The last submitted patch, 4: remove-parent-delete-2884223-4.patch, failed testing. View results

Comment #6

apaderno

he/him

Italian

Brescia, 🇮🇹 🇪🇺

CreditAttribution: apaderno as a volunteer commented 7 June 2017 at 08:42

Status:

Needs work

» Needs review

File	Size
remove-parent-delete-2884223-6.patch	397 bytes
8.4.x: PHP 5.6 & MySQL 5.5 22,067 pass

Comment #7

apaderno

he/him

Italian

Brescia, 🇮🇹 🇪🇺

CreditAttribution: apaderno as a volunteer commented 7 June 2017 at 20:42

It makes more sense to simply change the code as follows.

public function delete($key) {
  parent::delete($key);
  $this->keyValueStore->delete($key);
}

It doesn't make sense to call $this->deleteMultiple([$key]), which needs to loop over an array containing a single item, just to avoid calling $this->keyValueStore->delete($key) directly.

Comment #8

apaderno

he/him

Italian

Brescia, 🇮🇹 🇪🇺

CreditAttribution: apaderno as a volunteer commented 7 June 2017 at 20:48

File	Size
remove-parent-delete-2884223-8.patch	425 bytes
8.4.x: PHP 5.6 & MySQL 5.5 21,992 pass

Comment #9

Dinesh18 CreditAttribution: Dinesh18 as a volunteer commented 8 June 2017 at 06:52

Status:

Needs review

» Reviewed & tested by the community

I looked into both the delete and deleteMultiple API page. If you will look into the deleteMultiple , they have just used foreach for keys. So, for delete we just need to remove the foreach and rest remains the same.
#8 looks great to me. Changing the status to RTBC.

Comment #10

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 8 June 2017 at 09:58

Title:

parent::delete($key) is called twice

» In \Drupal\Core\State\State, parent::delete($key) is called twice

Clarifying title.

Comment #11

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 8 June 2017 at 20:32

Looks good to me

Comment #12

9 June 2017 at 11:49

catch committed a75874b on 8.4.x

Issue #2884223 by kiamlaluno, Dinesh18: In \Drupal\Core\State\State,...

Comment #13

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 9 June 2017 at 11:49

Status:

Reviewed & tested by the community

» Fixed

Committed a75874b and pushed to 8.4.x. Thanks!

Comment #14

apaderno

he/him

Italian

Brescia, 🇮🇹 🇪🇺

CreditAttribution: apaderno as a volunteer commented 9 June 2017 at 20:58

Version:	8.4.x-dev	» 8.3.x-dev
Status:	Fixed	» Needs review

File	Size
remove-parent-delete-2884223-14.patch	425 bytes
8.3.x: PHP 5.6 & MySQL 5.5 21,827 pass

Should I provide the patch also for previous versions?

Comment #15

Dinesh18 CreditAttribution: Dinesh18 as a volunteer commented 10 June 2017 at 04:59

Status:

Needs review

» Reviewed & tested by the community

Patch #14 looks good to me.
Changing the status to RTBC.

Comment #16

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy at Acquia commented 12 June 2017 at 08:19

Version:	8.3.x-dev	» 8.4.x-dev
Status:	Reviewed & tested by the community	» Fixed

I think if @catch intended to cherry-pick to Drupal 8.3.x, he would have. I don't think it does harm to cherry-pick neither it does harm to not, so going to move this back to 8.4.x fixed. Thanks for providing the patch either way!