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

Adds module:config:status command which checks if the module config i… #22733

Conversation

hostep
Copy link
Contributor

@hostep hostep commented May 4, 2019

…n the app/etc/config.php file is correct or not.

Description (*)

This adds a new command to bin/magento: module:config:status.
This command checks if the modules in the file app/etc/config.php are correct or not.

I've created this command since I'm getting a bit tired of seeing incorrect app/etc/config.php files being added to our git repo's whenever somebody adds a new module using composer and forgets to run bin/magento setup:upgrade, or when some branch get merged which has merge conflicts in the config.php file and the conflict gets solved incorrectly.
Having this new command, would allow us to have a pre-commit githook which can check for this problem and then tell the developer to fix the config.php file before committing.

Feel free to let me know if the command name or description needs to be changed. And also if the output messages need to be changed.

Fixed Issues (if relevant)

None that I could find.

Manual testing scenarios (*)

  1. Have a correct Magento installation
  2. Run bin/magento module:config:status, it should say: The modules configuration is up to date.
  3. Edit the file app/etc/config.php and remove or add a module. Or just switch the sort order of 2 modules.
  4. Run bin/magento module:config:status, it should say: The modules configuration in the 'app/etc/config.php' file is outdated. Run 'setup:upgrade' to fix it.
  5. Run bin/magento setup:upgrade
  6. Run bin/magento module:config:status, it should say: The modules configuration is up to date.

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)

@m2-assistant
Copy link

m2-assistant bot commented May 4, 2019

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

For more details, please, review the Magento Contributor Assistant documentation

@hostep hostep force-pushed the adds-module-config-status-command branch from 8a3ec61 to 8b9d156 Compare May 6, 2019 19:07
@hostep
Copy link
Contributor Author

hostep commented May 6, 2019

@dmytro-ch: I've force pushed the requested changes.
I changed some other output messages and the description of the command as well, so they are more consistent with each other.

@magento-engcom-team
Copy link
Contributor

Hi @dmytro-ch, thank you for the review.
ENGCOM-5080 has been created to process this Pull Request

@dmytro-ch
Copy link
Contributor

@hostep thank you!

@VasylShvorak VasylShvorak self-assigned this May 10, 2019
@VasylShvorak
Copy link
Contributor

✔️ QA passed

@hostep
Copy link
Contributor Author

hostep commented May 10, 2019

Thanks for the cleanup @nmalevanec! I'm curious which command you ran to detect these problems, so the next time I can try to fix these myself when creating a new PR.

@nmalevanec
Copy link
Contributor

Hi, @hostep. You can just run dev/tests/static/testsuite/Magento/Test/Php/LiveCodeTest.php test against modified php files.

@sivaschenko
Copy link
Member

@piotrekkaminski can you please take a look at this feature and provide your feedback

@sivaschenko
Copy link
Member

@magento run all tests

@hostep
Copy link
Contributor Author

hostep commented Jul 7, 2019

@sivaschenko: is it ok if I still update my PR? There are some missing declare(strict_types=1); lines, it seems like I forgot to add these. And should I add these to new classes only, or also to other classes touched by this PR?

I'll then also do some git rebasing of this PR on top of the latest 2.3-develop, maybe that helps with these tests which seem to be not running somehow.

I also still don't understand how to execute static tests on my local, the dev/tests/static/testsuite/Magento/Test/Php/LiveCodeTest.php file just contains a class and no executable code, so I'm a bit confused about the statement above.

Thanks!

…n the app/etc/config.php file is correct or not.
@hostep hostep force-pushed the adds-module-config-status-command branch from 60af850 to 50e2f31 Compare July 8, 2019 13:10
@hostep
Copy link
Contributor Author

hostep commented Jul 8, 2019

Rebased PR on latest 2.3-develop branch and added declare(strict_types=1); to the 2 new classes.

@sivaschenko
Copy link
Member

@magento run all tests

@hostep hostep force-pushed the adds-module-config-status-command branch from 50e2f31 to d510642 Compare July 10, 2019 18:48
@hostep
Copy link
Contributor Author

hostep commented Jul 10, 2019

Force pushed again, which will hopefully satisfy the static tests (I've amended @nmalevanec's commit for this, as not to loose his authorship of the other fixes).

It would still be great if somebody can tell me how to execute these static tests on my local environment, so I don't have to wait 2 days for the results and can just fix them in one go, thanks! 🙂

@magento-engcom-team magento-engcom-team merged commit d510642 into magento:2.3-develop Jul 11, 2019
@m2-assistant
Copy link

m2-assistant bot commented Jul 11, 2019

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

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.

7 participants