liuch / dmarc-srg

A php parser, viewer and summary report generator for incoming DMARC reports.
GNU General Public License v3.0
218 stars 32 forks source link

Fix optional fields #35

Closed smeinecke closed 1 year ago

smeinecke commented 1 year ago

Add check if optional fields are defined, PHP reported warning if undefined.

liuch commented 1 year ago

According to DMARC XML Schema, only these fields from your patch are optional: extra_contact_info, adkim, aspf. And... Have you seen the ReportData class? All default values are defined there. If it doesn't work, it should be fixed there.

smeinecke commented 1 year ago

According to DMARC XML Schema, only these fields from your patch are optional: extra_contact_info, adkim, aspf.

I just copied your definition in Report.php:

            'domain'             => [ 'required' => true,  'type' => 'string' ],
            'begin_time'         => [ 'required' => true,  'type' => 'object' ],
            'end_time'           => [ 'required' => true,  'type' => 'object' ],
            'org'                => [ 'required' => true,  'type' => 'string' ],
            'external_id'        => [ 'required' => true,  'type' => 'string' ],
            'email'              => [ 'required' => false, 'type' => 'string' ],
            'extra_contact_info' => [ 'required' => false, 'type' => 'string' ],
            'error_string'       => [ 'required' => false, 'type' => 'array'  ],
            'policy_adkim'       => [ 'required' => false, 'type' => 'string' ],
            'policy_aspf'        => [ 'required' => false, 'type' => 'string' ],
            'policy_p'           => [ 'required' => false, 'type' => 'string' ],
            'policy_sp'          => [ 'required' => false, 'type' => 'string' ],
            'policy_pct'         => [ 'required' => false, 'type' => 'string' ],
            'policy_fo'          => [ 'required' => false, 'type' => 'string' ],
            'records'            => [ 'required' => true,  'type' => 'array'  ]

Hm... Why not use $this->data['email'] ?? null instead? It's a little shorter.

I just wasn't sure if the null coalescing operator also works for array keys. But as I just tested it myself, it works fine :+1:; I'll adjust the pull request.

Have you seen the ReportData class? All default values are defined there. If it doesn't work, it should be fixed there.

I just got the PHP Notice in php cron that the optional field "email" is not defined (PHP Notice: Undefined index: email in ..) because it was missing in the XML. Because of this I have adapted the INSERT accordingly. I'll look again why the default was not used in the input of the INSERT.

smeinecke commented 1 year ago

I think i found the problem: https://github.com/liuch/dmarc-srg/blob/master/classes/Report/ReportData.php#L270 'default' => null is missing there ;)

liuch commented 1 year ago

I think i found the problem: https://github.com/liuch/dmarc-srg/blob/master/classes/Report/ReportData.php#L270 'default' => null is missing there ;)

When I wrote that code, I probably was an idealist and believed that all mail servers follow the standards. According to the RFC, this field must be filled.

I just copied your definition in Report.php...

I probably corrected this code later, when I found out that the world is not perfect :)

Thank you so much for your contribution.