Problem/Motivation

The System module's mail have 4 property paths that are not yet validatable:

vendor/bin/drush config:inspect --filter-keys=system.mail --detail --list-constraints
➜  🤖 Analyzing…

 Legend for Data: 
  ✅❓  → Correct primitive type, detailed validation impossible.
  ✅✅  → Correct primitive type, passed all validation constraints.
 ---------------------------------------- --------- ------------- ------ ----------------------------------------------------------------------------- 
  Key                                      Status    Validatable   Data   Validation constraints                                                       
 ---------------------------------------- --------- ------------- ------ ----------------------------------------------------------------------------- 
  system.mail                              Correct   67%           ✅❓   ValidKeys: '<infer>'                                                         
                                                                          LangcodeRequiredIfTranslatableValues: null                                   
   system.mail:                            Correct   Validatable   ✅✅   ValidKeys: '<infer>'                                                         
                                                                          LangcodeRequiredIfTranslatableValues: null                                   
   system.mail:_core                       Correct   Validatable   ✅✅   ValidKeys:                                                                   
                                                                            - default_config_hash                                                      
   system.mail:_core.default_config_hash   Correct   Validatable   ✅✅   NotNull: {  }                                                                
                                                                          Regex: '/^[a-zA-Z0-9\-_]+$/'                                                 
                                                                          Length: 43                                                                   
                                                                          ↣ PrimitiveType: {  }                                                        
   system.mail:interface                   Correct   NOT           ✅❓   ⚠️  @todo Add validation constraints here                                    
   system.mail:interface.default           Correct   Validatable   ✅✅   PluginExists:                                                                
                                                                            manager: plugin.manager.mail                                               
                                                                            interface: Drupal\Core\Mail\MailInterface                                  
                                                                          ↣ PrimitiveType: {  }                                                        
   system.mail:mailer_dsn                  Correct   Validatable   ✅✅   ValidKeys: '<infer>'                                                         
   system.mail:mailer_dsn.host             Correct   Validatable   ✅✅   NotBlank:                                                                    
                                                                            message: 'The mailer DSN must contain a host (use "default" by default).'  
                                                                          ↣ PrimitiveType: {  }                                                        
   system.mail:mailer_dsn.options          Correct   NOT           ✅❓   ⚠️  @todo Add validation constraints here                                    
   system.mail:mailer_dsn.password         Correct   NOT           ✅❓   ⚠️  @todo Add validation constraints here                                    
   system.mail:mailer_dsn.port             Correct   Validatable   ✅✅   Range:                                                                       
                                                                            min: 0                                                                     
                                                                            max: 65535                                                                 
                                                                          ↣ PrimitiveType: {  }                                                        
   system.mail:mailer_dsn.scheme           Correct   Validatable   ✅✅   NotBlank:                                                                    
                                                                            message: 'The mailer DSN must contain a scheme.'                           
                                                                          ↣ PrimitiveType: {  }                                                        
   system.mail:mailer_dsn.user             Correct   NOT           ✅❓   ⚠️  @todo Add validation constraints here                                    
 ---------------------------------------- --------- ------------- ------ ----------------------------------------------------------------------------- 

Steps to reproduce

  1. Get a local git clone of Drupal core 11.x.
  2. composer require drupal/config_inspector — or manually install https://www.drupal.org/project/config_inspector/releases/2.1.5 or newer (which supports Drupal 11!)
  3. composer require drush/drush
  4. vendor/bin/drush config:inspect --filter-keys=system.mail --detail --list-constraints

Proposed resolution

  1. Add validation constraints.
  2. Mark FullyValidatable.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3440975

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:

Comments

kunal.sachdev created an issue. See original summary.

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

kunal.sachdev’s picture

Status: Active » Needs review
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Seems like good validation to me.

alexpott’s picture

Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 93682028b4 to 11.x and f12a516b4f to 10.3.x. Thanks!

  • alexpott committed f12a516b on 10.3.x
    Issue #3440975 by kunal.sachdev, phenaproxima: Add validation...

  • alexpott committed 93682028 on 11.x
    Issue #3440975 by kunal.sachdev, phenaproxima: Add validation...
wim leers’s picture

Title: Add validation constraints to system.mail » [revert] Add validation constraints to system.mail
Status: Fixed » Reviewed & tested by the community

I have to disagree with #5. 😅🙈

This marked system.mail as fully validatable, but it did not change:

    interface:
      type: sequence
      label: 'Interfaces'
      sequence:
        type: string
        label: 'Interface'

… meaning literally any string is accepted, whereas AFAICT it must be a valid @Mail plugin ID? See \Drupal\Core\Mail\MailManager::getInstance().

Similarly, mailer_dsn's scheme and host should probably also be validated but currently aren't.

The key-value pairs under mailer_dsn are trickier/more questionable though, but the mail interfaces are a clear omission.

  • alexpott committed a003b4ec on 11.x
    Revert "Issue #3440975 by kunal.sachdev, phenaproxima: Add validation...
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

@Wim Leers great points. Reverted.

  • alexpott committed 124e4d66 on 10.3.x
    Revert "Issue #3440975 by kunal.sachdev, phenaproxima: Add validation...
wim leers’s picture

Title: [revert] Add validation constraints to system.mail » Add validation constraints to system.mail

Thanks 🙏

  • alexpott committed a003b4ec on 11.0.x
    Revert "Issue #3440975 by kunal.sachdev, phenaproxima: Add validation...

  • alexpott committed 93682028 on 11.0.x
    Issue #3440975 by kunal.sachdev, phenaproxima: Add validation...
kunal.sachdev’s picture

For #9 I think we already have a constraint

        constraints:
          PluginExists:
            manager: plugin.manager.mail
            interface: 'Drupal\Core\Mail\MailInterface'

to check if the plugin name is valid.

I also tried to validate the key-value pairs of mailre_dsn mainly scheme and host but those are more tricky. For 'scheme' I tried to add the same PluginExists constraint but with the plugin.manager.mailer_transport plugin manager which is from symfony_mailer module.

kunal.sachdev’s picture

Status: Needs work » Needs review
wim leers’s picture

Status: Needs review » Needs work
Issue tags: +Portland2024

#16: indeed, PluginExists is what you want.

Most importantly: you set this to Needs review but I see no new commits? 😅

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

penyaskito’s picture

Discussed with Wim and we were looking at https://symfony.com/doc/current/reference/constraints/AtLeastOneOf.html, as hostname could be a hostname, a ipv4 or a ipv6. After some pairing we figured out that using composite constraints might be complex and could be deferred to a follow-up, as it's not very common to use a IP for a mail server hostname.

However, back at home just discovered the Symfony "Hostname" validator will consider "default" as an invalid one, and that's our default for e.g. when using sendmail, so we really need to figure this out here.

penyaskito’s picture

Gave it a try and I think I ended up in the same place I ended up on Wim's computer.

Created https://git.drupalcode.org/issue/drupal-3440975/-/merge_requests/1/diffs for not polluting the original MR. I got stuck when in some place we are expecting typed data instead of the underlying value (or the opposite). Might be easier if we just extend from the symfony validators (as Regex does already), but definitely not as clean :-/

znerol’s picture

penyaskito’s picture

@WimLeers this should probably closed as duplicated, but do you think is worth opening another issue for trying to reuse the symfony validators OOTB. specially for those like AtLeastOneOf?

wim leers’s picture

Issue tags: +Needs followup

Nice, #3401255: Tighten config validation schema of system.mail mailer_dsn landed while I was on vacation, so … I agree this can be closed.

I do think it would be superb to create a new issue for adopting AtLeastOneOf! (I lost track of it after pairing with you on getting that to work at DrupalCon Portland, @penyaskito 🙈 — been too occupied with https://wimleers.com/tags/experience-builder.)

Could you open that? 🙏 And perhaps share/describe what the challenges are in adopting that?

znerol’s picture

kunal.sachdev’s picture

I also tried to use AltLeastOneOf constraint in #3441434: Add validation constraints to core.menu.schema.yml and I have mentioned the problems I faced in https://www.drupal.org/project/drupal/issues/3441434#comment-15686742.

quietone’s picture

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

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.

bbrala’s picture

Status: Needs work » Closed (outdated)

Followup was made, and this issue was du-plicated by: #3401255: Tighten config validation schema of system.mail mailer_dsn.

Fixed credits and closing as dupe

xjm’s picture

Status: Closed (outdated) » Closed (duplicate)