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

phpstan fixes up to level 3 #76

Closed williamdes closed 1 year ago

williamdes commented 1 year ago

Ref: https://github.com/liuch/dmarc-srg/pull/74#issuecomment-1596335453

phpstan is a tool that analyses your PHP code without running it and finds bugs or mistakes

In this PR I think 2 bugs are killed:

This is the errors that I did not fix

 ------ ---------------------------------------------------------------------- 
  Line   classes/Config.php                                                    
 ------ ---------------------------------------------------------------------- 
  83     Variable $data on left side of ?? always exists and is not nullable.  
 ------ ---------------------------------------------------------------------- 

 ------ ---------------------------------------------------------------------------------------------------------------------- 
  Line   classes/Database/Mariadb/ReportLogMapper.php                                                                          
 ------ ---------------------------------------------------------------------------------------------------------------------- 
  63     PHPDoc tag @param for parameter $data with type Liuch\DmarcSrg\Report\Report is incompatible with native type array.  
 ------ ---------------------------------------------------------------------------------------------------------------------- 

 ------ -------------------------------------------------------------- 
  Line   classes/Database/Mariadb/ReportMapper.php                     
 ------ -------------------------------------------------------------- 
  158    Offset 'active' does not exist on array{fqdn: string}.        
  589    Offset 'id' does not exist on array{fqdn: non-falsy-string}.  
 ------ -------------------------------------------------------------- 

 ------ ---------------------------------------------------------------------------------------------------------------------- 
  Line   classes/Database/ReportLogMapperInterface.php                                                                         
 ------ ---------------------------------------------------------------------------------------------------------------------- 
  43     PHPDoc tag @param for parameter $data with type Liuch\DmarcSrg\Report\Report is incompatible with native type array.  
 ------ ---------------------------------------------------------------------------------------------------------------------- 

 ------ --------------------------------------- 
  Line   classes/Mail/MailBody.php              
 ------ --------------------------------------- 
  133    Variable $ctype might not be defined.  
 ------ --------------------------------------- 

 ------ --------------------------------------- 
  Line   classes/Report/ReportFetcher.php       
 ------ --------------------------------------- 
  92     Variable $s_act might not be defined.  
  93     Variable $f_act might not be defined.  
 ------ --------------------------------------- 

 ------ -------------------------------------- 
  Line   classes/Settings/Setting.php          
 ------ -------------------------------------- 
  195    Variable $type might not be defined.  
 ------ -------------------------------------- 

 ------ ------------------------------------------------------------------------------------------------------------------------------------------------ 
  Line   classes/Settings/SettingsList.php                                                                                                               
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------ 
  97     Variable $list might not be defined.                                                                                                            
  102    Variable $list might not be defined.                                                                                                            
  153    Method Liuch\DmarcSrg\Settings\SettingsList::getSettingByName() should return Liuch\DmarcSrg\Settings\Setting but return statement is missing.  
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------ 

 ------ -------------------------------------------------------------------- 
  Line   classes/Statistics.php                                              
 ------ -------------------------------------------------------------------- 
  59     Variable $db on left side of ?? always exists and is not nullable.  
 ------ -------------------------------------------------------------------- 

 ------ -------------------------------------------------------------- 
  Line   files.php                                                     
 ------ -------------------------------------------------------------- 
  50     Variable $up_size in empty() always exists and is not falsy.  
 ------ -------------------------------------------------------------- 

 [ERROR] Found 14 errors                                                                                                
liuch commented 1 year ago

Thanks for the PR. I'm afraid I won't accept it all. For example, I will not accept such corrections:

-     * @param Domain|null $domain Domain for which the information is needed. Null is for all domains.
-     * @param array       $range  Array with two dates
+     * @param \Liuch\DmarcSrg\Domains\Domain|null $domain Domain for which the information is needed. Null is for all domains.
+     * @param array                               $range  Array with two dates
all domains.

These add a lot of spaces without solving any problem.

williamdes commented 1 year ago

Well, it actually solves something your type is wrong, it would need a use to get it right

Do you prefer uses but only useful for documentation or should I add native types too for the use to be technically useful

The phpdoc is technically wrong since the actual documentation depends on the file it's on

Do you understand what I mean?

I can revert back and add some use statements?

liuch commented 1 year ago

When started commenting on my year, I decided to stick to some markup. I chose phpdoc. But I don't want to follow the standard exactly if it hurts the readability of the code. The text goes to the right, which means that I have to use line breaks more often. I use Domain inside the method and see no reason to use \Liuch\DmarcSrg\Domains\Domain in the documentation of this method. Note that I don't mind this fix:

-     * @param PDOStatement $st   PDO Statement to bind to
-     * @param ind          $pos  Start position for binding
-     * @param array        $data Domain data
+     * @param \PDOStatement $st   PDO Statement to bind to
+     * @param int           $pos  Start position for binding
+     * @param array         $data Domain data
liuch commented 1 year ago

If you propose a new PR with my wishes in mind, I will gratefully accept it. Or I can make corrections myself based on the information you provide.

Thank you again for your work!

williamdes commented 1 year ago

You can do the changes on this PR, I am going to sleep for now

But the phpdoc changes are because there is a mistake

You can not do

namespace Foo;

@param Bar
// means Foo\Bar

It's all about the namespace, so I had to use a fully qualified type to go back to the root namespace and get it right

is this better to understand?

So you have another option

namespace Liuch\DmarcSrg\Foo;

use \Liuch\DmarcSrg\Domains\Domain;

@param Domain

Or fully qualify the param annotation

liuch commented 1 year ago

It's my fault. Now I understand what you mean. I thought there was a statement use at the top of these files. Sorry.

williamdes commented 1 year ago

It's my fault. Now I understand what you mean. I thought there was a statement use at the top of these files. Sorry.

That's okay, I will revert the changes and put uses

liuch commented 1 year ago

That's okay, I will revert the changes and put uses

There is no need to put uses. Let it be as it is.

williamdes commented 1 year ago

Should be okay now

Thank you for 01636a1e820ed960aa78753278b69aa5ceaa4b1d 🎉