Add option "lock-config" for shell command "config:set" #12178

avstudnitz · 2017-11-10T14:39:59Z

This is similar to the "lock" switch which writes configuration
values to app/etc/env.php. The "lock-config" switch writes it to
app/etc/config.php instead which can be shared between environments.
For consistency, the "lock" switch has been renamed to "lock-env".

Description

In most cases, configuration settings should be shared among all development, staging and live instances of a shop. To achieve this, you can use the same mechanism as with the --lock switch described in http://devdocs.magento.com/guides/v2.2/config-guide/cli/config-cli-subcommands-config-mgmt-set.html.

The only difference is that the configuration values will be stored in app/etc/config.php instead of app/etc/env.php. This file should usually be included in the VCS and thus shared between instances.

To store a config setting in the file via command line, the command would be as follows:

bin/magento config:set --lock-config <path> <value>

i.e.

bin/magento config:set --lock-config general/store_information/name "Sample Store"

Manual testing scenarios

Call bin/magento config:set --lock-config general/store_information/name "Sample Store" from the command line
Verify that the app/etc/config.php file contains a section as follows:

[...]
  'cache_types' => 
  array (
    'config' => 1,
    [...]
  ),
  'install' => 
  array (
    'date' => 'Wed, 08 Nov 2017 11:52:21 +0000',
  ),
  'system' => 
  array (
    'default' => 
    array (
      'general' => 
      array (
        'store_information' => 
        array (
          'name' => 'Sample Store',
        ),
      ),
    ),
  ),
);

Verify that the configuration setting "Store Name" in the section "General", group "Store Information" is filled with "Sample Store" and greyed out:

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)

avstudnitz · 2017-11-10T17:19:57Z

I am not totally happy with the naming of the option. Instead of --share I could think of --lock-shared. I think both are okay, but not perfect. I am open for suggestions.

fooman · 2017-11-14T02:56:45Z

Thanks @avstudnitz - read your blog post on this and an addition like this totally makes sense. Agree that naming this is tricky.

I would suggest the following:
--lock-env (which is just an alias for --lock as to not break anything)
--lock-config

avstudnitz · 2017-11-15T10:29:37Z

You can find the blog post about this functionality which was mentioned by @fooman at https://www.integer-net.com/magento-2-2-configuration-via-code/.

jalogut · 2017-11-22T22:01:58Z

Just referencing another PR with improvements in same area:

Add argument on app:config:dump to skip dumping all system settings. #12410

avstudnitz · 2017-11-24T08:45:21Z

I adjusted the PR accordingly (now using Virtual Classes to avoid code duplication) and updated the description. Thanks for the suggestion @fooman.

fooman · 2017-11-25T20:27:39Z

@avstudnitz would you mind taking a look at this failure as well https://travis-ci.org/magento/magento2/jobs/306722542#L1031 which seems related.

avstudnitz · 2017-11-29T13:58:27Z

@fooman Done.

okorshenko · 2017-12-12T09:46:05Z

app/code/Magento/Config/Console/Command/ConfigSet/ProcessorFacade.php

     * @since 100.2.0
     */
-    public function process($path, $value, $scope, $scopeCode, $lock)
+    public function process($path, $value, $scope, $scopeCode, $lock, $lockTarget = ConfigFilePool::APP_ENV)


Unfortunately this backward incompatible change. Please follow the instruction here http://devdocs.magento.com/guides/v2.0/contributor-guide/backward-compatible-development/#adding-parameters-in-public-methods

@okorshenko Can you explain to me why adding an optional parameter at the end should be backwards incompatible? I'd like to understand that.

Hi @avstudnitz this class has annotation @api. Adding new parameter to the method will break SPI clients of this class

Changed. If you have a better name than processWithLockTarget for the new method please tell me.

Old code "I" was not removed in initial commit for some reason.

…dia magento#1007 - Merge Pull Request magento-engcom/magento2ce#1007 from p-bystritsky/magento2:ISSUE-12378 - Merged commits: 1. 5c36f98 2. c27a56a

Added product codes to i18n

These codes have been renamed, so entries removed from i18n

- Merge Pull Request magento#12633 from miguelbalparda/magento2:image-fix - Merged commits: 1. ef1ffd5 2. cef5991

Added quotes around strings in i18n

… attributes. magento#1012

…false</var> for mobile device don't work in product view page gallery magento#1006

@virtual97

Accepted Public Pull Requests: - magento#12736: Issues/12374 (by @virtual97) Fixed GitHub Issues: - magento#12374: Model hasDataChanges always true (reported by @Detzler) has been fixed in magento#12736 by @virtual97 in 2.2-develop branch Related commits: 1. a847d89 2. ae62931

concerns addressed in last commit

fooman · 2017-12-30T05:21:27Z

@avstudnitz Thanks for addressing @okorshenko's feedback. Would you mind taking a look at the mentioned merge conflict as well.

This is similar to the "lock" switch which writes configuration values to app/etc/env.php. The "share" switch writes it to app/etc/config.php instead which can be shared between environments.

…hare option We are using Virtual Classes now as we have almost the same Processor twice and we can avoid having duplicate code this way.

The "getOption" method was called more often than before, so we had to adjust that.

This new method contains the new parameter and will be called from the old method.

…g-set-option-share # Conflicts: # app/code/Magento/Deploy/Model/Mode.php

magento-cicd2 · 2018-01-02T11:54:39Z

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.
8 out of 11 committers have signed the CLA.

✅ gwharton
✅ ishakhsuvarov
✅ p-bystritsky
✅ serhii-balko
✅ okorshenko
✅ sidolov
✅ nmalevanec
✅ RomaKis
❌ AzVo
❌ andimov
❌ duhon

avstudnitz · 2018-01-02T11:59:41Z

@fooman I resolved the merge conflict and rebased the branch. Seems that has some side effects, tell me if I should create a new clean pull request.

okorshenko · 2018-01-08T17:43:35Z

Hi @avstudnitz
Something goes wrong with your branch. Could you please extract your changes and apply them to new branches based on magento/magento2 : 2.2-develop
Please create new PR once ready.
Unfortunately, we can con merge this PR as is

avstudnitz · 2018-02-08T08:00:25Z

Continued in #13280.

fooman self-assigned this Nov 14, 2017

fooman added this to the November 2017 milestone Nov 14, 2017

fooman added the Release Line: 2.2 label Nov 14, 2017

magento-team added the Progress: needs update label Nov 14, 2017

fooman mentioned this pull request Nov 20, 2017

Making possible to define configuration by environment #12361

Closed

4 tasks

andrewhowdencom mentioned this pull request Nov 22, 2017

security issue: env.php must be writeable in production #4955

Closed

jalogut mentioned this pull request Nov 22, 2017

Add argument on app:config:dump to skip dumping all system settings. #12410

Merged

4 tasks

avstudnitz changed the title ~~Add option "share" for shell command "config:set"~~ Add option "lock-config" for shell command "config:set" Nov 24, 2017

jalogut mentioned this pull request Nov 25, 2017

Add command "app:config:status" to check if "app:config:import" needed #12441

Merged

4 tasks

fooman removed the Progress: needs update label Nov 29, 2017

okorshenko modified the milestones: November 2017, December 2017 Dec 1, 2017

fooman added the Progress: accept label Dec 5, 2017

okorshenko self-assigned this Dec 12, 2017

okorshenko previously requested changes Dec 12, 2017

View reviewed changes

gwharton and others added 8 commits December 13, 2017 10:44

Removed old code "I"

30a426b

Old code "I" was not removed in initial commit for some reason.

MAGETWO-85583: magento#12378: Regions list in Directory module for In…

c19467f

…dia magento#1007 - Merge Pull Request magento-engcom/magento2ce#1007 from p-bystritsky/magento2:ISSUE-12378 - Merged commits: 1. 5c36f98 2. c27a56a

Updated i18n

a6abc59

Added product codes to i18n

Removed legacy codes from i18n

b422f2c

These codes have been renamed, so entries removed from i18n

MAGETWO-85499: Magento Connect no longer exist magento#12633

5d39247

- Merge Pull Request magento#12633 from miguelbalparda/magento2:image-fix - Merged commits: 1. ef1ffd5 2. cef5991

Fixed quotes around strings

2ba87c6

Added quotes around strings in i18n

MAGETWO-85579: 10814: Attribute repository resets sourceModel for new…

32acf60

… attributes. magento#1012

MAGETWO-85549: magento#12285: The option <var name="allowfullscreen">…

ba93925

…false</var> for mobile device don't work in product view page gallery magento#1006

fooman added Progress: needs update and removed Progress: accept labels Dec 30, 2017

avstudnitz added 12 commits January 2, 2018 12:45

Add option "share" for shell command "config:set"

f05519b

This is similar to the "lock" switch which writes configuration values to app/etc/env.php. The "share" switch writes it to app/etc/config.php instead which can be shared between environments.

Add unit tests for option "share" of shell command "config:set"

9bc0ac9

Fix unit tests for option "share" of shell command "config:set"

e1b1fbd

12178 Refactor config:set command to use --lock-config instead of --s…

f75fdaf

…hare option We are using Virtual Classes now as we have almost the same Processor twice and we can avoid having duplicate code this way.

12178 Adjust unit tests

0df34b8

12178 Adjust integration tests

896e7a2

12178 Fix integration tests

982f9ce

12178 Fix integration tests

dba706f

The "getOption" method was called more often than before, so we had to adjust that.

12178 Code style optimizations to prevent CodeSniffer errors

77c1d37

12178 More code style optimizations to prevent CodeSniffer errors

01ebf97

7241 Restore old @api method "process" and add a new method

81f3152

This new method contains the new parameter and will be called from the old method.

Merge remote-tracking branch 'avs/config-set-option-share' into confi…

55361fe

…g-set-option-share # Conflicts: # app/code/Magento/Deploy/Model/Mode.php

avstudnitz added 3 commits January 2, 2018 14:20

12178 Fix unit test

26d1798

12178 Fix code style violations

d174998

12178 Add missing newline at EOF

48f416d

okorshenko modified the milestones: December 2017, January 2018 Jan 8, 2018

okorshenko closed this Jan 8, 2018

avstudnitz mentioned this pull request Jan 19, 2018

Add option "lock-config" for shell command "config:set" #13280

Merged

4 tasks

nmalevanec mentioned this pull request Feb 23, 2018

[Forwardport] Add option "lock-config" for shell command "config:set" #13832

Merged

4 tasks

ihor-sviziev mentioned this pull request Apr 23, 2018

[Forwardport] Add argument on app:config:dump to skip dumping all system settings. #14807

Merged

4 tasks

Add option "lock-config" for shell command "config:set" #12178

Add option "lock-config" for shell command "config:set" #12178

Uh oh!

Conversation

avstudnitz commented Nov 10, 2017 • edited by fooman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Manual testing scenarios

Contribution checklist

Uh oh!

avstudnitz commented Nov 10, 2017

Uh oh!

fooman commented Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avstudnitz commented Nov 15, 2017

Uh oh!

jalogut commented Nov 22, 2017

Uh oh!

avstudnitz commented Nov 24, 2017

Uh oh!

fooman commented Nov 25, 2017

Uh oh!

avstudnitz commented Nov 29, 2017

Uh oh!

okorshenko Dec 12, 2017

Choose a reason for hiding this comment

Uh oh!

avstudnitz Dec 12, 2017

Choose a reason for hiding this comment

Uh oh!

okorshenko Dec 12, 2017

Choose a reason for hiding this comment

Uh oh!

avstudnitz Dec 20, 2017

Choose a reason for hiding this comment

Uh oh!

fooman commented Dec 30, 2017

Uh oh!

magento-cicd2 commented Jan 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avstudnitz commented Jan 2, 2018

Uh oh!

okorshenko commented Jan 8, 2018

Uh oh!

avstudnitz commented Feb 8, 2018

Uh oh!

Uh oh!

avstudnitz commented Nov 10, 2017 •

edited by fooman

Loading

fooman commented Nov 14, 2017 •

edited

Loading

magento-cicd2 commented Jan 2, 2018 •

edited

Loading