Skip to content
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

Issue #19609 FIXED #19880

Merged
merged 11 commits into from
Jan 14, 2019
Merged

Conversation

mahesh-rajawat
Copy link
Member

i have preposed a solution to fix #19609 issue, if there is configuration field lock entry available for
default scope then all scopes filed will be readonly.

Description (*)

Fixed Issues (if relevant)

  1. config:set --lock-config does not act on other scopes #19609
  2. ...

Manual testing scenarios (*)

  1. create an entry in app/config.php file using this command:
    php bin/magento config:set --lock-config general/store_information/name "Test lock";
  2. then checked the other scopes, all are disabled now.
  3. Removed the default entry and create a stores scope entry:
    bin/magento config:set --scope=stores --scope-code=india --lock-config general/store_information/name "India"
  4. Field is disabled for this particular scope.

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)

Copy link
Member

@sivaschenko sivaschenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @maheshWebkul721 , thanks for your contribution! Please take a look at the code review comments

@@ -366,9 +366,12 @@ protected function _initElement(

$sharedClass = $this->_getSharedCssClass($field);
$requiresClass = $this->_getRequiresCssClass($field, $fieldPrefix);
$isReadOnly = $this->isDefaultFieldReadOnly($path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to move all the readonly check to a private function

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have fixed this, moved all checks in private function

{
$scope = $this->getScope();
$isReadOnly = false;
if ($scope !== ScopeConfigInterface::SCOPE_TYPE_DEFAULT) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this check is unnecessary, $isReadOnly value assigned inside the condition is correct for default scope

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this check.

$isReadOnly = $this->isDefaultFieldReadOnly($path);
if (!$isReadOnly) {
$isReadOnly = $this->getElementVisibility()->isDisabled($field->getPath())
?: $this->getSettingChecker()->isReadOnly($path, $this->getScope(), $this->getStringScopeCode());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider injecting \Magento\Config\Model\Config\Reader\Source\Deployed\SettingCheckeras \Magento\Theme\Model\Design\Config\DataProvider::getSettingChecker method is deprecated

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sivaschenko do i need to inject \Magento\Config\Model\Config\Reader\Source\Deployed\SettingChecker in constructor?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @maheshWebkul721 , yes, please

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sivaschenko added!

@sivaschenko sivaschenko self-assigned this Jan 3, 2019
@sivaschenko
Copy link
Member

Hi @maheshWebkul721 please add a new dependency in a backward compatible way, please see the Adding a constructor parameter section and code example here: https://devdocs.magento.com/guides/v2.3/contributor-guide/backward-compatible-development/#php

@mahesh-rajawat
Copy link
Member Author

@sivaschenko added constructor parameter as per contribution guide.

@ghost
Copy link

ghost commented Jan 14, 2019

Hi @maheshWebkul721, 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.

@magento-engcom-team magento-engcom-team merged commit 37e0613 into magento:2.3-develop Jan 14, 2019
magento-engcom-team pushed a commit that referenced this pull request Jan 14, 2019
@magento-engcom-team
Copy link
Contributor

Hi @maheshWebkul721. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

config:set --lock-config does not act on other scopes
4 participants