Improving PHP Support for Dependency Injection

Most modern frameworks support Dependency Injection, including Symfony, Zend 2, and of course Magento 2. One of the criticisms of Magento 2 has been the extra code volume it has created compared to Magento 1. (It is the same problem in other frameworks – it is highlighted in Magento 2 because there is a direct comparison to Magento 1 code.) This led me to the question – how could it be reduced?

Constructor injection is my personal preference – I prefer objects being fully initialized by the time the constructor returns. I also prefer objects being immutable where possible – it reduces bugs. Setter injection does not achieve these goals. So I am going to focus on constructor injection only here.

So why is there so much code bloat? Is it worse than other programming languages? Have a look at this example in Magento 2, class AccountManagement. (I purposely picked a particularly ugly class by the way!) Don’t worry about what the code does, just look for the pattern. The class declares properties for all the injected values, with separate /*@var*/ lines to get type hinting. Then it declares @param for each argument (needed because older PHP versions don’t allow scalar type hinting in the function declaration). Then the arguments are all declared in the constructor. Then the constructor body copies each argument to a property. So much repetition!

So ignoring doing a good solid scrub of the code to reduce the argument list, how do other languages cope with the same problem?

  • Java has the same problem. People are just used to it and the IDEs make it pretty quick to add new properties.
  • Scala introduced a new concept. There is a primary constructor for a class, and all constructor arguments are automatically visible throughout the class. So no need to declare properties or copy values across – it’s done for you by magic. One of the design goals of Scala was to reduce verbosity, so it is interesting to see that they considered this as a pattern worth fixing in the language.
  • Hack introduced a language feature to help called constructor argument promotion. What they did in Hack was allow public/protected/private keywords in front of constructor arguments – annotated arguments are then declared as properties and copied automatically for you. So like Scala, but you get to control which arguments turn into properties.

So what would the Hack version look like in PHP? Here is a before and after example.

Before:

class AccountManagement implements AccountManagementInterface
{
…snip…
    /**
     * @var CustomerFactory
     */
    private $customerFactory;
 
    /**
     * @var \Magento\Customer\Api\Data\ValidationResultsInterfaceFactory
     */
    private $validationResultsDataFactory;
 
    /**
     * @var ManagerInterface
     */
    private $eventManager;

    /**
     * @var \Magento\Store\Model\StoreManagerInterface
     */
    private $storeManager;
 
    /**
     * @var Random
     */
    private $mathRandom;
 
    /**
     * @var Validator
     */
    private $validator;
 
    /**
     * @var AddressRepositoryInterface
     */
    private $addressRepository;
 
    /**
     * @var CustomerMetadataInterface
     */
    private $customerMetadataService;
 
    /**
     * @var PsrLogger
     */
    protected $logger;
 
    /**
     * @var Encryptor
     */
    private $encryptor;
 
    /**
     * @var CustomerRegistry
     */
    private $customerRegistry;
 
    /**
     * @var ConfigShare
     */
    private $configShare;
 
    /**
     * @var StringHelper
     */
    private $stringHelper;
 
    /**
     * @var CustomerRepositoryInterface
     */
    private $customerRepository;

    /**
     * @var ScopeConfigInterface
     */

    private $scopeConfig;
    /**
     * @var TransportBuilder
     */
    private $transportBuilder;
 
    /**
     * @var DataObjectProcessor
     */
    protected $dataProcessor;
 
    /**
     * @var \Magento\Framework\Registry
     */
    protected $registry;
 
    /**
     * @var CustomerViewHelper
     */
    protected $customerViewHelper;
 
    /**
     * @var DateTime
     */
    protected $dateTime;
 
    /**
     * @var ObjectFactory
     */
    protected $objectFactory;
 
    /**
     * @var \Magento\Framework\Api\ExtensibleDataObjectConverter
     */
    protected $extensibleDataObjectConverter;
 
    /**
     * @var CustomerModel
     */
    protected $customerModel;
 
    /**
     * @param CustomerFactory $customerFactory
     * @param ManagerInterface $eventManager
     * @param StoreManagerInterface $storeManager
     * @param Random $mathRandom
     * @param Validator $validator
     * @param ValidationResultsInterfaceFactory $validationResultsDataFactory
     * @param AddressRepositoryInterface $addressRepository
     * @param CustomerMetadataInterface $customerMetadataService
     * @param CustomerRegistry $customerRegistry
     * @param PsrLogger $logger
     * @param Encryptor $encryptor
     * @param ConfigShare $configShare
     * @param StringHelper $stringHelper
     * @param CustomerRepositoryInterface $customerRepository
     * @param ScopeConfigInterface $scopeConfig
     * @param TransportBuilder $transportBuilder
     * @param DataObjectProcessor $dataProcessor
     * @param Registry $registry
     * @param CustomerViewHelper $customerViewHelper
     * @param DateTime $dateTime
     * @param CustomerModel $customerModel
     * @param ObjectFactory $objectFactory
     * @param ExtensibleDataObjectConverter $extensibleDataObjectConverter
     *
     * @SuppressWarnings(PHPMD.ExcessiveParameterList)
     */
    public function __construct(
        CustomerFactory $customerFactory,
        ManagerInterface $eventManager,
        StoreManagerInterface $storeManager,
        Random $mathRandom,
        Validator $validator,
        ValidationResultsInterfaceFactory $validationResultsDataFactory,
        AddressRepositoryInterface $addressRepository,
        CustomerMetadataInterface $customerMetadataService,
        CustomerRegistry $customerRegistry,
        PsrLogger $logger,
        Encryptor $encryptor,
        ConfigShare $configShare,
        StringHelper $stringHelper,
        CustomerRepositoryInterface $customerRepository,
        ScopeConfigInterface $scopeConfig,
        TransportBuilder $transportBuilder,
        DataObjectProcessor $dataProcessor,
        Registry $registry,
        CustomerViewHelper $customerViewHelper,
        DateTime $dateTime,
        CustomerModel $customerModel,
        ObjectFactory $objectFactory,
        ExtensibleDataObjectConverter $extensibleDataObjectConverter
    ) {
        $this->customerFactory = $customerFactory;
        $this->eventManager = $eventManager;
        $this->storeManager = $storeManager;
        $this->mathRandom = $mathRandom;
        $this->validator = $validator;
        $this->validationResultsDataFactory = $validationResultsDataFactory;
        $this->addressRepository = $addressRepository;
        $this->customerMetadataService = $customerMetadataService;
        $this->customerRegistry = $customerRegistry;
        $this->logger = $logger;
        $this->encryptor = $encryptor;
        $this->configShare = $configShare;
        $this->stringHelper = $stringHelper;
        $this->customerRepository = $customerRepository;
        $this->scopeConfig = $scopeConfig;
        $this->transportBuilder = $transportBuilder;
        $this->dataProcessor = $dataProcessor;
        $this->registry = $registry;
        $this->customerViewHelper = $customerViewHelper;
        $this->dateTime = $dateTime;
        $this->customerModel = $customerModel;
        $this->objectFactory = $objectFactory;
        $this->extensibleDataObjectConverter = $extensibleDataObjectConverter;
    }

After:

class AccountManagement implements AccountManagementInterface
{
…snip…
    public function __construct(
        private CustomerFactory $customerFactory,
        private ManagerInterface $eventManager,
        private StoreManagerInterface $storeManager,
        private Random $mathRandom,
        private Validator $validator,
        private ValidationResultsInterfaceFactory $validationResultsDataFactory,
        private AddressRepositoryInterface $addressRepository,
        private CustomerMetadataInterface $customerMetadataService,
        private CustomerRegistry $customerRegistry,
        private PsrLogger $logger,
        private Encryptor $encryptor,
        private ConfigShare $configShare,
        private StringHelper $stringHelper,
        private CustomerRepositoryInterface $customerRepository,
        private ScopeConfigInterface $scopeConfig,
        private TransportBuilder $transportBuilder,
        private DataObjectProcessor $dataProcessor,
        private Registry $registry,
        private CustomerViewHelper $customerViewHelper,
        private DateTime $dateTime,
        private CustomerModel $customerModel,
        private ObjectFactory $objectFactory,
        private ExtensibleDataObjectConverter $extensibleDataObjectConverter
    ) {
        // Empty.
    }

As you can see, around 190 lines of code reduces to about 30 lines. Now sure, you can reduce that 190 lines by putting the /*@var*/ on the same line as the properties etc, but as you can see, it still would reduce the volume a lot.

So what do you think? Should PHP copy Scala and Hack and be better at dependency injection than Java?  😉

 

4 comments

  1. Matt Johnson · · Reply

    If supporting a constructor syntax for automatically creating properties in the class was an option, I would definitely appreciate being able to specify which parameters turn into properties automatically and which ones do not. In some situations the long list of parameters may just be getting passed along to the parent constructor of an inherited class, and I may not need them defined as properties in my extended class.

  2. Okay I’ll stick my neck out here. Nothing personal 🙂

    You say “be better at dependency injection than java”. You also say “Java has the same problem. People are just used to it and the IDEs make it pretty quick to add new properties.”.

    Well that point about the IDE isn’t just about the IDE its the whole language. PHP is a weakly typed language that has its base as a rapid development scripting language. It is not an Enterprise level language, and whilst I appreciate what everyone is trying to do with it you need to understand that other languages just do this stuff better, faster, and with much less rope to hang yourself.

    Java is a verbose language, no question. But when I code java in IntelliJ I’m using a combination of the strong typing and IDE prompts to write code, I dont sit there and manually write a heck of a lot of what I’m doing. The boilerplate stuff is done for me in many many instances.

    With the dependency injection I’m definitely there with you on your suggested approach of not having to create/assign the properties but I think at that point the worth of doing is really just from a readability perspective because even PHPStorm supports the auto-creation of the property/assignment once you get it into the constructor method call.

    Where I have more of an issue is in this dogmatic use of dependency injection everywhere. Whilst I fully understand the benefits of DI in having a loosely coupled system etc I personally believe it comes with a penalty of verbose code, navigation difficulties and as such making it harder to debug. What you are doing with DI is exposing ‘code smell’ and trying to push people to use smaller classes, unit test, etc etc. But the downside is that you are now doing enterprise level programming in a language that doesnt support and I would argue in an ecosystem that doesnt have the resources necessary to follow.

    In our Java code we don’t use constructor level DI, and we have never seen issues with this approach. In fact we don’t inject everything (gasp!), we use it when we see a need for it. Sometimes we just call “new”. If I was IBM or Reuters then I would re-assess the approach but even then I personally am not sure I’d sacrifice speed for this.

    As a FYI we use setter level injection with autowiring which is basically like this:

    @Autowired
    protected Foo foo;

    then in the method we can call:

    foo->bar();

    Maybe one day I’ll say ‘we need to move to constructor DI’.

    If I type $this->foo->bar() in PHP then no error will be raised whatsoever. The only way you would see that issue is with a code sniffer or at runtime. The scope for error is immense (as I’ve seen when you add in the underscore inconsistencies).

    We have over 1500 tests so far in ShipperHQ and follow a TDD approach in almost all circumstances, and we have never had an issue with not being able to test. Do we write unit tests for every single class/method – no. Does anyone – no. I’ve worked in teams of 100+ people in the Financial industry and we never wrote unit tests for every single method/class their either and those projects were burning millions per month.

    Whenever you do architecture you need to look at what you are trying to achieve. The magento architecture is forcing principles on us that slow down development and in many cases increase complexity. They also increase the expertise required of the developer, even when I worked in the city finding a developer who is adept in design patterns is very hard to find.

    Having the most loosely coupled code in the world is wonderful, do we need every single class in over 30K of them to be loosely coupled?

    Personally I’d rather Magento focus on getting the interfaces/service contracts in, and in the right place. Ive got an example at present where I am trying to get/set/reset configuration attributes and I’ve got 3 different models that I need in my constructor to support each separate call (where in my mind it should be 1 interface) – surely thats more important than DI’ing the world?

    The big bang approach here also forces a total rewrite of all that everyone has written. You have moved the solution upmarket as only those agencies with sufficient resources will be able to get behind that. The rest will have to hope the community provides them with enough collateral support.

    Let me be clear, Magento 2 has much promise, but you are pushing this enterprise and I’d question if the language or ecosystem can support that.

  3. Me? End a post with a provocative statement to get a response? Never! …. Honestly! … Well maybe sometimes!

    To me the interesting question is the following: GIVEN that dependency injection is becoming more common in PHP applications (irrespective of “enterprise systems design”), SHOULD PHP consider a language change to remove boilerplate code? The focus of this post was not meant to be a PHP vs Java comparison. That might be interesting as a later post!

    But since you went to the effort to make some other comments (thanks!), I will give my person view on these as well, although it is off topic to the real question. (Darn those hook questions!)

    Regarding distraction level to Magento 2 development team, there is zero time being spent on this. If it was released in PHP 7 (it won’t be!) we still could not use it until PHP 7 became the minimum supported version of PHP. This is exploring the field as all the major PHP frameworks support dependency injection – this is not a Magento specific problem. This is wondering “if dependency injection is the trend, can the pain be reduced?”

    Regarding “is PHP enterprise ready”. Well, there are large companies using PHP. Just as some use Ruby on Rails etc. There are more using Java for internal enterprise software, but the whole containerization and microservices makes the language less relevant. Many large sites nowuse microservice architectures, and not “Java enterprise system” architectures. In that world, PHP for front end with faster development cycles (just edit and reload page) makes sense.

    So is Magento and other frameworks adopting more object oriented patterns and modern programming techniques trying to be “enterprise like”? No! That is NOT the goal. The goal is to adopt modern programming language techniques that have proven themselves to be worthwhile. It just happens that enterprise systems have also adopted the same programming patterns. So I can understand people see this as “why are people trying to make PHP an enterprise language”, but I don’t. I see it more “people are trying to apply modern programming techniques to PHP because they help, and PHP is popular”. But obviously that is a personal view.

    Thanks for the comment!

  4. Josh Di Fabio · · Reply

    Good post, Alan.

    This is a real pain-point for me personally. Any scenario in which we could quite easily produce software to write code for us (in this case: defining properties, their doc blocks, and their initialisation inside the constructor) is a big smell in my opinion.

    I’m not a big fan of property scopes being defined in the constructor’s parameter list à la Hack, but any new language feature which allows us to do away with property injection boilerplate would be very welcome indeed!

    Thankfully, there are a number of internals developers working hard to make PHP a better language.

    Regarding scalar type declarations: PHP 7 adds support for them.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: