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

Update section about exceptions #5209

Merged
merged 8 commits into from Aug 23, 2019
Merged

Update section about exceptions #5209

merged 8 commits into from Aug 23, 2019

Conversation

melnikovi
Copy link
Member

@melnikovi melnikovi commented Aug 19, 2019 •
edited by dshevtsov

Purpose of this pull request

Update section about exceptions in technical guidelines.

Affected DevDocs pages

Links to Magento source code

  • ...

whatsnew
Added a technical guideline (clause 5.20) about how to handle exceptions in observers and plugins.

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Aug 19, 2019 •
edited

CLA assistant check
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.
2 out of 3 committers have signed the CLA.

✅ jeff-matthews
✅ dshevtsov
❌ melnikovi

@m2-community-project m2-community-project bot added this to Ready for Review in Pull Request Progress Aug 19, 2019
@devops-devdocs
Copy link
Collaborator

An admin must run tests on this PR before it can be merged.

@dshevtsov dshevtsov self-assigned this Aug 19, 2019
@m2-community-project m2-community-project bot moved this from Ready for Review to Reviewer Approved in Pull Request Progress Aug 19, 2019
dshevtsov
dshevtsov reviewed Aug 19, 2019
View reviewed changes
Copy link
Collaborator

@dshevtsov dshevtsov left a comment •
edited

Choose a reason for hiding this comment

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

Please consider splitting into two statements and avoiding passive voice and nested semantical recursions:

Plugins MUST throw only exceptions declared by the method to which the plugins are added.
Observers MUST throw only exceptions declared by the method that triggers an event.

Discussed with @melnikovi .

erikmarr
erikmarr approved these changes Aug 19, 2019
View reviewed changes
Copy link
Contributor

@erikmarr erikmarr left a comment

Choose a reason for hiding this comment

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

Looks good! Just a minor word order swap.

guides/v2.2/coding-standards/technical-guidelines.md Outdated Show resolved Hide resolved
@melnikovi @erikmarr
Change wording
6e3d94e 
Co-Authored-By: Erik Marr <45772211+erikmarr@users.noreply.github.com>
@melnikovi melnikovi mentioned this pull request Aug 20, 2019
Closed
@jeff-matthews jeff-matthews added 2.2.x 2.3.x Magento 2.3 related changes Internal Dev Differentiates work between community and Magento staff Technical Updates to the code or processes that alter the technical content of the doc labels Aug 22, 2019
@jeff-matthews
Copy link
Contributor

running tests

@dshevtsov dshevtsov added Major Update Significant original updates to existing content and removed Technical Updates to the code or processes that alter the technical content of the doc labels Aug 22, 2019
@dshevtsov
Copy link
Collaborator

dshevtsov commented Aug 22, 2019 •
edited

@jeff-matthews this pull request still requires a review form an architect @maghamed .

@jeff-matthews
Copy link
Contributor

Thanks @dshevtsov. I forgot that all architect reviewers must approve; not just one.

Waiting for your approval @maghamed.

@dshevtsov
Copy link
Collaborator

running tests

@dshevtsov
Copy link
Collaborator

running tests

@dshevtsov
Copy link
Collaborator

running tests

@dshevtsov dshevtsov self-requested a review August 23, 2019 00:05
@dshevtsov dshevtsov merged commit de6f27a into magento:master Aug 23, 2019
@ghost
Copy link

ghost commented Aug 23, 2019

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

@m2-community-project m2-community-project bot moved this from Reviewer Approved to Done in Pull Request Progress Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@buskamuza buskamuza buskamuza approved these changes

@maghamed maghamed maghamed approved these changes

@paliarush paliarush paliarush approved these changes

@dshevtsov dshevtsov dshevtsov approved these changes

@erikmarr erikmarr erikmarr approved these changes

Assignees

@dshevtsov dshevtsov

@erikmarr erikmarr

Labels
2.2.x 2.3.x Magento 2.3 related changes Internal Dev Differentiates work between community and Magento staff Major Update Significant original updates to existing content
Projects
Pull Request Progress
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@melnikovi @magento-cicd2 @devops-devdocs @jeff-matthews @dshevtsov @buskamuza @maghamed @paliarush @erikmarr @skinlikeporcelain