Skip to content
Snippets Groups Projects

Issue #2853729: update_cron() can run on every cron for easy misconfigure

Open Issue #2853729: update_cron() can run on every cron for easy misconfigure
7 unresolved threads
Open Paul Mitchum requested to merge issue/drupal-2853729:2853729-updatecron-can-run into 10.1.x
7 unresolved threads

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
67 67 public function buildForm(array $form, FormStateInterface $form_state) {
68 68 $config = $this->config('update.settings');
69 69
70 // Adjust existing value to fit our set of choices.
71 $frequency = $config->get('check.interval_days');
72 if ($frequency < 1) {
73 $frequency = 1;
74 }
75 if ($frequency > 1) {
76 $frequency = 7;
77 }
  • Seems like a reasonable approach

  • Paul Mitchum added 26 commits

    added 26 commits

    • f1a8c0c6...88ca0c51 - 24 commits from branch project:10.1.x
    • 52b74031 - Merge branch '10.1.x' into 2853729-updatecron-can-run
    • 191b5aaf - form uses numeric field if frequency value is not 1 or 7

    Compare with previous version

  • Lee Rowlands
  • 13
    14 /**
    15 * {@inheritdoc}
    16 */
    17 protected static $modules = ['update'];
    18
    19 /**
    20 * {@inheritdoc}
    21 */
    22 protected $defaultTheme = 'stark';
    23
    24 /**
    25 * Tests different frequency form elements for different values.
    26 */
    27 public function testFrequencyFormElement() {
    28 $this->drupalLogin(
  • great idea, one question and one suggestion

  • Paul Mitchum added 248 commits

    added 248 commits

    • b2d6c8b7...a8729aad - 246 commits from branch project:10.1.x
    • 4d3d221b - Merge branch '10.1.x' into 2853729-updatecron-can-run
    • 221dcf1e - addressed int type coerce, converted test to kernel test

    Compare with previous version

  • Stephen Mustgrave added 18 commits

    added 18 commits

    Compare with previous version

  • 84 97 '#default_value' => $config->get('check.disabled_extensions'),
    85 98 ];
    86 99
    87 $notification_emails = $config->get('notification.emails');
    100 // Empty config is returned as string, not sequence.
    101 $notification_emails = $config->get('notification.emails') ?? [];
    • Comment on lines -87 to +101

      This is an unnecessary change. It's only because you have a kernel test and you're not doing a proper module install. Call ->installConfig('udpate') in the kernel test to fix this.

    • Please register or sign in to reply
  • 66 66 $requirements['update_core']['reason'] = UpdateFetcherInterface::UNKNOWN;
    67 67 $requirements['update_core']['description'] = _update_no_data();
    68 68 }
    69
    70 // Report configuration error.
    71 $frequency = \Drupal::config('update.settings')->get('check.interval_days');
    72 if ($frequency < 1) {
    73 $requirements['update_core'] = [
    74 'title' => t('Update misconfigured'),
    75 'description' => t('Update module is set to check for updates more frequently than once per day. This results in a check every cron run, which is wasteful. <a href=":freq_config">Modify update frequency</a>.', [
    76 ':freq_config' => Url::fromRoute('update.settings')->toString(),
    77 ]),
    78 'severity' => REQUIREMENT_WARNING,
    79 ];
    80 }
    • Comment on lines +70 to +80

      I think rather than this we should have a hook_post_update_NAME function in the module to fix the configuration. And we should add a constraint to the config schema to ensure that frequency is > 1. This would then allow other config setting tools like drush to have the same validation.

    • Please register or sign in to reply
  • 175 175 function update_cron() {
    176 176 $update_config = \Drupal::config('update.settings');
    177 177 $frequency = $update_config->get('check.interval_days');
    178 if ($frequency < 1) {
    179 \Drupal::logger('update')->notice('Update module frequency set to ' . $frequency . ' days. Acting as if it is set to 1. See: [change record]');
    180 $frequency = 1;
    181 }
  • 25 // Value, expected, form element type.
    26 [0, 1, 'radios'],
    27 [1, 1, 'radios'],
    28 [2, 2, 'number'],
    29 [6, 6, 'number'],
    30 [7, 7, 'radios'],
    31 [8, 8, 'number'],
    32 ];
    33 }
    34
    35 /**
    36 * @dataProvider providerFormData
    37 */
    38 public function testUpdateSettingsForm($config_value, $expected, $form_element_type) {
    39 // Set config.
    40 $config = $this->config('update.settings');
  • 70 // Adjust the frequency so it's never 0.
    71 $frequency = intval($config->get('check.interval_days'));
    72 if ($frequency < 1) {
    73 $frequency = 1;
    74 }
    75 // Use the radio buttons if frequency is daily or weekly.
    70 76 $form['update_check_frequency'] = [
    71 77 '#type' => 'radios',
    72 78 '#title' => $this->t('Check for updates'),
    73 '#default_value' => $config->get('check.interval_days'),
    79 '#default_value' => $frequency,
    74 80 '#options' => [
    75 81 '1' => $this->t('Daily'),
    76 82 '7' => $this->t('Weekly'),
    77 83 ],
    78 84 '#description' => $this->t('Select how frequently you want to automatically check for new releases of your currently installed modules and themes.'),
    Please register or sign in to reply
    Loading