Fixing direct use of object manager with injected block factory.

[astorm]

Statements by Magento team members have indicated that objects should not be instantiated directly in method code. Instead, new objects should be injected via automatic constructor dependency injection.

The following pull requests swaps a direct instantiation in

app/code/Magento/Cms/Controller/Adminhtml/Block/Delete.php

with an injected factory object. The object manager call itself is replaced with a call to the factory method's create method. This moves this dependency into the controller class's constructor.

This pull requests adds no new functionality, and fixes no new bugs. It brings a single PHP file better into line with Magento's stated coding conventions.

If these changes are not welcome, please indicate that in the ticket responses.

If these tickets are welcome, but there's addition unstated steps/context, please let me know what needs to be addressed.

This is a contribution to the Magento source code repository. My contribution consists of code, files, and text explicitly added to this GitHub pull request by myself. My contribution does not consist of code, files, or text outside of this specific pull request, and nothing said in this pull request or elsewhere will be constituted as expanding the scope of this contribution.

[adragus-inviqa]

Call me a pedant, but:

• code style is a little off
• isn't \Magento\Cms\Api\BlockRepositoryInterface::deleteById() preferred here?
• I'd rather have the parent ctor called first
• returning in a ctor is counter-intuitive
• I don't think there's a need for the Injected Dependency Description comment; just the type is fine

Aka:

public function __construct(
    \Magento\Backend\App\Action\Context $context, 
    \Magento\Framework\Registry $coreRegistry,
    \Magento\Cms\Api\BlockRepositoryInterface $blockRepository
) {
    parent::__construct($context, $coreRegistry);
    $this->blockRepository = $blockRepository;
}

// and then...
$this->blockRepository->deleteById($id);

// ...instead of
$model = $this->_objectManager->create('Magento\Cms\Model\Block');
$model->load($id);
$model->delete();

[astorm]

@adragus-inviqa I love pedants! Are you a part of the core team, or just another civilian?

1. Re: Code style -- how so? i.e. can you point to a specific thing that's off and what it's "on" behavior should be?
2. Re: repository -- that seems like its best tackled in a separate pass. Repositories behave differently than the stock Magento objects, which means attempting this en masse (which is my end goal) would be a massive understanding. Right now the goal is removing direct use of the object manager to aid future refactoring efforts.
3. Parent constructor I'm neutral on -- is there a style guide from Magento Inc. on this, or is that just your preference?
4. Returning the constructor may be counter intuitive, but it's a style convention from other frameworks/language (even though it doesn't do anything) Happy to remove if it's mentioned somewhere in Magento style guide
5. A comment seemed better than no comment -- can wiggle on that if Magento has a stated guideline there.

[Vinai]

Can't help but but in:

1. A comment seemed better than no comment -- can wiggle on that if Magento has a stated guideline there.

If a comment doesn't provide real value I think it's better to have no comment. Otherwise I have to read it and decide whether it is important or not, and that is additional cognitive load. What tends to happen when there are many "trash" comments, my brain just ignores them by default. But even that comes with a cost, as it keeps relevant bits of code further apart.
Unfortunately project coding guidelines tend to disagree with this, and so a ton of meaningless comments tend to accumulate. (I could rant on but I'll shut up now 😃 ).

[orlangur]

What is an earliest version for which constructor signature change is allowed? 2.1 or 3.0?

In particular, I don't see how http://devdocs.magento.com/guides/v2.0/architecture/backward-compatibility.html correlates with changes like https://github.com/magento/magento2/blob/2.1/app/code/Magento/Catalog/Model/Product.php#L2575

[astorm]

@orlangur -- thanks for participating -- are you Magento core -- or just another civilian like the rest of us? Asking so I can exploit your brain and position if it's the former, and only your brain is its the later ;)

Regarding your comments/questions, I'm not a semantic versioning expert -- but one thing to note is the semantic versioning applies to the component/composer version, not the Magento Magento version (2.1, 3.0, etc). The major Magento version is considered a "marketing feature" -- from a technical point of view Magento 2.x means a collection of composer components at a certain version. So the semantic versioning changes for this module would mean the module-cms components version would need to change. At least -- that's my understanding of it.

Re: constructor signature change -- I don't know the details of the policy, and what Magento have talked about in the past can be hard to digest on a practical level, but constructor signatures seem like different things than method signatures. PHP itself doesn't enforce signature parity on class inheritance for constructors. More than that though, per Magento's stated policy of "use dependency injection always", constructor signature changes are the only way to get a new object into a constructor. Also, constructor argument order mostly doesn't matter, since Magento's using the object manager behind the scenes to instantiate things (I say mostly because I've noticed some odd behavior with default values that aren't all at the end).

If method signature changes are a big hairy deal then that means shipped Magento code is going to be very hard to refactor, and very hard to refactor in a way that's consistent with the "don't use the object manager" guidelines. Hopefully that's not the case.

Thoughts/context?

[astorm]

@Vinai Right there with you on meaningless comments -- this pull request is mostly about clarifying what would be involved/needed to fix all those object manager calls. Also, thanks for the feedback on the breaking unit tests question (http://magento.stackexchange.com/questions/117170/magento-2-fixing-call-to-undefined-method-mock-blockfactory-4b440480create) -- was just what I was looking for. (mentioned here in case future googlers end up at this ticket)

[orlangur]

if it's the former, and only your brain is its the later ;)

Yep, brain only. I would ask internally and bring the answer here otherwise :)

More than that though, per Magento's stated policy of "use dependency injection always", constructor signature changes are the only way to get a new object into a constructor

I discovered it is not true in reality:
90f4dec#diff-d31bcdab992b8a290ce2d9ae7ed5bb62
So there is a hacky but more reliable way to not cause fatal errors accidentally by code like https://github.com/magento/magento2/blob/2.1/app/code/Magento/Catalog/Model/Product.php#L2575

Which is much more strict policy than described in http://devdocs.magento.com/guides/v2.0/architecture/backward-compatibility.html which guarantees only @api marked code stability.

[astorm]

@adragus-inviqa implemented your suggested feedback, let me know how that looks to you

[astorm]

Ah, that's interesting and sort of funny @orlangur. You are absolutely correct that, on a technical level, that you can instantiate an object like that. However, it's exactly that sort of code I'm trying to refactor out. I get that it's more convenient to directly use the object manager like that, but even in a helper method it means whatever method calls that helper method has a new object dependency, which complicates testing, and introduces possible instabilities. That's why Magento's coding guidelines are so strongly against direct use of the object manager.

The reason I'm piloting a single "how to remove direct object manager" use with this pull request is I'd like to get the community behind an effort to remove these sorts of things from the core en masse. When Magento's core code doesn't follow their own guidelines, it leads well intentioned folks like you to assume that direct use of the object manager is OK.

[orlangur]

it leads well intentioned folks like you to assume that direct use of the object manager is OK

Nope, I do not assume it's acceptable, but I even met some blog posts which suggested to use \Magento\Framework\App\ObjectManager::getInstance() if you miss God Class Mage. I'm just stating the contradiction between declared and actually used policy which should be eliminated for good.

As to me, \Magento\Framework\App\ObjectManager::getInstance() should be eliminated at all to avoid bad practices, it was a temporary solution, you know.

Let's wait for the clarification in #4749

[orlangur]

See static tests report: https://travis-ci.org/magento/magento2/jobs/133509578

#4726 (comment) seems PSR-2 compatible, PHPDoc is missing for constructor also and for property PHPDoc asterisks should be one space rigther.

Would be nice if you squash all three commits into one and force push as there is only one logical change.

[astorm]

Thanks @orlangur -- looks like I misinterpreted your comments. Sorry about that :) Thanks for pointing out the formatting concerns, just address those, and @Vinai's testing feedback.

[vrann]

@astorm thank you for the contribution, finally we've got to it :) can you please update your branch with the latest develop?