magento-coding-standard: Exception thrown in single catch block passes

Hi, @lenaorobei @paliarush, but I think these 2 examples should be okay.

Exsample1:

<?php
/**
 * Copyright © Magento, Inc. All rights reserved.
 * See COPYING.txt for license details.
 */

namespace Magento;


class UserRepo
    /**
     * @param int $id
     * @throws \Magento\Framework\Exception\NotFoundException
     */
    public function load(int $id)
    {
        /// some logic
        throw new \Magento\Framework\Exception\NotFoundException('We cant load UserData');
    }
}

class UserViewHelper
{
    /** @var UserRepo */
    private $userRepo;

    /**
     * @param UserRepo $userRepo
     */
    public function __construct(UserRepo $userRepo)
    {
        $this->userRepo = $userRepo;
    }

    /**
     * @param int $id
     * @throws \Magento\Framework\Exception\LocalizedException
     */
    public function execute(int $id)
    {
        try
        {
          $this->userRepo->load($id);
        }catch (\Exception $exception)
        {
            throw new Magento\Framework\Exception\LocalizedException('Nice user friendly message');
        }
    }
}

Exsample2:

<?php
/**
 * Copyright © Magento, Inc. All rights reserved.
 * See COPYING.txt for license details.
 */

namespace Magento;


use Exception;
use Magento\Framework\Exception\LocalizedException;
use NotFoundException;

class UserRepo
{
    /**
     * @param int $id
     * @throws Framework\Exception\NotFoundException
     */
    public function load(int $id)
    {
        /// some logic
        throw new Framework\Exception\NotFoundException('We cant load UserData');
    }
}

class UserViewHelper
{
    /** @var UserRepo */
    private $userRepo;

    /**
     * @param UserRepo $userRepo
     */
    public function __construct(UserRepo $userRepo)
    {
        $this->userRepo = $userRepo;
    }

    /**
     * @param int $id
     * @throws LocalizedException
     */
    public function execute(int $id)
    {
        try {
            $this->userRepo->load($id);
        } catch (NotFoundException $exception) {
            throw new Magento\Framework\Exception\LocalizedException('Nice user friendly message case 1');
        } catch (Exception $exception) {
            throw new Magento\Framework\Exception\LocalizedException('Nice user friendly message case 2');
        }
    }
}

Main Use case you use a Libray like Zend and must wrap a LowerLayerException for security reasons also the library exception are not translated for the frontend than try and catch should be okay in my opinion.

Both examples should fail, but only one fails and it’s false negative. In case if sniff raises warnings when it shouldn’t - it’s false positive.

Can we clarify this rule in the documentation? Should it be: Exceptions must not be thrown from where they are caught? It looks like the sniff is written that way looking at the code in https://github.com/magento/magento-coding-standard/pull/43/files#diff-74e86b8751cc145dc88ad85ae7a8f753R91.

“Exceptions must not be handled in the same function where they are thrown” means, in my mind, that code within catch blocks shouldn’t operate on exception objects whatsoever. Obviously, logging the exception has huge benefit, but also beneficial is notifying parent callers when something went wrong (the “dreaded” throw in question). What is the preferred way to notify parent callers? How is nested try/catch exception delegation supposed to work (and be fixed, for that matter)?

\Magento\Framework\App\Bootstrap::run, the main entrance point for all app requests, violates this rule, as well as hundreds of other methods in Magento (not defending them, just highlighting ubiquity).

This needs some clear documentation (not just 13 words) and steps to fix with example code, as it is slowing down our code deliveries because we run into this sniff being violated almost on a daily basis for existing files we edit and we were told not to ignore them.

Either way, the sniff is a false positive or false negative and needs attention. If it is indeed a false positive, we’ll try to work around it without disabling.

Yep Lena I will take a look how we can solve it

Lena Orobei notifications@github.com schrieb am Fr. 31. Mai 2019 um 16:46:

@larsroettig https://github.com/larsroettig could you please check this isse? Thank you.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/magento/magento-coding-standard/issues/93?email_source=notifications&email_token=ABILLGUPCPIZ6ZDZMTPKUPTPYE227A5CNFSM4HJEHKYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWVNM3A#issuecomment-497735276, or mute the thread https://github.com/notifications/unsubscribe-auth/ABILLGT2VJGQSJ7NY3PFLKLPYE227ANCNFSM4HJEHKYA .

–

Lars Röttig

Certified PHP Engineer / Web Developer

Magento Community Maintainer MAGENTO 2 CERTIFIED PROFESSIONAL DEVELOPER MAGENTO CERTIFIED DEVELOPER PLUS Zend Certified PHP Engineer

Telefon +49 8031 2210 55 - 0 Telefax +49 8031 2210 55 - 22 l.roettig@techdivision.com

TechDivision GmbH

Spinnereiinsel 3a 83059 Kolbermoor www.techdivision.com

Magento Gold Partner TYPO3 Gold Member

Geschäftsführer: Josef und Stefan Willkommer, Tim Wagner Handelsregister Nr. HRB 17123, Amtsgericht Traunstein UStID gemäß § 27 a Umsatzsteuergesetz: DE249664276