Set view models as shareable by default

[thomas-kl1]

Until now, when a viewModel is set to a block via the layout xml, a new instance is created. But by default, all dependencies of a class are a single instance which is shared.

Description

The purpose of this PR is to allows developers to choose if a view model must be shared or not. By default all view models are shared in order to increase performance (data only needs to be loaded once).

Contribution checklist

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds on Travis CI are green)

[thomas-kl1]

Does anyone can point me a direction to resolve the failed phpunit test?

[slavvka]

Hey @thomas-blackbird. Thank you for the contribution! You modified class but didn't modify its unit test. You need to update it:

1. Add method mock for method _objectManager->get
2. Add additional check when shared attribute is on, off or undefined

[thomas-kl1]

Hi @slavvka, thanks for the advice! I didn't pay attention 🙄
Could you review my changes in unit tests? Not sure if it's the right way to do it.

[slavvka]

@thomas-blackbird I am worrying more about changing the behavior for such objects. Could you investigate deeper if we may have issues with objects that weren't supposed to be shared earlier?

[thomas-kl1]

@slavvka I'm currently investigating the issues cases which can occur whit this change.
Some feature will be broken, eg: here
Suggestion: use attribute shared="false".

I think it's more common in developers mind that the instance should be shared by default (it's actually the case in Magento di). However it can break third part extension..

[thomas-kl1]

Hi @slavvka

I don't think that the attribute "shared" should be false by default. The goal of this PR is to fix, which I consider as, a conception error.

In Magento 2, usage of ViewModel is encouraged, it's purpose is to provide data to the view. The main strength of the view model is that it could be reused in many views, and the data could be shared.
However, the view model is created as many times as it's called in the layout. So you can't manage cache in your instance (with lazy-loading getter) between different views. Today, if you want to improve the performance, you have to declare a new block, which have your viewModels in its signature. So the viewModels are shared. In that way, passing viewModel via the layout is useless.

I guess all developers have in mind that the instances are shared. Indeed, why should it works differently as the DI works in the framework?
We shouldn't sacrifice performance for backward compatibility, the potential breaks are even not that hard to fix.
If it's not set as "true" by default, I'm quite sure that a large part of developers won't use this attribute, and it would be more harmful...

If you really think that backward compatibility should be maintained, this PR won't have any impact on performance.

[slavvka]

@thomas-blackbird Thank you for the reply! I totally agree with you but my concerns are about potential BIC issues all over Magento (CE, EE, B2B, 3rd party extensions) that may occur. Could you please share any statistics on performance improvement that can be achieved with this fix if any? It would be very helpful to making the right decision.

[thomas-kl1]

@slavvka Ok 👍
I'll try to gather data, but I can't guarantee you it fastly

[slavvka]

@thomas-blackbird np. I think it's more important to deliver a correct solution rather a quick one :)

[slavvka]

Hey @thomas-blackbird I am closing this PR. Please feel free to reopen it once you gather additional data

[thomas-kl1]

Hi @slavvka , actually I've no clue how to fetch these information. I still think that the instance should be shared by default.
But as it seems to be designed at first to pass collections trough blocks to template, and regarding the backward compatibility policy, it's fair to make it as false by default.

I'm ready to apply the changes in that way (not shareable by default) and to check the view models which might be shared or not.

[slavvka]

Hey @thomas-blackbird You did't need to change defaults. I discussed that with Magento Architect and we agreed that your improvement is useless if shareable isn't default behavior. So please revert last changes and we will run all the tests to see what happens.

[thomas-kl1]

@slavvka Haha good news 🎉 May I squash the commits too?

[slavvka]

@thomas-blackbird Yep, it would be good :)

[thomas-kl1]

@slavvka done, I've also applied the modification to existing modules.

[slavvka]

@thomas-blackbird Awesome. What principles did you use to mark models as non-shareable?

[thomas-kl1]

@slavvka How should I process? How can I relate the PRs?
Do I just apply changes in layouts or do I apply the modification into the framework too.

[thomas-kl1]

@slavvka Please, could you update?

[slavvka]

Hey @thomas-blackbird we are working on making changes in all related repos and testing Magento. Please be patient.

[thomas-kl1]

@slavvka How should I process? How can I relate the PRs?
Do I just apply changes in layouts or do I apply the modification into the framework too.

Yes I know, my message was for some guidance request to port this PR to the repositories you have mentioned here:

Hey @thomas-blackbird Thank you but you can do the same only in public repos like sample data and msi. If you could make corresponding PRs there I would appreciate that :)

[magento-cicd2]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ nmalevanec
✅ sivaschenko
❌ thomas-kl1

[sidolov]

Hi @thomas-kl1 , please, sign CLA, otherwise, we can't process your pull request

[thomas-kl1]

Hi @sidolov I've already signed the CLA, before and after I've renamed my account, I don't know why it's displayed as failed..

[sivaschenko]

Pending merge of magento/inventory#1954 to MSI

[m2-assistant]

Hi @thomas-kl1, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.