improve update status email subject
7 open threads
Closes #1818764
Merge request reports
Activity
added 126 commits
-
9a719962...82d325ee - 125 commits from branch
project:11.x
- 7998ca80 - Update message and add tests
-
9a719962...82d325ee - 125 commits from branch
- Resolved by quietone
- Resolved by quietone
- Resolved by Stephen Mustgrave
- Resolved by Stephen Mustgrave
- Resolved by quietone
added 1 commit
added 1521 commits
-
7c880d4b...93ec7a93 - 1520 commits from branch
project:11.x
- 05379fea - Merge branch '11.x' into '1818764-correct--improve'
-
7c880d4b...93ec7a93 - 1520 commits from branch
added 211 commits
-
05379fea...10a80a91 - 210 commits from branch
project:11.x
- 8d611611 - Issue # 1818764: Fixed conflicts after rebasing at file:...
-
05379fea...10a80a91 - 210 commits from branch
added 1 commit
- f308ee46 - Issue # 1818764: Fixed some PHPCS and PHPSTAN issues reported by the pipeline jobs
added 1 commit
- d2c43240 - Issue # 1818764: Added missing argument () in the testUpdateEmail() test method
added 5 commits
Toggle commit listadded 1 commit
added 170 commits
-
d945ccc5...a2db325e - 164 commits from branch
project:11.x
- 74cea444 - Update message and add tests
- ce5b582c - Rework the logic for the subject line
- 52e82b95 - Issue #1818764 by xurizaemon, quietone, haydeniv, amitgoyal, smustgrave, xjm,...
- ae964b6a - Issue # 1818764: Fixed some PHPCS and PHPSTAN issues reported by the pipeline jobs
- 00540182 - fix rebase
- 41d150d6 - Issue #1818764: Update subject lines to be 80 characters long to appease the review bot.
Toggle commit list-
d945ccc5...a2db325e - 164 commits from branch
200 200 $langcode = $message['langcode']; 201 201 $language = \Drupal::languageManager()->getLanguage($langcode); 202 $message['subject'] .= $this->t('New release(s) available for @site_name', ['@site_name' => \Drupal::config('system.site')->get('name')], ['langcode' => $langcode]); 202 203 // Set the message subject. 204 $subject = ''; 205 $all_reasons = array_unique(array_values($params)); 206 // If any of the update reasons in $params are security-related, then use 207 // the security subject. 208 $security = array_filter($all_reasons, function ($item) { 209 return in_array($item, [UpdateManagerInterface::NOT_SECURE, UpdateManagerInterface::REVOKED]); 210 }); 211 if ($security) { 212 $subject = $this->t('Security release(s) available for @site_name', [ 213 '@site_name' => \Drupal::config('system.site') 214 ->get('name'), changed this line in version 17 of the diff
213 '@site_name' => \Drupal::config('system.site') 214 ->get('name'), 215 'langcode' => $langcode, 216 ]); 217 } 218 219 // If security updates are not included, check next for fetch failures. If 220 // all the reasons are fetch failures, then use that as the subject. 221 if (empty($subject)) { 222 $fetch = array_filter($all_reasons, function ($item) { 223 return $item > 0; 224 }); 225 if (empty($fetch) && !empty($all_reasons)) { 226 $subject = $this->t('Failed to get release information for @site_name', [ 227 '@site_name' => \Drupal::config('system.site') 228 ->get('name'), changed this line in version 17 of the diff
224 }); 225 if (empty($fetch) && !empty($all_reasons)) { 226 $subject = $this->t('Failed to get release information for @site_name', [ 227 '@site_name' => \Drupal::config('system.site') 228 ->get('name'), 229 'langcode' => $langcode, 230 ]); 231 } 232 } 233 234 // If the subject still has not been set, then there are only non-security 235 // updates. Use the subject for non-security updates. 236 if (empty($subject)) { 237 $subject = $this->t('New release(s) available for @site_name', [ 238 '@site_name' => \Drupal::config('system.site') 239 ->get('name'), changed this line in version 17 of the diff
158 for ($i = 0; $i < count($expected_body); $i++) { 159 is_string($message['body'][$i]) ? $this->assertSame($expected_body[$i], $message['body'][$i]) : $this->assertSame($expected_body[$i], $message['body'][$i]->render()); 149 160 } 150 else { 151 if (empty($params)) { 152 $this->assertSame($expected_body[0], $message['body'][0]); 153 $this->assertSame($expected_body[1], $message['body'][1]->render()); 154 } 155 else { 156 $this->assertSame($expected_body[0], $message['body'][0]->render()); 157 $this->assertSame($expected_body[1], $message['body'][1]); 158 $this->assertSame($expected_body[2], $message['body'][2]); 159 $this->assertSame($expected_body[3], $message['body'][3]->render()); 160 } 161 for ($i = 0; $i < count($expected_body); $i++) { 162 is_string($message['body'][$i]) ? $this->assertSame($expected_body[$i], $message['body'][$i]) : $this->assertSame($expected_body[$i], $message['body'][$i]->render()); - Comment on lines -143 to +162
These ternaries are extremely dense to have on one line and not very readable. It would be better to refactor this somehow. Possibly the ternaries could instead be grouped on earlier lines for local variables, something like:
$message_subject = is_string($message['subject']) ? $message['subject'] : $message['subject']->render();
(and etc. for the other assertions).
Edited by Jess
182 ], 182 "New release(s) available for ", 183 [ 184 "See the available updates page for more information:\nhttps://example.com/admin/reports/updates/settings", 185 'Your site is currently configured to send these emails when any updates are available. To get notified only for security updates, https://example.com/admin/reports/updates.', 186 ], 183 187 ], 184 188 'security' => [ 185 189 'security', 186 190 [], 187 191 FALSE, 192 "New release(s) available for ", 188 193 [ 189 194 "See the available updates page for more information:\nhttps://example.com/admin/reports/updates/settings", 190 'Your site is currently configured to send these emails only when security updates are available. To get notified for any available updates, https://example.com/admin/reports/updates.', 195 "Your site is currently configured to send these emails only when security updates are available. To get notified for any available updates, https://example.com/admin/reports/updates.", 310 ], 311 'all fetch fail' => [ 312 'all', 313 [ 314 'core' => UpdateFetcherInterface::NOT_FETCHED, 315 'contrib' => UpdateFetcherInterface::NOT_FETCHED, 316 ], 317 FALSE, 318 "Failed to get release information for ", 319 [ 320 'There was a problem checking <a href="https://example.com/admin/reports/updates/settings">available updates</a> for Drupal.', 321 'There was a problem checking <a href="https://example.com/admin/reports/updates/settings">available updates</a> for your modules or themes.', 322 "See the available updates page for more information:\nhttps://example.com/admin/reports/updates/settings", 323 "Your site is currently configured to send these emails when any updates are available. To get notified only for security updates, https://example.com/admin/reports/updates.", 324 ], 325 ], - Comment on lines +223 to +325
It would be good to have an inline comment describing the purpose of each provider scenario here and what we expect for each.
Edited by Jess
228 ->get('name'), 229 'langcode' => $langcode, 230 ]); 231 } 232 } 233 234 // If the subject still has not been set, then there are only non-security 235 // updates. Use the subject for non-security updates. 236 if (empty($subject)) { 237 $subject = $this->t('New release(s) available for @site_name', [ 238 '@site_name' => \Drupal::config('system.site') 239 ->get('name'), 240 'langcode' => $langcode, 241 ]); 242 } 243 // Update the existing message. - Edited by Jess
changed this line in version 17 of the diff
added 246 commits
-
b68873fb...6439ca10 - 239 commits from branch
project:11.x
- 1d8d0b03 - Update message and add tests
- c32e0dd0 - Rework the logic for the subject line
- 57fe4048 - Issue #1818764 by xurizaemon, quietone, haydeniv, amitgoyal, smustgrave, xjm,...
- d8782a3a - Issue # 1818764: Fixed some PHPCS and PHPSTAN issues reported by the pipeline jobs
- daf7306f - fix rebase
- 189cbbf6 - Merge branch '11.x' into 1818764-correct--improve
- 06d291b5 - Issue # 1818764 by danrod: Fixed some conflicts
Toggle commit list-
b68873fb...6439ca10 - 239 commits from branch
added 1 commit
- 56131b45 - Issue # 1818764 by danrod: Applied some suggested changes by xjm
Please register or sign in to reply