Rule: Keyword `new` should not be used

[jissereitsma]

The new keyword should not be used to instantiate new classes. Constructor DI needs to be used instead.

[jissereitsma]

This was coined by @renttek in https://github.com/extdn/extdn-phpcs/issues/8 I personally do not agree with this, as this would limit non-Magento code too much. However, it is open for debate.

[avstudnitz]

I agree that there are some use cases when new is a good approach. It shouldn't be used for Magento classes though.

[fooman]

It's used heavily for exceptions:

          throw new \Magento\Framework\Exception\LocalizedException(
                __('My Message here')
            );

How about for a data transport object?

    $transport = new \Magento\Framework\DataObject(
        ['test' => 'data']
    );
    $this->eventManager->dispatch(
        'custom_event',
        [
            'transport' => $transport
        ]
    );

[jissereitsma]

Completely agree with @fooman. Theoretically, exceptions and DataObject could be generated through factories as well, that are again injected in the constructor. However, this takes the DI pattern so literally, that it starts to be smelly.

[renttek]

I think the use of new is not bad per se. There are many cases like throwing Exceptions, object creation in Factories or for creating DTOs (like \Magento\Framework\DataObject) where new is absolutely ok. I many other cases the usage of new creates side effects, which are also can make code more complex to test. The question is, can/should this be checked?

[navarr]

Just wanted to add the bit that: Magento architects have said Exceptions should be created with the new keyword. I got in trouble for creating an ExceptionFactory.

[schmengler]

@navarr thank god, there's still some sanity :D I started a neverending argument when replacing a new DataObject with a domain specific object without using a factory.

While I don't agree with "never use new", I see the need for at least a soft rule that encourage dependency injection, especially for service-like classes.

A random thought: Would it make sense to "not use new to create instances of @api classes"?

[schmengler]

We agreed that the spirit of the rule is good, namely to encourage dependency injection, but it may be too strict. We want to try it out for a while, with a low warning level and of course document when and why this makes sense.

[lenaorobei]

Might be helpful https://github.com/magento/marketplace-eqp/blob/master/MEQP2/Sniffs/Classes/ObjectInstantiationSniff.php

[schmengler]

Thank you @lenaorobei, I included and documented the rule in #45