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

Update technical-guidelines.md #3871

Merged
merged 2 commits into from Mar 15, 2019

Conversation

@paliarush
Copy link
Contributor

commented Mar 5, 2019

This PR is a:

  • Content fix or rewrite

Summary

When this pull request is merged, it will...

whatsnew
Updated definition of Exceptions 5.17.

@magento-cicd2

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

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

@akaplya

akaplya approved these changes Mar 5, 2019

@akaplya

akaplya approved these changes Mar 5, 2019

@jcalcaben jcalcaben added this to Backlog in Processing PRs via automation Mar 5, 2019

@dshevtsov dshevtsov self-assigned this Mar 6, 2019

Processing PRs automation moved this from Backlog to Needs testing Mar 6, 2019

@jcalcaben jcalcaben moved this from Needs testing to Needs writer review in Processing PRs Mar 6, 2019

Processing PRs automation moved this from Needs writer review to Needs testing Mar 8, 2019

@@ -496,7 +496,7 @@ class View extends Template
5.16. If a method uses system resources (such as files, sockets, streams, etc.), the code MUST be wrapped with a `try` block and the corresponding `finally` block. In the `finally` sections, all resources SHOULD be properly released.
5.17. `LocalizedException` SHOULD only be thrown in the Presentation layer (Controllers, Blocks).
5.17. Exceptions which need to be displayed to the user MUST be sub-types of `LocalizedException`. Any other types of exceptions MUST be wrapped with `LocalizedException` before being displayed to the user.

This comment has been minimized.

Copy link
@melnikovi

melnikovi Mar 8, 2019

Member

I think we also want to use LocalizedException only in presentation layer, after change it's not clear.

This comment has been minimized.

Copy link
@paliarush

paliarush Mar 15, 2019

Author Contributor

We use it in service contracts, which are directly exposed as web API, what is the alternative proposal for service contracts?

This comment has been minimized.

Copy link
@melnikovi

melnikovi Mar 15, 2019

Member

We can convert service contract exceptions to LocalizedException before displaying to the user. Can we keep somehow part that LocalizedException should be only be thrown in presentation layer.

This comment has been minimized.

Copy link
@paliarush

paliarush Mar 15, 2019

Author Contributor

What will be the conversion rule? We have a hierarchy of LocalizedException classes. Where will the translation be done?

Also this will be backward incompatible change to @api since most service contracts have LocalizedException declared via @throws

@dobooth dobooth assigned dobooth and unassigned dshevtsov Mar 15, 2019

@dobooth

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

running tests

@dobooth dobooth merged commit f5aa20d into magento:master Mar 15, 2019

2 checks passed

Jenkins Tests passed
Details
licence/cla Contributor License Agreement is signed.
Details

Processing PRs automation moved this from Needs testing to Done Mar 15, 2019

@contribution-survey

This comment has been minimized.

Copy link

commented Mar 15, 2019

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

@dshevtsov

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

@dobooth please add whatsnew

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.