Framework Escaper - remove direct Object Manager usage

[maciejslawik]

Description (*)

This PR removes direct ObjectManager calls from \Magento\Framework\Escaper. Additionally, it allows for injecting different implementations of \Psr\Log\LoggerInterface via the constructor.

Manual testing scenarios (*)

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)

[magento-cicd2]

All committers have signed the CLA.

[magento-engcom-team]

Hi @maciejslawik. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento-engcom-team give me test instance - deploy test instance based on PR changes
• @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

[sivaschenko]

Thanks for your contribution @maciejslawik , however adding a constructor is backward incompatible change - it will result in issue on instances where this class is instantiated or extended. Please see https://devdocs.magento.com/guides/v2.0/contributor-guide/backward-compatible-development/

[maciejslawik]

@sivaschenko Alright, I understand.
How about adding this constructor and setting default values for those parameters to null and keeping those 2 getters? This should provide backward compatibility and still give a chance to inject custom implementations.

[orlangur]

Such change is backward-incompatible.

Please inject new dependencies in backward-compatible manner: https://devdocs.magento.com/guides/v2.3/contributor-guide/backward-compatible-development/#adding-a-constructor-parameter

[maciejslawik]

I added null as default to keep all instances that are instantiated directly unbroken.

[orlangur]

@maciejslawik please remove private deprecated methods as you did before and add property initialization into constructor (as in devdoc I mentioned).

[magento-engcom-team]

Hi @sivaschenko, thank you for the review.
ENGCOM-3518 has been created to process this Pull Request

[magento-engcom-team]

@maciejslawik thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

[orlangur]

@maciejslawik please remove private deprecated methods as you did before and add property initialization into constructor (as in devdoc I mentioned).

[maciejslawik]

@orlangur I pushed changes that should reflect your request.

[orlangur]

@maciejslawik great! Changes look good now, please rebase against latest 2.3-develop as there is a conflict.

[maciejslawik]

@orlangur Rebase has been done.

[magento-engcom-team]

Hi @orlangur, thank you for the review.
ENGCOM-3518 has been created to process this Pull Request

[magento-engcom-team]

Hi @maciejslawik. Thank you for your contribution.
We will aim to release these changes as part of 2.3.1.
Please check the release notes for final confirmation.

[sivaschenko]

Hi @maciejslawik unfortunately we had to revert this PR changes because of backward compatibility concerns:
If the Escaper was extended and parent constructor was not called - that will result in escaper and logger properties not been inititalized

[orlangur]

@sivaschenko mentioned use case does not seem valid to me. It means any other similar addition of dependency is backward incompatible.

First of all, we do not recommend inheritance. In the second place, it is not correct to not call parent constructor.

[sivaschenko]

@orlangur in this specific case there is a high risk that if the class was extended the parent constructor was not called as there was no constructor implementation in place.

[orlangur]

@slitvyachenko did somebody really extend this class having some troubles? I believe we should follow https://devdocs.magento.com/guides/v2.3/contributor-guide/backward-compatible-development/ and nothing more than that. If doc is not sufficient, it should be extended with your note - "dependencies MUST NOT be added in case there were none before" or something.

[sivaschenko]

@orlangur I believe that change is falling into the category of "adding public method" rather than adding more dependencies (each class does have a constructor, but in this case, as the constructor was not explicitly defined, it would have been treated as it does not exist).