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

[GH-20] triggerIfCalledFromOutside #21

Merged
merged 1 commit into from
Mar 11, 2021

Conversation

beberlei
Copy link
Member

Fixes #20 and reverts #18

@beberlei beberlei force-pushed the GH-20-TriggerWhenNotFromPackage branch from 992b668 to 7028bea Compare March 11, 2021 20:51
@beberlei beberlei force-pushed the GH-20-TriggerWhenNotFromPackage branch from 7028bea to 907ac04 Compare March 11, 2021 21:03
@beberlei beberlei merged commit db319bc into master Mar 11, 2021
@beberlei beberlei deleted the GH-20-TriggerWhenNotFromPackage branch March 11, 2021 21:10
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

Might be worth creating a directory with a fake app with fake vendors for test purposes. Example: https://github.com/symfony/symfony/tree/5.x/src/Symfony/Bridge/PhpUnit/Tests/DeprecationErrorHandler

@@ -77,6 +80,18 @@ the message.
);
```

When you want to trigger a deprecation only when it is called by a function
outside of the current package, but not trigger when the package itself is the cause,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
outside of the current package, but not trigger when the package itself is the cause,
outside of the current package, but not trigger when the package itself is the caller,

then use:

```php
\Doctrine\Deprecations\Deprecation::triggerIfCalledFromOutside(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
\Doctrine\Deprecations\Deprecation::triggerIfCalledFromOutside(
\Doctrine\Deprecations\Deprecation::triggerOnExternalCall(

}

// do not move this condition to the top, because we still want to
// count occcurences of deprecations even when we are not logging them.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// count occcurences of deprecations even when we are not logging them.
// count occurences of deprecations even when we are not logging them.

// dependency and the caller is not a file in that package.
// When $package is installed as a root package, then this deprecation
// is always ignored
if (strpos($backtrace[0]['file'], 'vendor/' . $package . '/') === false) {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I know its not perfect, but its a cheap way to do the check vs the one in phpunit bridge that does not have to care for performance that much.

Comment on lines +103 to +105
public static function triggerIfCalledFromOutside(string $package, string $link, string $message, ...$args): void
{
$backtrace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 2);
Copy link
Member

Choose a reason for hiding this comment

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

Unconditional usage of debug_backtrace() makes this method unnecessarily expensive even if logging is disabled.

return;
}

// do not move this condition to the top, because we still want to
Copy link
Member

Choose a reason for hiding this comment

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

I believe, in production, this method should return as early as possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, i agree.

I started that in the follow up #24 - though I have an idea to simplify that even more.

@nicolas-grekas
Copy link
Member

This feature should not be required actually: a codebase should not call any of its own deprecated API ever. When internal calls to deprecated APIs cannot be avoided for legacy reasons, adding a quick internal API is going to be much faster and seamless (eg an extra arg to disable the deprecation.)

We achieved this in Symfony, so I'm pretty sure there is no need for this at all.

@morozov
Copy link
Member

morozov commented Mar 15, 2021

a codebase should not call any of its own deprecated API ever.

This API may be an extension point. Not calling it is already a BC break.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 15, 2021

I agree, yet there is always a way to make this API internal. We do it when needed, it works perfectly fine.

@morozov
Copy link
Member

morozov commented Mar 15, 2021

If you make it internal and stop calling, it's still a BC break because in a previous version it wasn't internal and was called. Could you point to an example of the approach you're talking about?

@morozov
Copy link
Member

morozov commented Mar 15, 2021

a codebase should not call any of its own deprecated API ever.

How does this align with this change?

symfony/symfony@98ff750#diff-842527ea5582808ca82ff360aa1f7ceaa44584c9c4b4edd6c27381f62d834d3cR84

I don't see any logical differences between the two approaches except for the fact that Benjamin's one hides all the magic to itself while Symfony's approach requires the developer to implement it over and over.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 15, 2021

The difference is that the code is self-aware, dealing internally with its own concerns. It doesn't leak this concern to a 3rd party lib that will need to hack with the call stack to get past any API boundaries. In terms of perf also, it's negligible, compared to calling debug_backtrace().

@morozov
Copy link
Member

morozov commented Mar 15, 2021

The difference is that the code is self-aware, dealing internally with its own concerns. It doesn't leak this concern to a 3rd party lib [...]

Why is it a problem? All projects use 3rd-party libraries of other concerns.

In terms of perf also, it's negligible, compared to calling debug_backtrace().

In terms of perf, the approach in this library should be close to a no-op in production. In the rest of the environments, it's not a real concern.

Unlike @trigger_error() which only God knows how will behave depending on the runtime configuration.

a codebase should not call any of its own deprecated API ever.

Does this argument still hold?

@nicolas-grekas
Copy link
Member

I think it's a concern yes when there is no proper API involved. Hacking with the backtrace is doing black magic.

What do you mean by no-op? It's true I never benched the overhead of calling debug_backtrace(). Did you? Do you have any numbers?

About my argument, it holds, but I failed to communicate it appropriately. My fear is that this method will be abused to create broken deprecation layers. When deprecating something, authors should try as hard a possible to make their own codebase deprecation-free. This is an involving process. This method could be a convenient way to skip writing a proper BC layer.

@beberlei
Copy link
Member Author

He is referring to the outstanding PR that checks for the mode==none, which will exit early in production before calling debug_backtrace

@beberlei
Copy link
Member Author

beberlei commented Mar 15, 2021

@nicolas-grekas reimplementing the deprecation flags over and over is error prone and in case of some of our more funny PDO extended APIs with variable arguments not really possible.

We tried the flags as you can see in my DBAL PR, but they make it much harder than this approach. We are encapsulating the problem for our use case and make it reusable. As a side benefit triggering deprecations stays declarative and does not become imperative

@morozov
Copy link
Member

morozov commented Mar 15, 2021

My fear is that this method will be abused to create broken deprecation layers [...] This method could be a convenient way to skip writing a proper BC layer.

IMO this fear is orthogonal to the implementation. In both cases, some calls to the deprecated methods are allowed. Although in this case, it's implemented by a very specific method which you can grep and see which exactly of your APIs are still used.

Hacking with the backtrace is doing black magic.

So I assume, hacking with handling suppressed error messages and using non-existing method arguments is not?

I agree that the Symfony's approach is more fine-grained in the way that it allows callers to certify "I'm calling a deprecated method" rather than the deprecated method stating "I'm allowed to be called from within the library". But I'm not sure how essential it is at the moment.

In addition to runtime checks, there are checks at Psalm level 2 which you'll have to suppress if you need to call a method from within the library.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 15, 2021

Unlike @trigger_error() which only God knows how will behave depending on the runtime configuration.

Not "God", but also "not your business" - aka these are decoupled. You don't need to create defensive strategies against authors of userland error handlers. That's what we did in Symfony, and it just works well. Everybody does their part and things play nicely together. Good news also: since Symfony paved the way for many years, you can be pretty confident that userland error handlers are already compliant so that you can build on the required assumptions with confidence.

hacking with handling suppressed error messages and using non-existing method arguments is not?

This feels like a rhetorical question... In case it's not: trigger_error() + @ are defined to be used like we do in Symfony. There is no hack here, this is just using the tools for what they've been designed for.

I agree that the Symfony's approach is more fine-grained in the way that it allows callers to certify "I'm calling a deprecated method" rather than the deprecated method stating "I'm allowed to be called from within the library". But I'm not sure how essential it is at the moment.

Thanks for putting it that way, you've got it right. Not a minor thing to me, especially since you plan to release this for reuse. That's a significant foot gun.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert temporarily ignored to file based triggerIfNotCalledFromWithinPackage
4 participants