Awesome, thanks @acobster! Things are looking good on the testing side. There are some failures (not in PHPUnit, but static analysis). Most of them are in code not connected to this PR — but there's one we should def. take a look at ...

 ------ -------------------------------------------------------------------- 
  Line   Term.php                                                            
 ------ -------------------------------------------------------------------- 
  229    Static method Timber\Timber::get_term() invoked with 2 parameters,  
         0-1 required.        ```

Codecov Report

❗ No coverage uploaded for pull request base (2.x-factories@7d38ef1). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             2.x-factories    #2203   +/-   ##
================================================
  Coverage                 ?   91.28%           
  Complexity               ?     1626           
================================================
  Files                    ?       53           
  Lines                    ?     4097           
  Branches                 ?        0           
================================================
  Hits                     ?     3740           
  Misses                   ?      357           
  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 7d38ef1...da49967. Read the comment docs.

@jarednova thanks for that - fixed. I'll make a note to start running the analyzer before making PRs, too.

@szepeviktor not directly related to this PR, but some of my code is getting flagged:

 ------ -------------------------------------------------------
  Line   Comment.php                                           
 ------ -------------------------------------------------------
  123    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.
 ------ -------------------------------------------------------

what about new static() is unsafe? We need to use late static binding here, or else class maps simply won't work.

new static() is unsafe when the class is not final.
@ondrejmirtes is very good at explaining this.

Someone can extend from your class and override the constructor with different arguments, so that your new static call will crash. See: https://3v4l.org/7JlG4

You have these options:

1. Use new self() - you might realize you don't need new static().
2. Make your constructor final.
3. Make your class final at which point you can also use new self() :)
4. Create an abstract method createX() that each child class needs to implement with their own new self implementation.

5. Make your constructor part of an implemented interface at which point its signature cannot change.

Thanks @ondrejmirtes and @szepeviktor - making the constructors final seems heavy-handed to me for this case but sound like we have some other good options.

@jarednova this discussion made me realize that I never actually implemented Term::build()...d'oh! So standby for that.

OK @jarednova I'll have an update soon but in the meantime I wanted to get back to this discussion about the new static warning. In short, I believe each of those options in this specific case is untenable for our users and I'm still recommending that we annotate these instances with ignores.

@szepeviktor - please note that this is not about the cost/benefit of syntax checks, coding standards, or static analysis in general. The net benefit of all those things is clear and is taken as a given (tip of the hat to @ondrejmirtes - I'm already liking this tool a lot more that phpcs! 😃). This is about answering the question: what is the cost vs. the benefit for our users of appeasing or ignoring this specific warning?

Here is why I think each option won't work in our particular case:

1. We need late static binding here or else Class Maps simply will not work.
2. Making the constructor final precludes any user in the future doing anything involving a custom constructor. For example, I might want to do this:

class MyPost extends Post {
  protected function __construct($stuff, $things) {
    // do something special with $stuff and $things
  }

  public static function build( WP_Post $wp_post ) {
    [ $stuff, $things ] = SomeApi::special_sauce();
    return new static($stuff, $things);
  }  
}

3. Making any of these classes final would essentially make most of Timber's API useless.
4. I'd be fine with each subclass implementing its own build() method to call new self() if these were purely internal classes. Unfortunately these are classes that are designed to be extended, which means we'd be putting the burden on users to implement something that Factories are already capable of doing, and defeats the purpose of the Factory pattern to begin with.
5. Protected methods, including constructors, cannot be part of an interface by definition. I tried this on the abstract Timber\Core class but abstract classes do not make the same guarantees about the signature.

To sum up, following the letter of this specific error just swaps out a (already very rare) Fatal Error about required arguments with one about final constructors, and moreover limits our users' movement for very little benefit to them.

The most non-disrupting way would probably be to introduce a new interface implemented by the class where the new static code is. The interface would contain public function __construct with the arguments to enforce. This would only break code that already has the bad constructor and already represents broken code today.

I believe each of those options in this specific case

I think a developer has to have some ground rules
which are not evaluated on per project basis.
Many businessmen do this (the per project thinking)

I am looking at things from a 2 years perspective.

a developer has to have some ground rules

Agreed. It seems we only differ on what those ground rules are. Here are some of mine:

• Think from your users' perspective
• Use the right tool for the job
• Identify reasonable exceptions

Think from your users' perspective

You're right.
I'm not able to manufacture a WordPress developer compatible software
as ~30-50% of the source code will be highly abnormal from modern PHP perspective.

Hahaha. The WP core is a rat's nest, no two ways about it!

Sometimes WP is the right tool for the job, and sometimes it isn't.

Thanks everyone for the thoughts and expertise on this issue. I'm really glad we've been able to work out both the guidelines and the exceptions we need to make given the circumstances. @acobster do you have any more commits coming to this branch, or is ready for final review?

@jarednova @gchtr turned out there was quite a bit more work that I'd missed once I made the constructor protected. Should be ready for review now.

@gchtr I was definitely coming at it with PHPUnit-induced tunnel vision. 😆 You're right on with all of this; I'll circle back this week. Thanks for the detailed review!

cc @jarednova

@acobster @gchtr looking good! I second the notes/comments made above. As for ...

public function terms( $query_args = [], $options = [] ) {}  

@jarednova Would this be something you could work on?

Yup! I'll get to work on a PR for that

Update: the PR is #2208

@gchtr @jarednova I think I've addressed all the review feedback except the docs update. Take another look at let me know?

Copy that, added one last test — and presto!