Exception thrown in single catch block passes

[danmooney2]

Preconditions

• Magento.Exceptions.ThrowCatch rule implemented (#29: [New Rule] Exceptions must not be handled in the same function #43; tested on Magento Coding Standard 2.0.0 and 1.0.1)

Steps to reproduce

• Run phpcs with --standard=Magento2 against file containing multiple catch blocks (it fails as it should):

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

namespace Magento;

/**
 * Doer
 */
class Doer
{
    /**
     * Do Something
     */
    public function doSomething()
    {
        try {
            $result = 2;
        } catch (\DummyException $e) {
            throw $e;
        } catch (\NewException $e) {
            throw $e;
        }

        return $result;
    }
}

Run phpcs with --standard=Magento2 against file containing one catch block (it passes when it should not):

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

namespace Magento;

/**
 * Doer
 */
class Doer
{
    /**
     * Do Something
     */
    public function doSomething()
    {
        try {
            $result = 2;
        } catch (\DummyException $e) {
            throw $e;
        }

        return $result;
    }
}

Expected result

• Both files should fail Magento.Exceptions.ThrowCatch rule

Actual result

• Only file with two catch blocks fails Magento.Exceptions.ThrowCatch rule

[larsroettig]

Hi @danmooney2,
it is not a Bug the check is for _Exceptions must not be handled in the same function where they are thrown. _. In your example the both thrown the Exceptions out of the function.

[danmooney2]

Both examples throw all of their exceptions out of the function so both files should pass then? Please elaborate why having 1 catch block fails, while having > 1 catch block that does the same thing passes this rule.

[danmooney2]

@larsroettig Could you please clarify if the sniff is written properly, and if so, why example 1 passes while example 2 fails?

[lenaorobei]

@larsroettig could you please check this isse? Thank you.

[larsroettig]

Yep Lena I will take a look how we can solve it Lena Orobei <[email protected]> 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 <#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 [email protected] 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

[danmooney2]

@lenaorobei Now that we have mutual agreement on the issue, what is the process for removing buggy sniffs from the ruleset?

[lenaorobei]

I would probably decrease its severity to 0 until it fixed.
Need architectural input here @paliarush.

[paliarush]

If I understood the scenario correctly, it is false negative. In this case we should just report an issue to fix the sniff.

If it is false positive - then it should be removed until fixed.

[danmooney2]

I believe both files referenced above should actually pass the rule; editing the expectation

[larsroettig]

I think is a false positive I forget to take a look sorry at the weekend Alex Paliarush <[email protected]> schrieb am Di. 4. Juni 2019 um 17:20:

If I understood the scenario correctly, it is false negative. In this case we should just report an issue to fix the sniff. If it is false positive - then it should be removed until fixed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#93?email_source=notifications&email_token=ABILLGVZIRB4VM5QANWGTCDPY2B5BA5CNFSM4HJEHKYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODW45MEQ#issuecomment-498718226>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABILLGQV4GKE4YCDU7PEXCTPY2B5BANCNFSM4HJEHKYA> .

-- 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 [email protected] 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

[lenaorobei]

@danmooney2 looking at your examples I would say that sniff should fail in both case. The whole purpose of this rule was to prohibit exceptions handling (catching and doing something) and re-throwing in the same function. Maybe I'm missing something. Could you please explain why do you think those examples should pass?

[danmooney2]

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.

[danmooney2]

I reverted back to original issue description/expectation (false positive). I changed it out of a knee jerk fear of having to do even more refactoring of something I don't completely understand 😨, apologies.

[danmooney2]

"Exceptions MUST NOT be rethrown."

When read like that the rule makes much more sense. We're conflating handling and throwing: handling is not always throwing, but throwing is always handling.

[lenaorobei]

@danmooney2 I've talked with @paliarush regarding exceptions handling best practices. The main points are:

• Exceptions must not be used for conditional logic.
• Each application layer must use its own exceptions scope, e.g.:

function doSomething()
{
    try {
        // do something
    } catch (\LowerLayerException $e) {
        // do something if needed
        throw new \MyLayerException;
    return $result;
}

Hope it makes sense.

[danmooney2]

OK I understand not rethrowing caught exceptions in the same function (that's entirely what the sniff is doing). Unless the sniff gets updated to cover everything you just mentioned, I still petition to change the wording of the rule from "handling" to "throwing" for those unlucky individuals who interpret handling as simply having the existence of a catch block.

[danmooney2]

Also @lenaorobei I thought we agreed this is a false positive; both examples should fail.

[lenaorobei]

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.

[larsroettig]

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.

[lenaorobei]

@larsroettig both examples should be fine since it is different exception type.

[lenaorobei]

#120