-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Move additional dependencies from private getters to constructor - Magento_PageCache #26684
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move additional dependencies from private getters to constructor - Magento_PageCache #26684
Conversation
Hi @Bartlomiejsz. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
746b13f
to
c1a8ead
Compare
$this->_config = $config; | ||
$this->_cache = $cache; | ||
$this->fullPageCache = $fullPageCache; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Bartlomiejsz. We can consider this change as backward incompatible. If some other class inherits the current class in some Magento based project, we will have an error upon calling parent::construct(..)
in a child class because the child class is not supposed to pass $fullPageCache
and $tagResolver
arguments.
What I suggest here is to instantiate the corresponding object if it's not being passed as an argument. See the following instantiation as an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rogyar, I asked similar question in one of my previous PRs, such non-backward compatible changes I'm doing in those PRs are based on info I received in #26269, where I was asked to replace backward-compatible change with one that doesn't use ObjectManager, since we're developing for 2.4 now.
Also such changes are being directly merged to core by Magento core team 4e6f6ae
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Bartlomiejsz. You are right, that's fair since we are editing a non-API class. That's rather a professional habit avoid bringing backward-incompatible changes :)
$this->_config = $config; | ||
$this->_cache = $cache; | ||
$this->fullPageCache = $fullPageCache; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, check my previous comment regarding the backward compatibility
For QA. Looks like we have a couple of fluky functional tests since changes in the current PR are not related to order creation process somehow. |
Hi @rogyar, thank you for the review. |
✔️ QA passed |
…structor - Magento_PageCache #26684
Hi @Bartlomiejsz, thank you for your contribution! |
Description (*)
This PR moves additionally introduced dependencies from private getter methods into constructor in Magento_PageCache module.
Related Pull Requests
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)