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.

Sign up for GitHub

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

Jump to bottom

Fix #27523: throw informative errors in setup:db:generate-patch #27579

Merged
merged 10 commits into from
Sep 2, 2020
Merged

Fix #27523: throw informative errors in setup:db:generate-patch #27579

merged 10 commits into from
Sep 2, 2020

Conversation

korostii
Copy link
Contributor

@korostii korostii commented Apr 4, 2020 •
edited by ihor-sviziev
Loading

Description (*)

In most cases, if there is indeed a write permission issue, the error does show the complete path to directory which cannot be created:
Screenshot from 2020-04-04 17-21-06
However, if the requested module does not exist, the console command will compile an invalid(incomplete) target path and will show a rather cryptic error message:
Screenshot from 2020-04-04 17-09-33

Fixed Issues (if relevant)

  1. Fixes CLI Patch Generator: Lack of information what path cannot be created #27523: CLI Patch Generator: Lack of information what path cannot be created

Manual testing scenarios (*)

  1. Run php bin/magento setup:db-declaration:generate-patch Vendor_Module SomePatch, while there isn't any registered modules with name matching Vendor_Module
  2. You should see an informative error message like Cannot find a registered module with name "Vendor_Module"

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)

@m2-assistant
Copy link

m2-assistant bot commented Apr 4, 2020

Hi @korostii. 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.4-develop instance - deploy vanilla Magento instance

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

@ghost ghost added this to Pending Review in Pull Requests Dashboard Apr 4, 2020
@rogyar rogyar self-assigned this Apr 4, 2020
@rogyar rogyar self-requested a review April 4, 2020 15:31
@ghost ghost moved this from Pending Review to Changes Requested in Pull Requests Dashboard Apr 4, 2020
@rogyar rogyar added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: bug fix labels Apr 4, 2020
@korostii korostii requested a review from rogyar April 4, 2020 16:06
rogyar
rogyar reviewed Apr 8, 2020
View reviewed changes
app/code/Magento/Developer/Test/Unit/Console/Command/GeneratePatchCommandTest.php
->with('module', 'Vendor_Module')
->willReturn('/long/path/to/Vendor/Module');

$read = $this->createMock(Read::class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @korostii. Thank you for fixing the issues mentioned earlier. There was a comment from me regarding suffixing mock properties with Mock suffix. The same applies to the variables. This suffix improves code readability by letting know straight away what object we are working on mock or a real instance.

Just a recommendation for the future 😉

@ghost ghost moved this from Changes Requested to Ready for Testing in Pull Requests Dashboard Apr 8, 2020
@magento-engcom-team
Copy link
Contributor

Hi @rogyar, thank you for the review.
ENGCOM-7280 has been created to process this Pull Request

@engcom-Alfa engcom-Alfa self-assigned this Apr 9, 2020
@engcom-Alfa engcom-Alfa moved this from Ready for Testing to Testing in Progress in Pull Requests Dashboard Apr 9, 2020
@korostii korostii requested a review from slavvka July 9, 2020 20:03
@engcom-Kilo
Copy link
Contributor

@magento run all tests

@engcom-Kilo engcom-Kilo requested a review from rogyar August 5, 2020 08:09
@engcom-Kilo engcom-Kilo moved this from Changes Requested to Review in Progress in Pull Requests Dashboard Aug 5, 2020
@sidolov sidolov removed this from the 2.4.1 milestone Aug 20, 2020
@ghost ghost moved this from Review in Progress to Ready for Testing in Pull Requests Dashboard Aug 27, 2020
@ghost ghost assigned sidolov Aug 27, 2020
@magento-engcom-team
Copy link
Contributor

Hi @sidolov, thank you for the review.
ENGCOM-7280 has been created to process this Pull Request

@engcom-Alfa engcom-Alfa moved this from Ready for Testing to Testing in Progress in Pull Requests Dashboard Aug 31, 2020
@engcom-Alfa
Copy link
Contributor

✔️ QA Passed

Run: php bin/magento setup:db-declaration:generate-patch Vendor_Module SomePatch

Before:
Screenshot from 2020-04-09 14-33-36

After:
Screenshot from 2020-04-09 14-45-45

@engcom-Alfa engcom-Alfa moved this from Testing in Progress to Extended Testing (optional) in Pull Requests Dashboard Aug 31, 2020
@engcom-Charlie engcom-Charlie self-assigned this Aug 31, 2020
@engcom-Charlie engcom-Charlie moved this from Extended Testing (optional) to Merge in Progress in Pull Requests Dashboard Aug 31, 2020
@magento-engcom-team magento-engcom-team mentioned this pull request Sep 2, 2020
Closed
magento-engcom-team pushed a commit that referenced this pull request Sep 2, 2020
@sidolov
ENGCOM-7280: Fix #27523: throw informative errors in setup:db:generat…
9a3510b 
…e-patch #27579
@magento-engcom-team magento-engcom-team merged commit b881a6f into magento:2.4-develop Sep 2, 2020
@m2-assistant
Copy link

m2-assistant bot commented Sep 2, 2020

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

@ghost ghost moved this from Merge in Progress to Recently Merged in Pull Requests Dashboard Sep 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sidolov sidolov sidolov approved these changes

@slavvka slavvka Awaiting requested review from slavvka

@rogyar rogyar Awaiting requested review from rogyar

Assignees

@rogyar rogyar

@slavvka slavvka

@sidolov sidolov

@engcom-Alfa engcom-Alfa

@engcom-Charlie engcom-Charlie

@engcom-Delta engcom-Delta

@engcom-Kilo engcom-Kilo

Labels
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: bug fix Award: test coverage Community Insider: Magecom community-insider-contribution Component: Developer Event: Global-Contribution-Day Priority: P3 May be fixed according to the position in the backlog. Progress: accept QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope Release Line: 2.4 Severity: S2 Major restrictions or short-term circumventions are required until a fix is available.
Projects
Pull Requests Dashboard
  
Recently Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CLI Patch Generator: Lack of information what path cannot be created