Comment Errors

"SiteBrandingConvertedIntoBlockUpdateTest.php" was based on a similar update test but unfortunately a number of comments in the new file were not updated and have incorrect references.

Correct comments

Provide a patch to update the comments.

CommentFileSizeAuthor
#18 interdiff-Followup_to_Use_a_Branding_Block_docs-2567077-12-18.txt642 bytesdcrocks
#18 Followup_to_Use_a_Branding_Block_docs-2567077-18.patch683 bytesdcrocks
#12 interdiff-Followup_to_Use_a_Branding_Block_docs-2567077-9-12.txt657 bytesdcrocks
#12 Followup_to_Use_a_Branding_Block_docs-2567077-12.patch728 bytesdcrocks
#9 Followup_to_Use_a_Branding_Block_docs-2567077-9.patch723 bytesdcrocks
#4 interdiff-Followup_to_Use_a_Branding_Block-2567077-1-3.txt468 bytesdcrocks
#4 Followup_to_Use_a_Branding_Block-2567077-3.patch1.46 KBdcrocks
#2 Followup_to_Use_a_Branding_Block-2567077-1.patch1 KBdcrocks

Comments

dcrocks created an issue. See original summary.

dcrocks’s picture

dcrocks commented
StatusFileSize
new Followup_to_Use_a_Branding_Block-2567077-1.patch1 KB

Here is a quick patch. Are there other small followups that could be added here?

dcrocks’s picture

dcrocks commented
Status: Active » Needs review

duh

dcrocks’s picture

dcrocks commented
StatusFileSize
new Followup_to_Use_a_Branding_Block-2567077-3.patch1.46 KB
new interdiff-Followup_to_Use_a_Branding_Block-2567077-1-3.txt468 bytes

Added one more small change.

rainbowarray’s picture

rainbowarray
she/her
Primary language English
Location Twin Cities, Minnesota
commented
Issue tags: +Needs manual testing, +Needs Wim Leers

We may want input from Win and/or manual testing on the upgrade path. Since Wim suggested this change, should be fine, but it would be great to have sign-off from him.

dcrocks’s picture

dcrocks commented
Issue summary: View changes

Since 8.0 has a RC can we still fix the cache setting issue or just the documentation error?

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Assigned: Unassigned » wim leers
Issue tags: -Needs Wim Leers +rc target triage

Mostly documentation but but the patch removes something small in the update.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Needs work

Thanks for the patch!

This patch seems to be several changes, some of which are rc eligible as docs-only changes, and one of which is not clear to me.

Can we split it into two issues?

+++ b/core/modules/system/src/Tests/Update/SiteBrandingConvertedIntoBlockUpdateTest.php
@@ -10,9 +10,10 @@
- * Tests the upgrade path for local actions/tasks being converted into blocks.
+ * Tests the upgrade path for "Use branding block in place of page template
+ * branding variables (site name, slogan, site logo)".

One line summaries need to be one line, with additional information on additional paragraphs. Reference: https://www.drupal.org/node/1354#drupal

dcrocks’s picture

dcrocks commented
Title: Followup to Use a Branding Block - 2005546 » Followup to Use a Branding Block docs - 2005546
Status: Needs work » Needs review
StatusFileSize
new Followup_to_Use_a_Branding_Block_docs-2567077-9.patch723 bytes

Another patch did part of what this patch was supposed to so restarted with a new patch. Also split so this has only the documentation fix.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented 
+++ b/core/modules/system/src/Tests/Update/SiteBrandingConvertedIntoBlockUpdateTest.php
@@ -8,9 +8,9 @@
+ * Tests the upgrade path for site variables being converted into a block.

'site variables' I think should be 'page variables' as they were really only available at the page level? Could be wrong.

dcrocks’s picture

dcrocks commented
Issue summary: View changes
Related issues: +#2614506: Followup to Use a Branding Block cache fix - 2005546

Created a 2nd followup, #2614506: Followup to Use a Branding Block cache fix - 2005546 and cleaned some minor IS problems.

dcrocks’s picture

dcrocks commented
StatusFileSize
new Followup_to_Use_a_Branding_Block_docs-2567077-12.patch728 bytes
new interdiff-Followup_to_Use_a_Branding_Block_docs-2567077-9-12.txt657 bytes

How about both.

ps. @alexpott Do you know where the cache line was fixed or the @file doc fix was done?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned

I don't think I'm necessary here.

dcrocks’s picture

dcrocks commented

This is do trivial. Could someone RTBC it?

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing, -rc target triage +rc eligible, +Documentation

Yup thanks for the bump.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue tags: -rc eligible

RTBC++

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/system/src/Tests/Update/SiteBrandingConvertedIntoBlockUpdateTest.php
@@ -8,9 +8,9 @@
  *
- * @see https://www.drupal.org/node/507488
+ * @see https://www.drupal.org/node/2005546

Let's actually just remove this. We don't need to link the issue that added code since that can be discovered via git blame. If there is some specific information from the issue that is relevant we should include it in the docs, but I don't think that's the case here.

Thanks!

dcrocks’s picture

dcrocks commented
Status: Needs work » Needs review
StatusFileSize
new Followup_to_Use_a_Branding_Block_docs-2567077-18.patch683 bytes
new interdiff-Followup_to_Use_a_Branding_Block_docs-2567077-12-18.txt642 bytes

Made change.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Reviewed & tested by the community

Good call, thanks @xjm and @dcrocks

alexpott’s picture

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

Committed ff12935 and pushed to 80.x and 8.1.x. Thanks!

  • alexpott committed ff12935 on 8.1.x 
    Issue #2567077 by dcrocks, joelpittet, xjm: Followup to Use a Branding...

  • alexpott committed aa42bd4 on 
    Issue #2567077 by dcrocks, joelpittet, xjm: Followup to Use a Branding...

Status: Fixed » Closed (fixed)

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