Corrected scope of _construct to protected

[dfelton]

Description

Corrects the scope of _construct() method in Magento\Cron\Model\ResourceModel\Schedule.php.

_construct() is intended to act as a "mock" constructor and not supposed to be accessed outside of the context of $this

Fixed Issues (if relevant)

None.

Manual testing scenarios (*)

No tests performed. However I did perform a scan of the code base to ensure _construct() is never currently called outside of the context of $this.

grep -r \>_construct .
./lib/internal/Magento/Framework/View/Element/AbstractBlock.php:        $this->_construct();
./lib/internal/Magento/Framework/Model/ResourceModel/Db/Collection/AbstractCollection.php:        $this->_construct();
./lib/internal/Magento/Framework/Model/ResourceModel/AbstractResource.php:        $this->_construct();
./lib/internal/Magento/Framework/Model/AbstractModel.php:        $this->_construct();
./lib/internal/Magento/Framework/Data/Form/AbstractForm.php:        $this->_construct();
./app/code/Magento/Eav/Model/Entity/Collection/AbstractCollection.php:        $this->_construct();

Questions or comments

This should really be a PR into 2.4-develop as this is a non-backwards compatible change. However no such branch exists for me to open a PR to.

While this does not break any functionality within Magento itself, it is potentially possible that developers have (against best practices) performed in their code something similar to the following:

<?php

namespace VendorName\ModuleName\Model\FooBar;

class BadPractice
{
    private $schedule;

    public function __construct(\Magento\Core\Model\Cron\ResourceModel\Schedule $schedule)
    {
        $this->schedule = $schedule;
    }

    public function notSureWhyAnyoneWouldEverDoThis()
    {
        $this->schedule->_construct();
    }

    // ... remaining class contents...
}

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 are green)

Resolved issues:

1. resolves [Issue] Corrected scope of _construct to protected #29643: Corrected scope of _construct to protected

[m2-assistant]

Hi @dfelton. 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 give me test instance - deploy test instance based on PR changes
• @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

[rogyar]

Hi @dfelton. Thank you for your suggestion. From my point of view, we should make sure that by bringing this change we make more improvements than breakdowns.
From the backward compatibility perspective, there's a high chance that some 3rd party implementation overrides the construct method. In that way, this change has a chance to break a lot of existing setups. So we should not do that even in 2.4. Especially considering the fact that the modified class is marked as @api.

[dfelton]

Thank you for your feedback @rogyar. May I please continue the conversation here, under the goal for myself to gain a further understanding.

From my point of view, we should make sure that by bringing this change we make more improvements than breakdowns.

Are you stating that if this scope change were to ever occur in Magento 2.x.x, it should be accommodated with other changes that further improve the application, or that the very act of changing this, should improve the application over cause a breakdown?

If the former, it would seem to me that it would only then ever be "snuck into" a PR that contains improvements unrelated to this line change.

If the latter, I do understand the mentality of "don't fix what isn't broken"... But then it seems to me a simple typo here is now forced to remain in Magento 2.x for the remainder of it's life cycle, until Magento 3.x.

From the backward compatibility perspective, there's a high chance that some 3rd party implementation overrides the construct method. In that way, this change has a chance to break a lot of existing setups. So we should not do that even in 2.4.

I apologize but I fail to see how this scope change would cause backwards incompatible changes for this particular scenario. If a third party implementation has overridden the construct method then:

1. The override of _construct has been given a scope of public.

   • If this is the case, then the override causes the scope to remain public, the same as before the commit change, and does not break third party implementations.

2. The override of _construct has been given a scope of protected or private

   • If this case, then the third party extension already follows a practice where it is never called outside of the context of $this. And this commit does not cause a backwards incompatible changes for such third party implementations.

However, in acknowledgement of your concern, it is possible that third party extensions have implemented before|after|around plugin logic to this method prior to. If this is the case, then yes this change would certainly cause a backwards incompatible change and break these implementations.

The other scenario, would be no override|plugin at all, and the method is simply called outside the context of $this (as in my example from my original comment). This would also be a broken implementation, albeit a strange thing to do as there would be no good reason to call the method from my perspective.

At the end of the day, I don't really feel strongly here one way or the other. If we decide to close out this PR without this change merged that's fine with me. Leaving this as public scope doesn't leave anything broken or anything I just noticed it by chance the other day and knew this is not the intended scope for this method.

Thank you for your time.

[rogyar]

Hi @dfelton. I got your point. Make sense for me.

Let me bring this change to the internal discussion. I will get back to you with the decision.

Thank you!

[ihor-sviziev]

@rogyar is there any updates? Did you finished discussion?
I think it’s good improvement.

[ihor-sviziev]

I'm moving to "to approve" as this is backward incompatible change and it should be approved.
During approval please review following info: #25537 (comment)

[rogyar]

Hi @ihor-sviziev. Thank you for the reminder. Yes, we have previously discussed that point. Approving

[rogyar]

@magento run all tests

[magento-engcom-team]

Hi @rogyar, thank you for the review.
ENGCOM-6998 has been created to process this Pull Request
✳️ @rogyar, could you please add one of the following labels to the Pull Request?

Label	Description
Auto-Tests: Covered	All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered	Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required	Changes in Pull Request does not require coverage by auto-tests

[magento-engcom-team]

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

[engcom-Delta]

✔️ QA passed

[ihor-sviziev]

@ishakhsuvarov, maybe you can push it? it's a pretty tiny fix. I would merge OR decide to just close it

[dfelton]

@ihor-sviziev I have force-pushed to my branch a fresh rebasing from 2.5-develop, deleting the additional commit that was showing as this PR has changed targets over the years.

[engcom-Lima]

Referring to the comment as this PR is already QA passed. Hence, moving this PR to extended testing as builds are failed.

[dfelton]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[engcom-Dash]

Moving this PR to on hold since we are not picking up any PRs against the 2.5-develop branch right now. Once we pick it up, we will move it to the appropriate status.