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

Make possible to set NULL values using config:set command #15216

Closed
wants to merge 1 commit into from

Conversation

jalogut
Copy link
Contributor

@jalogut jalogut commented May 15, 2018

Description

Currently is not possible to set null values using config:set command. That is needed, if we want to overwrite core_config_data values using config.php and env.php files.

This PR makes that possible by replacing "NULL" param value with null before the value is saved into the config files.

Manual testing scenarios

  1. bin/magento config:set <path> NULL
  2. I would expect this value set to null. However, the current status is that "NULL" string is saved instead.

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)

@jalogut jalogut changed the title Make possible to set null values using config:set command Make possible to set NULL values using config:set command May 15, 2018
@hostep
Copy link
Contributor

hostep commented May 15, 2018

What if you want to use the actual word "NULL", that won't be possible anymore? (I realize almost nobody will want to do this, but still ...)

In the n98/magerun2 project, they fixed it with an extra flag --no-null, which change the interpretation when you provide NULL, either the value null or the word "NULL", maybe that's an idea which you can use over here as well?
See: https://github.com/netz98/n98-magerun2/blob/fb75eb2/src/N98/Magento/Command/Config/Store/SetCommand.php#L77-L86

@jalogut
Copy link
Contributor Author

jalogut commented May 15, 2018

Yes, I know about the flag on n98. It’s just that I do not think anybody will need “NULL” as a string. I also think that it should be avoided, so I did not want to provide this option. If you think that it is needed, I wouldn’t have any problem to add it.

@hostep
Copy link
Contributor

hostep commented May 15, 2018

I also believe almost nobody will need "NULL" as a string, but still, one day, someone will probably need it for some obscure reason.
Let's wait for someone from the Magento team to give some feedback here to see if the extra flag is needed or not.

@orlangur orlangur self-assigned this Jul 11, 2018
Copy link
Contributor

@orlangur orlangur left a comment

Choose a reason for hiding this comment

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

I don't like that we should use capsed NULL here and that string behavior is changed.

Can we have a separate command like bin/magento config:unset <path> or bin/magento config:set-null <path> maybe?

@orlangur
Copy link
Contributor

orlangur commented Aug 2, 2018

Closing due to inactivity. Feel free to reach me out anytime later if you wish to continue work on this pull request and it will be reopened.

@orlangur orlangur closed this Aug 2, 2018
@jalogut
Copy link
Contributor Author

jalogut commented Aug 11, 2018

Hi @orlangur

I’d rather go for the option —no-null as @hostep mentioned. Adding a especific command just for null values doesn't seem like the right solution for me. What do you think?

If you use config propagation, you need to assign NULL value to unset config settings. With config propagation files, you configure values but not different actions depending on the value.

@jalogut jalogut reopened this Aug 11, 2018
@joshuaadickerson
Copy link

unset seems best. Makes sure nobody will mistake it.

@orlangur
Copy link
Contributor

orlangur commented Sep 3, 2018

Raised in Slack.

@orlangur
Copy link
Contributor

Hi @jalogut, among maintainers the preferred version is bin/magento config:unset <path>, as this is a new feature please create a PR against 2.3-develop.

@orlangur orlangur closed this Oct 27, 2018
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.

5 participants