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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 15 additions & 15 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ the deduplication can be disabled:

## Usage from a library/producer perspective:

When you want to unconditionally trigger a deprecation even when called
from the library itself then the `trigger` method is the way to go:

```php
\Doctrine\Deprecations\Deprecation::trigger(
"doctrine/orm",
Expand All @@ -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(

"doctrine/orm",
"https://link/to/deprecations-description",
"message"
);
```

Based on the issue link each deprecation message is only triggered once per
request, so it must be unique for each deprecation.

Expand All @@ -87,21 +102,6 @@ Note: A producer/library should never call `Deprecation::enableWith` methods
and leave the decision how to handle deprecations to application and
frameworks.

Sometimes you cannot avoid to trigger a deprecation from the library itself,
specifically when the new alternative functionality still calls the old API
internally.

In that case you can temporarily ignore a deprecation for 1 or more calls with
this API:

```php
\Doctrine\Deprecations\Deprecation::ignoreDeprecationTemporarily(
'https://github.com/doctrine/orm/issue/1234'
);
```

Pass the number of times to ignore as a second argument when it is more than once.

## Usage in PHPUnit tests

There is a `VerifyDeprecations` trait that you can use to make assertions on
Expand Down
69 changes: 50 additions & 19 deletions lib/Doctrine/Deprecations/Deprecation.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use function basename;
use function debug_backtrace;
use function sprintf;
use function strpos;
use function trigger_error;

use const DEBUG_BACKTRACE_IGNORE_ARGS;
Expand Down Expand Up @@ -55,9 +56,6 @@ class Deprecation
/** @var array<string,int> */
private static $ignoredLinks = [];

/** @var array<string,int> */
private static $temporarilyIgnoredLinks = [];

/** @var bool */
private static $deduplication = true;

Expand All @@ -72,15 +70,49 @@ class Deprecation
*/
public static function trigger(string $package, string $link, string $message, ...$args): void
{
// Do not trigger this deprecation if it is temporarily ignored,
// because it is expected to be called for 1 or more times.
if (array_key_exists($link, self::$temporarilyIgnoredLinks)) {
self::$temporarilyIgnoredLinks[$link]--;
if (array_key_exists($link, self::$ignoredLinks)) {
self::$ignoredLinks[$link]++;
} else {
self::$ignoredLinks[$link] = 1;
}

if (self::$deduplication === true && self::$ignoredLinks[$link] > 1) {
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.

// 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.

if (self::$type === self::TYPE_NONE) {
return;
}

if (isset(self::$ignoredPackages[$package])) {
return;
}

$backtrace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 2);

$message = sprintf($message, ...$args);

self::delegateTriggerToBackend($message, $backtrace, $link, $package);
}

/**
* @param mixed $args
*/
public static function triggerIfCalledFromOutside(string $package, string $link, string $message, ...$args): void
{
$backtrace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 2);
Comment on lines +103 to +105
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.


if (self::$temporarilyIgnoredLinks[$link] <= 0) {
unset(self::$temporarilyIgnoredLinks[$link]);
}
// "outside" means we assume that $package is currently installed as a
// 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.

return;
}

if (strpos($backtrace[1]['file'], 'vendor/' . $package . '/') !== false) {
return;
}

Expand All @@ -104,10 +136,16 @@ public static function trigger(string $package, string $link, string $message, .
return;
}

$backtrace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 2);

$message = sprintf($message, ...$args);

self::delegateTriggerToBackend($message, $backtrace, $link, $package);
}

/**
* @param array<mixed> $backtrace
*/
private static function delegateTriggerToBackend(string $message, array $backtrace, string $link, string $package): void
{
if (self::$type === self::TYPE_TRIGGER_ERROR) {
$message .= sprintf(
' (%s:%d called by %s:%d, %s, package %s)',
Expand Down Expand Up @@ -175,8 +213,6 @@ public static function disable(): void
foreach (self::$ignoredLinks as $link => $count) {
self::$ignoredLinks[$link] = 0;
}

self::$temporarilyIgnoredLinks = [];
}

public static function ignorePackage(string $packageName): void
Expand All @@ -191,11 +227,6 @@ public static function ignoreDeprecations(string ...$links): void
}
}

public static function ignoreDeprecationTemporarily(string $link, int $times = 1): void
{
self::$temporarilyIgnoredLinks[$link] = $times;
}

public static function getUniqueTriggeredDeprecationsCount(): int
{
return array_reduce(self::$ignoredLinks, static function (int $carry, int $count) {
Expand Down
31 changes: 0 additions & 31 deletions tests/Doctrine/Deprecations/DeprecationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -184,35 +184,4 @@ public function testDeprecationWithIgnoredPackage(): void
$this->assertEquals(1, Deprecation::getUniqueTriggeredDeprecationsCount());
$this->assertEquals(['https://github.com/doctrine/orm/issue/1234' => 1], Deprecation::getTriggeredDeprecations());
}

public function testDeprecationWithTemporarilyIgnored(): void
{
Deprecation::enableWithTriggerError();
Deprecation::ignoreDeprecationTemporarily('https://github.com/doctrine/orm/issue/3333');

Deprecation::trigger(
'doctrine/orm',
'https://github.com/doctrine/orm/issue/3333',
'this is deprecated %s %d',
'foo',
1234
);

$this->assertEquals(0, Deprecation::getUniqueTriggeredDeprecationsCount());

try {
Deprecation::trigger(
'doctrine/orm',
'https://github.com/doctrine/orm/issue/3333',
'this is deprecated %s %d',
'foo',
1234
);

$this->fail('Not expected');
} catch (Throwable $e) {
$this->assertEquals(1, Deprecation::getUniqueTriggeredDeprecationsCount());
$this->assertEquals(['https://github.com/doctrine/orm/issue/3333' => 1], Deprecation::getTriggeredDeprecations());
}
}
}