Problem/Motivation

Follow up from #3201472: Clarify via the field description that the notification email address is used for the reply-to address, not the from address

We don't have a default value for mail_notification in system.site, this feels like it breaks our standard contract for config objects, it should at least be null I think. I want to ask @alexpott about this. It will likely mean a follow-up issue.

Originally caused by #494518: Allow for custom user registration approval email address.

Steps to reproduce

NA

Proposed resolution

Add default value NULL for mail_notification in site.system config

Remaining tasks

Research

User interface changes

NA

API changes

NA

Data model changes

NA

Release notes snippet

TBD

Issue fork drupal-3342489

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

smustgrave created an issue. See original summary.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

_shY made their first commit to this issue’s fork.

_shY opened merge request !5163

_shy’s picture

_shy
Primary language Ukrainian
Location Ukraine, Lutsk 🇺🇦
commented
Status: Active » Needs review

Okay, the tests are passed. Let's see if the changes will fit.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Thanks! But will need an upgrade path for existing sites.

_shy’s picture

_shy
Primary language Ukrainian
Location Ukraine, Lutsk 🇺🇦
commented
Status: Needs work » Needs review

All tests are passed.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Thanks! But think in the update we should check if the value is set first. This current update could undo someones setting. So if the value is set we can leave alone, if not set a default.

_shy’s picture

_shy
Primary language Ukrainian
Location Ukraine, Lutsk 🇺🇦
commented
Status: Needs work » Needs review

Thanks for noticing that. You are totally right @smustgrave, I just missed that thing. That's why I don't like to work at night ;)

Let's check if the tests will pass.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Tests are all green.

_shy’s picture

_shy
Primary language Ukrainian
Location Ukraine, Lutsk 🇺🇦
commented

Hi.
Are more changes needed here?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work
_shy’s picture

_shy
Primary language Ukrainian
Location Ukraine, Lutsk 🇺🇦
commented
Status: Needs work » Needs review

Fixed changes after the last comment. All tests are passed.
But, I don't know why it shows so many changes, I just pulled changes from the 11.x branch.

_shy’s picture

_shy
Primary language Ukrainian
Location Ukraine, Lutsk 🇺🇦
commented

Ok, now it looks better =)

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Feedback and moving to system post_update hook seems to have been addressed.

_shy’s picture

_shy
Primary language Ukrainian
Location Ukraine, Lutsk 🇺🇦
commented

Updated MR.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work
_shy’s picture

_shy
Primary language Ukrainian
Location Ukraine, Lutsk 🇺🇦
commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Fixed the issue summary to not include TBD and NA for proposed solution.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Category: Feature request » Task
Issue summary: View changes
Issue tags: +Configuration schema, +validation
Related issues: +#2379683: Fix configuration schema issues in contact (indirectly user and system) modules, +#494518: Allow for custom user registration approval email address, +#3201472: Clarify via the field description that the notification email address is used for the reply-to address, not the from address

This was previously missed in #2379683: Fix configuration schema issues in contact (indirectly user and system) modules and originally introduced in #494518: Allow for custom user registration approval email address.

I ran into this same problem in #3364108: Configuration schema & required keys.

MR looks great, confirming RTBC 👍

  • longwave committed 4f633dcc on 11.x 
    Issue #3342489 by _shY, smustgrave, longwave, Wim Leers: Add default...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

Committed 4f633dc and pushed to 11.x. Thanks!

Not backportable due to the update hook.

Status: Fixed » Closed (fixed)

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