Fixed check for exclude

[tpdrs]

#953

[Jean85]

Thanks for this PR, but we're still internally discussing this and how to proceed.

This PR restores the behavior described in the unified APIs, and makes it easy to include the whole project, and exclude vendor, which should be the most common case; OTOH though it makes impossible to do the reverse, like including a dependency inside vendor (maybe an internal dependency?).

[ste93cry]

Thank you for the contribution, however this as-is cannot be merged because Unified API docs clearly says that in_app_include overrides in_app_exclude, so the behavior you're trying to fix is expected and is not a bug, although I think that we all agree that this depending on the needs of each user may or may not be a good thing

This option can be overridden using in-app-include.
https://docs.sentry.io/error-reporting/configuration/?platform=php#in-app-exclude

[ste93cry]

This PR restores the behavior described in the unified APIs

I would like to clarify this sentence because I feel that it's not: the behavior described in the Unified API is that the in_app_exclude option overrides the in_app_include, so what has been implemented in 2.3 is the correct behavior and thus there is technically nothing to change. You should not blindly migrate from project_root to in_app_include: instead you should make each of its entries more specific than the ones in in_app_exclude and it should work

[tpdrs]

So before I set project_root = /app and excluded /app/vendor, now I have to mention all folders to include (/app/src, /app/bin, /app/tests and don't use app_exclude)

[tpdrs]

Let's look to the code

    /**
     * Checks whether a certain frame should be marked as "in app" or not.
     *
     * @param string      $file         The file to check
     * @param string|null $functionName The name of the function
     */
    private function isFrameInApp(string $file, ?string $functionName): bool
    {
        if (self::INTERNAL_FRAME_FILENAME === $file) {
            return false;
        }

        if (null !== $functionName && 0 === strpos($functionName, 'Sentry\\')) {
            return false;
        }

        $projectRoot = $this->options->getProjectRoot();
        $excludedAppPaths = $this->options->getInAppExcludedPaths();
        $includedAppPaths = $this->options->getInAppIncludedPaths();
        $absoluteFilePath = @realpath($file) ?: $file;
        $isInApp = false;

        if (null !== $projectRoot) {
            $isInApp = 0 === mb_strpos($absoluteFilePath, $projectRoot);
        }

        foreach ($excludedAppPaths as $excludedAppPath) {
            if (0 === mb_strpos($absoluteFilePath, $excludedAppPath)) {
                $isInApp = false;

                break;
            }
        }

        foreach ($includedAppPaths as $includedAppPath) {
            if (0 === mb_strpos($absoluteFilePath, $includedAppPath)) {
                $isInApp = true;

                break;
            }
        }

        return $isInApp;
    }

If you say $projectRoot is deprecated then $excludedAppPaths is useless because $isInApp by default is false and you can remove foreach with check excluded.

[tpdrs]

@ste93cry any comments?

[ste93cry]

So, there is indeed one bug that should be addressed here: $isInApp should be true by default. This would make the things working as follow:

• Everything is included by default (regardless of any option)
• in_app_exclude sets $isInApp to false if any entry matches
• in_app_include sets $isInApp to true if any entry matches

With this behavior it's possible to include everything by default, exclude some folders (e.g. app/vendor) and still include some paths like app/vendor/xxx if needed. Does it makes more sense to you? If so, it would be really cool if you could adjust this PR to fit with the above specs

[tpdrs]

@ste93cry I fixed default value for isInApp if in_app_include is not specified.

[ste93cry]

As I said above, the default value of $isInApp must be true regardless of any option, otherwise we will report all files as not-in-app if you didn't configure project_root nor in_app_include nor in_app_exclude.

[tpdrs]

Fixed

[ste93cry]

@Jean85 I've removed the Type: Breaking label because this PR became a fix for a regression and so even though we are going to change the behavior from 2.3.0 we're really just fixing it. I had a look at the code and it seems fine, if @tpdrs confirms that this solves their original issue LGTM

[tpdrs]

Yep. This one solves my issues.

[stayallive]

Thanks for taking the time to make this PR 👌

[Jean85]

We need a changelog entry!

[tpdrs]

Updated changelog