2.x - 🥺 final __constructor really takes the jelly out of my doughnut

[mindfullsilence]

Please don't do this to the end users (me). I'm in the midst of using v2rc1 and came across this restriction - which seriously limits my ability to provide services to the class unless I write a bunch of setter methods or create an init function that I have to call every time the class is instantiated (both options aren't great). This removes any possibility of DI, autowiring, and I'm sure a lot of other things I can't think of right now.

In addition, the logic for this decision seems flawed: the build() method also shares the same concern you're discussing in pull 2660 about the constructor - it's just not throwing a warning because PHPStan only looks at the constructor for this potential issue. If I override the build() method with a different signature, it will break just as many things as overriding the __constructor() would. Marking both the build and __constructor as final would basically make initializing the class with additional setup code incredibly messy, if not impossible - but it doesn't make sense to mark one without marking the other for this PHPStan warning.

How do you deal with this PHPStan warning, then? Simply don't define a constructor in the Timber\Post class. The library is already structured exactly how it should be to allow for this. By not defining it, you allow it to be defined by child classes, and the PHPStan warning goes away.

To protect against the issue PHPStan is warning about, but for the build method, define an Interface based on what PostFactory needs. Also protect yourself from improperly written child classes by moving some of the import logic around and marking the import() method as final.

// Timber\PostInterface.php
interface PostInterface {
  
    public static function build(WP_Post $post) : PostInterface;

    public function import(WP_Post $post) : void;

}


// in PostFactory.php
class PostFactory {
    //...

    // declare the interface as the return type, rather than the concrete instance
    protected function build(WP_Post $post) : PostInterface
    {
        $class = $this->get_post_class($post);
       
        if ( ! is_a($class, Timber\PostInterface::class) ) {
             throw new Exception("$class must implement Timber\PostInterface");
        }

        $instance = $class::build($post);
        $instance->import($post);

        return $instance;
    }

    //...
}

 // in Post.php

class Post extends CoreEntity implements DatedInterface, Setupable, PostInterface {
    //...

    // new build() method does not call import, since it's now called from PostFactory
    // build() is only responsible for returning an instance
    public static function build(WP_Post $wp_post) : PostInterface {
        $post = new static();

        // No longer call import from here
        // $post->import($data);

        return $post;
    }

    // mark import() as final
    final public function import(WP_Post $wp_post) : void {
        // Move post data resolver logic here - which makes more sense anyway, 
        // and protects `build()` from being overwritten by child classes in a poor way:
        $this->id = $wp_post->ID;
        $this->ID = $wp_post->ID;
        $this->wp_object = $wp_post;

        $data = \get_object_vars($wp_post);
        $data = $this->get_info($data);

        /**
         * Filters the imported post data.
         *
         * Used internally for previews.
         *
         * @since 2.0.0
         * @see   Timber::init()
         * @param array        $data An array of post data to import.
         * @param \Timber\Post $post The Timber post instance.
         */
        $data = \apply_filters('timber/post/import_data', $data, $post);

        parent::import($data);
    }

    //...
}

Now if the user wants to extend and setup their own constructor dependencies - it's easy, and reliable:

// MyPost.php

class MyPost extends Timer\Post implements Timber\PostInterface {
  
  protected ExampleApiService $api;

  // This can now be overridden, and the method signature from PostInterface is enforced
  public static build(WP_Post $wp_post) : Timber\PostInterface {
    return my_di_container()->resolve(self::class, ['post' => $wp_post]);
  }

  // ExampleApiService resolved via a userland DI container via autowiring (for example)
  public function __construct(WP_Post $post, ExampleApiService $api) {
    $this->api = $api;
    //..
  }

}

Pretty pretty please with sugar on top, please don't kill this PHPStan mosquito with a final __constructor() cannon.

[mindfullsilence]

Related issues/PRs

#2792
#2792
#2088

[n3storm]

https://github.com/stevebauman/unfinalize

[gchtr]

Thanks for taking the time to explain the issues with a final constructor, @mindfullsilence. We definitely hear you. We don’t want to restrict advanced uses cases like yours in Timber. It should work both for developers with less and advanced experience.

But I feel that improving this throughout the Timber codebase is too much for the 2.x release. For the 2.x release, I don’t mind removing final and basically revert the changes #2660. We could tackle a proper solution in a future release.

@nlemoine What do you think?

[gchtr]

I talked to @nlemoine this morning and we both agree that it makes sense to revert this change for 2.x and figure out a solution in a future release. PR in #2827.

[nlemoine]

@mindfullsilence Thanks for your detailed issue and solution proposals. I stumbled upon an issue myself with the final keyword myself but found a way around.

I would still add a couple of notes on this.

This removes any possibility of DI, autowiring, and I'm sure a lot of other things I can't think of right now.

Not completely, the class constructor isn't the only way to inject services.

use Psr\Log\LoggerInterface;

class Thing extends Post {
    private LoggerInterface $logger;
    public function setLogger(LoggerInterface $logger) {
        $this->logger = $logger;
    }
}

You could also use a LoggerAware trait. Most containers will support injection through setters.

The Post class is mostly a DTO that holds/wrap WP_Post data. Although nothing prevents you to inject something in it, common practices usually don't inject services into this kind of object (e.g. db entities).

$post = $container->get(Post::class);

This is an uncommon way to grab your models. Can you provide a pseudo code real life example? Just so we can see different use cases of Post in the wild and consider them (or document the way to handle them properly).

[mindfullsilence]

Really appreciate the consideration and action on this one, guys. Totally understand sticking with a revert for now until more consideration is taken into account down the road.

To @nlemoine's point regarding dependency injection through setters, I agree - setters do work. I actually mention this in my OP:

limits my ability to provide services to the class unless I write a bunch of setter methods

Setters are how I ended up working around my problem, but it feels super messy and brittle compared to asking for the dependencies in the constructor itself. Boiling down a simple use-case right now would be tough. However, shortly after my project is complete (expected completion date is Jan 1st 2024), I'll return here to at-minimum provide some more in-depth use cases for you, and hopefully share with you the theming framework I've been working on (if I can convince my bosses to allow me to open-source, which has a good chance of happening).

In essence, it's an orchestrator of Timber, Acorn, and ACF Builder, and I see it as important to the goals of my project to allow the theme developer to utilize the service container and dependency injection capabilities built into Laravel (Acorn provides Laravel functionality to WordPress). One of the major benefits to Laravel's service container is autowiring constructor dependencies without the need to manually register your class with the container. Making that a part of my project is a big goal.

[nlemoine]

Thanks for your feedback @mindfullsilence.

Now that you provided some details about your stack, I'm guessing your problem lied in things like:

$container->make(Timber\Post:class, [
    'post' => $post,
]);

Just like I mentioned before, DTO are usually not using injected services. This is an extended implementation of the ContainerInterface which basically just have get/has. This is of course theoretical and it sometimes doesn't play well with real world use cases. Well, we'll see later how we can make everything work for most users.

In essence, it's an orchestrator of Timber, Acorn, and ACF Builder

Nice! Not so much into Acorn but I'm huge ACF Builder user. I'm currently developing a library based on it that's quite promising if I find the time to finish it.

[mindfullsilence]

I'm currently developing a library based on it that's quite promising if I find the time to finish it.

Sweet! If you ever complete it, I'd be happy to give it a thorough test drive!