inpsyde / wp-app-container

DI Container and related tools to be used at website level.
GNU General Public License v2.0
38 stars 2 forks source link

Fix/registered later providers #7

Closed shvlv closed 3 years ago

shvlv commented 3 years ago
Chrico commented 3 years ago

Shouldn't we maybe integrate https://github.com/inpsyde/wp-context/ here? :)

gmazzap commented 3 years ago

@shvlv Why did you extracted ContextInterface?

As @Chrico said, the idea is that at some point we make use of https://github.com/inpsyde/wp-context/ here and if we have a separate interface that will be harder. Having a class will allow later on to have something like:

namespace Inpsyde\App;
use Inpsyde\WpContext;

/** @deprecated Use Inpsyde\WpContext; instead **/
final class Context extends WpContext {}

And so we can deprecate the context class without breaking backward compatibility (and later on in next major remove docntext definetively).

If you introduce a new interface we can't do that anymore, or at least not that easily. Not ot mention that we would need to deprecate two things not one.

I think the bug fix can be done without adding a new interface, which anyway is something that should never happen in a bugfix release.

shvlv commented 3 years ago

@gmazzap Thanks for your detailed feedback. I agree that a new interface is not bugfix related thing and probably I'll provide new PR after discussion.

  1. The main reason for extracting a new interface was unit testing. I think we should have green master tests before bug fixing. But several tests not passed (example) because you know when Context is instantiated we have false for every context even core. This class is final so we can't create regular mock and have several options:
    • mock overall implementation (provide constants, mock WP functions) - not unit testing way
    • use Reflection or another magic like https://github.com/dg/bypass-finals - very strange
    • provide an interface and create a regular mock. For me, classes with public methods have implicit interface anyway (all public methods) so interface extraction is not breaking anything. I've decided to use this way.
    • after you answer I checked https://github.com/inpsyde/wp-context#testing-code-that-uses-wpcontext. I feel this is strange to provide the public method only for unit testing purposes, isn't it?

What do you think, is it possible to get green master tests before fixing new bugs? Which is the way to take?

  1. I didn't know about plans with https://github.com/inpsyde/wp-context integrations. Sure, it's reasonable. But I don't get why we should deprecate our interface in this case. Following the Law of Demeter, we should create our facade implementation for our interface. For wp-app-container we need the single method in fact (IMO, others can be deprecated).

    interface ContextInterface extends \JsonSerializable
    {
    /**
     * @param string $context
     * @param string ...$contexts
     * @return bool
     */
    public function is(string $context, string ...$contexts): bool;
    }

    Future implementation:

    final class Context implements ContextInterface {
    /**
     * @var Inpsyde\WpContext
     */
    private $wpContext;
    
    public function is(string $context, string ...$contexts): bool
    {
        return $this->wpContext->is($context, ...$contexts);
    }
    }

    This is not only allowing a better unit testing experience but also an open way to provide custom context implementation.

    final class MyCustomContext implements ContextInterface {
    
    public const WOOCOMMERCE = 'woocommerce';
    
    /**
     * @var Inpsyde\App\Context
     */
    private $appContainerContext;
    
    public function is(string $context, string ...$contexts): bool
    {
        if (
            ((self::WOOCOMMERCE == $context) || in_array(self::WOOCOMMERCE, $contexts, true))
            && class_exists('WooCommerce')) {
            return true;
        }
    
        return $this->appContainerContext->is($context, ...$contexts);
    }
    }
shvlv commented 3 years ago

@gmazzap open bugfix only new PR - https://github.com/inpsyde/wp-app-container/pull/8

gmazzap commented 3 years ago

Changes have benn done with #8 and other releases (v2 / v3) no need to keep this open.

Thanks @shvlv