Problem/Motivation

#2949964: Add an EntityOwnerTrait to standardize the base field needed by EntityOwnerInterface got its update numbers wrong.

Proposed resolution

Update them quickly.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#9 interdiff.txt1.53 KBlongwave
#9 2999306-9.patch7.07 KBlongwave
#8 2999306-6.patch6.34 KBalexpott
#8 0-6-interdiff.txt620 bytesalexpott
2949964-quick-follow-up.patch6.23 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Issue summary: View changes
Parent issue: » #2949964: Add an EntityOwnerTrait to standardize the base field needed by EntityOwnerInterface

alexpott credited chr.fritsch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented

This was spotted and reported to irl by @chr.fritsch

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Status: Needs review » Reviewed & tested by the community

Phew, good catch! From what I can tell, there are no side effects from running these twice, so any risk here pretty well mitigated.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave as a volunteer commented 
+++ b/core/modules/comment/tests/src/Functional/Update/CommentHostnameUpdateTest.php
@@ -44,9 +44,9 @@ public function testCommentUpdate8600() {
   public function testCommentUpdate8601() {

This test method has a misleading name now.

edit: this is actually wrong

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave as a volunteer commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/comment/tests/src/Functional/Update/CommentHostnameUpdateTest.php
@@ -44,9 +44,9 @@ public function testCommentUpdate8600() {
   /**
-   * Tests comment_update_8601().
+   * Tests comment_update_8700().
    *
-   * @see comment_update_8601
+   * @see comment_update_8700
    */
   public function testCommentUpdate8601() {

I don't think these should change? But comment_update_8601 doesn't exist, as tested here?

This patch also doesn't touch core/modules/comment/tests/src/Functional/Update/CommentUpdateTest.php

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Status: Needs work » Needs review
FileSize
0-6-interdiff.txt620 bytes
2999306-6.patch6.34 KB

@longwave++

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave as a volunteer commented
FileSize
2999306-9.patch7.07 KB
interdiff.txt1.53 KB

No, that is not right, that test is misnamed already - it's for a post_update hook.

Fixed here, out of scope but I hope that is ok.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented

@longwave I think that fixing update numbering and test names is in scope.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented
Status: Needs review » Reviewed & tested by the community

All the changes are looking good to me.

alexpott’s picture

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

Committed e432eea and pushed to 8.7.x. Thanks!

  • alexpott committed e432eea on 8.7.x 
    Issue #2999306 by longwave, chr.fritsch, alexpott: Update numbering -...

Status: Fixed » Closed (fixed)

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