Detaching this from 2.x-user-factories for now. We can revisit later.

Codecov Report

❗ No coverage uploaded for pull request base (2.x-factories@2dc9794). Click here to learn what that means.
The diff coverage is 88.6%.

@@               Coverage Diff               @@
##             2.x-factories   #2157   +/-   ##
===============================================
  Coverage                 ?   93.7%           
  Complexity               ?    1610           
===============================================
  Files                    ?      54           
  Lines                    ?    4069           
  Branches                 ?       0           
===============================================
  Hits                     ?    3813           
  Misses                   ?     256           
  Partials                 ?       0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2dc9794...9bdb2f8. Read the comment docs.

Oy! That wasn't any fun. But I think it's done?!?!?! Let's see how the tests go. There are a couple of things to review when it comes to new static(); in the constructor(s)

The three static analysis errors are ...

 ------ ------------------------------------------------------- 
  Line   Comment.php                                            
 ------ ------------------------------------------------------- 
  123    Unsafe usage of new static().                          
         💡 Consider making the class or the constructor final.  
 ------ ------------------------------------------------------- 
 ------ ------------------------------------------------------- 
  Line   Integrations/CoAuthorsPlusUser.php                     
 ------ ------------------------------------------------------- 
  14     Unsafe usage of new static().                          
         💡 Consider making the class or the constructor final.  
 ------ ------------------------------------------------------- 
 ------ ------------------------------------------------------- 
  Line   User.php                                               
 ------ ------------------------------------------------------- 
  115    Unsafe usage of new static().                          
         💡 Consider making the class or the constructor final.  
 ------ ------------------------------------------------------- 

I'm a little unsure of the best way to resolve. Given that users shouldn't be extending the __constructor() method should we just make those final @acobster @gchtr?

Not sure I necessarily agree that users shouldn't be extending the constructor. We just don't want constructors being called directly, so that all instantiation happens via Class Maps and funneled through ::build(). It's probably a rare case that you actually need a custom constructor, but I could see some default object state being useful, so if we're going to cut off that possibility completely I'd at least want to know why.

Not really sure what PHPStan's issue with this actually is. I found the commit where they introduced this check, but no justification for it.

cc @szepeviktor

Please see https://3v4l.org/On7K3

So the issue is that type declarations on a subclass's constructor might conflict with the declarations on the parent? I don't think that's an issue here, since our constructor takes no arguments. If people want to override __construct(), they can do so with zero arguments. Otherwise they will find out right away that required constructor args on CPT classes will cause a Fatal Error, and we can document this (rather advanced) use-case somewhere if we deem it necessary. So, to me this seems like a docs problem and not a safety issue. My proposal is to ignore the new static() rule in this case.

Thoughts?

will cause a Fatal Error

PHPStan is here with us to help prevent just that :)

Understood, and it's a great tool for that. :) All I'm saying is that at some point we need to look at the costs and benefits of following its rules to the letter. And in this specific instance - which I argue is outside the scope of the problem that example lays out - I believe the costs outweigh the benefits. We'll never be able to standardize our way out of every fatal error, and even if we prevent this one from happening, without a guide for how to do it the right way, our users will just stumble onto PHP Fatal error: Cannot override final method User::__construct(). Not a lot of benefit there from my perspective, so we might as well just provide a good advanced guide and keep each constructor as-is for flexilibity. So for now I'm standing by my recommendation of annotating these instances to be ignored and adding docs for when and how to override core constructors.

Would love to hear from @jarednova or @gchtr on this too!

PHPStan has a cost that may seem very high.
Although in reality PHPStan's (long-term) benefit greatly exceeds its cost.

I agree in principle, but I'm wondering what you see as being the long-term benefit in this particular case? Are you saying you disagree with my proposal? If so, why?

I run web applications for SaaS teams and daily see that they cannot make a sustainable product without these "basic" tests:

1. syntax check
2. coding standard
3. static analysis (PHPStan Level 4+)

That is my daily observation.