Problem/Motivation

          // The description for an update comes from its Doxygen.
          $func = new ReflectionFunction($module . '_update_' . $update);
          $description = str_replace(array("\n", '*', '/'), '', $func->getDocComment());

My reading of this is that if the docblock text contains a slash that'll get zapped. So anything like 'either/or' or 'and/or' will be garbled.

Also, if the docblock has more than one line, we'll get too much whitespace in between the lines, as only the '*' is removed and there'll be at least one space either side of it.

Steps to reproduce

See #1.

Proposed resolution

Use regular expressions to modify the comment instead of a simple str_replace.

Remaining tasks

Review
Commit

User interface changes

Displayed update hook text can contain slashes and less extra spaces.

CommentFileSizeAuthor
#18 1624278-18.patch6.49 KBquietone
#18 interdiff-14-18.txt4.15 KBquietone
#14 1624278-14.patch6.59 KBquietone
#14 interdiff-10-12.txt1.63 KBquietone
#10 1624278-10.patch4.59 KBquietone
#10 1624278-10-fail.patch3.59 KBquietone
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim’s picture

joachim CreditAttribution: joachim commented

Quick command line script to demonstrate:

<?php

/**
 * This updates foo to foo and/or bar.
 *
 * Line two is long and then wraps it wraps it warps it wraps it warps it wraps
 * round to three.
 */
function foo() {

}

$func = new ReflectionFunction('foo');
$description = str_replace(array("\n", '*', '/'), '', $func->getDocComment());

print $description;

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Version: 8.9.x-dev » 9.2.x-dev
Issue summary: View changes
Status: Active » Needs review
FileSize
1624278-10-fail.patch3.59 KB
1624278-10.patch4.59 KB

A possible solution.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Issue tags: +Bug Smash Initiative

The last submitted patch, 10: 1624278-10-fail.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 10: 1624278-10.patch, failed testing. View results

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff-10-12.txt1.63 KB
1624278-14.patch6.59 KB

Fixing the failing tests, just needed to remove a space from the string to test.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Issue summary: View changes

Ready for review

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented 
  1. +++ b/core/includes/update.inc
@@ -397,7 +397,15 @@ function update_get_update_list() {
+          $patterns = [];
+          $patterns[0] = '/^\s*[\/*]*/';
+          $patterns[1] = '/\n\s*\**/';
+          $patterns[2] = '/\/$/';
+          $replacements = [];
+          $replacements[0] = '';
+          $replacements[1] = '';
+          $replacements[2] = '';
+          $description = preg_replace($patterns, $replacements, $func->getDocComment());

    This is quite longwinded, we could just say:

    $patterns = ['/^\s*[\/*]*/', '/\n\s*\**/', '/\/$/'];
$description = preg_replace($patterns, '', $func->getDocComment());

  2. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateDescriptionTest.php
@@ -0,0 +1,71 @@
+    $session->responseContains('8001 -  Update test of slash in description and/or.');

    Why do we still have two spaces after the dash? Is it out of scope to tidy this up completely so it is just "8001 - Description"?

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented 
+++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateDescriptionTest.php
@@ -0,0 +1,71 @@
+    $session = $this->assertSession();

Usually we don't extract this to a variable and just call assertSession() each time.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
FileSize
interdiff-14-18.txt4.15 KB
1624278-18.patch6.49 KB

#16 1,2 Changed format and modified to remove the extra space.
#17 - Ah, that is how it is done in the migrate_drupal_ui tests so just force of habit. Fixed.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

Thanks, this looks great now!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: 1624278-18.patch, failed testing. View results

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented

Please do not ask the testbot to try again until #3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
Status: Needs work » Reviewed & tested by the community

#3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest was committed. Ordered retest and put this issue back to RTBC per #19.

  • larowlan committed fe06cae on 9.2.x 
    Issue #1624278 by quietone, longwave: cleanup of docblock to UI text in...

  • larowlan committed be53a26 on 9.1.x 
    Issue #1624278 by quietone, longwave: cleanup of docblock to UI text in...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs followup

Such a shame we need a BrowserTest to test this.
Can we get a follow up task to make update_get_update_list() unit testable?

Committed fe06cae and pushed to 9.2.x. Thanks!

Backported this to 9.1.x because the risk of disruption is low.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

Followup created, #3210502: Convert UpdateDescriptionTest to a kernel test

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Issue tags: -Needs followup

Looks like I forgot to remove a tag.