Cron tries to send update notification email while no email is set

🐛 If notifications.email is a string, an error occurs Needs work seems closely-related. What is the connection of 📌 Introduce a new #config_target Form API property to make it super simple to use validation constraints on simple config forms, and adopt it in several core config forms Fixed to this issue?

Thank you for correcting the metadata. We suspect that the changes made for 3382510 📌 Introduce a new #config_target Form API property to make it super simple to use validation constraints on simple config forms, and adopt it in several core config forms Fixed caused this issue.

As suspected, this was probably introduced in 3382510. This MR should resolve two issues:

1. Ensure that an empty values now save to update.settings as {} as they did before
2. Ensure that any sites who's config has been saved as notification: emails: - '' register as an empty value

I've restored the template which should have the proposed resolution section complete. I'll add that I have had issues pushed back by committers for the lack of a proposed resolution.

I have only glanced at the MR and I see that it is on 10.2. Since the diff will be applied to HEAD first this should be on 11.x.

And I do think this will need tests. And based on #7.1 I'd like to know if the tests added 📌 Introduce a new #config_target Form API property to make it super simple to use validation constraints on simple config forms, and adopt it in several core config forms Fixed were sufficient, it seems not if the type a configuration setting could change. But I can't look into that now.

I'm not sure if it makes sense for me to be the one to complete the proposed resolution as I would be working backwards from the work I've already done.

In my opinion, as described on the original issue, blank lines entered into the field get saved to config as blank array elements. IE entering in a bunch of spaces and line breaks results in config like so:

notification: 
  emails: 
    - ''
    - ''
    - ''

This then causes the issue, because these come back as a keyed array of empty values when sending out the notifications. empty() doesn't see this as an empty array.

I believe the solution should take into account that 1. empty lines not be recorded and 2. if all lines are empty, then the config should instead be stored as an empty array / sequence

notification: 
  emails: { }

This resolves the issue going forward, but not for sites that have already exported the incorrect config. I'm not sure if it makes sense to spread a wider net to capture empty values or rather correct the incorrect config.

I can apply the fix to HEAD. It looks like it would be near identical. But I will hold off in case, there is a better solution that's needed here.

As for tests in #3382510, it doesn't look like there were any. The issue was around introducing ConfigTarget(), but how it's been implemented on the Update Settings form is unique to it and the logic defined in it's from and to methods.

MRs need to be created against 11.x first. Thanks!

For anyone else affected by this, a workaround is to use drush to set the relevant variable to an empty value (rather than having the form set it to a list of empty strings):

drush cset update.settings notification.emails []

For the testing part. I've added two tests.

1. The first one I added during the 🐛 Do not log an error message when no emails are specified for update notifications Closed: duplicate , I think it's important to cover how the field is saving the values right now(for more info pleas check this 🐛 Do not log an error message when no emails are specified for update notifications Closed: duplicate comment
2. The second test I did my best to actually replicate the same bug with all the conditions, it should be good enough right now, thanks to @quietone for pointing this up(for more context see this 🐛 Do not log an error message when no emails are specified for update notifications Closed: duplicate comment)

I wasn't able to change the first MR to point to 11.x so I just opened another one with right target. Do you guys know how can I open a branch without the fix to prove that the tests are failing? I've tried once but I wasn't able to dispatch a pipeline on the branch unless I open a MR. Moving back to NR.

The default GitLab CI has a test-only job. You don't have to worry about setting up a separate branch / MR for that.

Thank you @dww! I didn't know, this makes everything much easier, here's the results of the test-only job:

There was 1 failure:
1) Drupal\Tests\update\Functional\UpdateMiscTest::testEmptyEmailListNotification
Failed asserting that an array is empty.
/builds/issue/drupal-3440501/core/modules/update/tests/src/Functional/UpdateMiscTest.php:287
FAILURES!
Tests: 10, Assertions: 104, Failures: 1.
HTML output directory sites/simpletest/browser_output is not a writable directory.
PHPUnit 10.5.26 by Sebastian Bergmann and contributors.
Runtime:       PHP 8.3.9
Configuration: /builds/issue/drupal-3440501/core/phpunit.xml.dist
F                                                                   1 / 1 (100%)
Time: 00:03.023, Memory: 8.00 MB
There was 1 failure:
1) Drupal\Tests\update\Functional\UpdateSettingsFormTest::testUpdateSettingsForm
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
-Array &0 []
+Array &0 [
+    0 => '',
+]
/builds/issue/drupal-3440501/core/modules/update/tests/src/Functional/UpdateSettingsFormTest.php:102

smustgrave → changed the visibility of the branch 3440501-cron-tries-to to hidden.

Hiding the old MR and added N/A to the sections that aren't relevant but can solution be filled in.

Left a nitpicky comment on MR. A

This is great, just ran into this exact problem and the array_filter() was how I was going to solve it too. I committed the @smustgrave suggested changes, though haven't changed the main solution nor the tests which is around the array_filter().

This is reviewed and ready. 🚀

@threeg, anyone can update the proposed resolution to reflect what has been agreed.. Having that up to date is very useful for reviewers and committers to confirm that the implementation matches the community agreement.

I read the IS, comments and MR. I updated the proposed resolution and changed 2 comments in the MR to use email and not e-mail according to our standards, See https://www.drupal.org/drupalorg/style-guide/content → . The MR changes are minor, so leaving at RTBC.

While this make sense I do wonder about sites that don't have an update email address and don't realize it. Maybe they need a reminder?

Does this also fix, 🐛 If notifications.email is a string, an error occurs Needs work ?

Mostly looks great, thanks! This is very close to
ready. However, I opened an MR thread about one of the new tests that I think should be addressed before this is RTBC.

@dww thoughts on the comments on the thread?

Does this also fix, 🐛 If notifications.email is a string, an error occurs Needs work ?

@quietone as far as I could test, this issue would fix this scenario, but it would require to re-save the form again somehow(maybe just a hook update), if a site has exported the configurations with the empty string, the site admin would have to update the configuration again and remove the empty string, and then the next time someone tries to save the form with an empty string, the error would not be happening anymore. I hope this answer your question.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Just rebased the MR, moving back to NR

Hi @murilohp,

I have reviewed your code, and it is working for me on my local setup as well. The mail notification is sent only when the email address is available. I am attaching mail screen shot as well.

https://git.drupalcode.org/issue/drupal-3440501/-/jobs/3073737 appears to show both test changes are failing as expected.
I checked the schema

    notification:
      type: mapping
      label: 'Notification settings'
      mapping:
        emails:
          type: sequence
          label: 'Email addresses to notify when updates are available'
          sequence:
            type: email
            label: 'Email'

And this appears correct for the change to be able to send multiple emails.

All threads appear to be resolved and feedback I believe addressed

LGTM.

Highlighted config snippet in description

I need a static patch. It fixed the issue for me perfectly. Thank you everybody 🙏🏻

I left one minor comment on the MR

But I've got a question about why we're not getting the benefit of config validation here.
Because the setting is configured as a sequence of type email

And email is defined as follows:

email:
  label: 'Email'
  class: '\Drupal\Core\TypedData\Plugin\DataType\Email'
  constraints:
    Email:
      message: "%value is not a valid email address."

i.e. it has a constraint, so what are we missing to have this automatically validate things for us here

The answer might be that we're not making use of the #config_target attribute on the field (which could be because we're doing the translation from strings to array.

For the feedback provided by @larowlan