Handle composed monolog-bridge activation strategy #336

pvgnd · 2020-02-16T12:05:01Z

This PR instantiate the new HttpCodeActivationStrategy & NotFoundActivationStrategy with an inner strategy instead of an actionLevel

closes #369

DependencyInjection/MonologExtension.php

nico-incubiq · 2020-12-17T14:30:45Z

Travis builds are failing under the new version of xdebug, you can fix that with setting XDEBUG_MODE to "coverage" in the config. See https://travis-ci.community/t/xdebug-3-is-installed-by-default-breaking-builds/10748

pvgnd · 2020-12-18T15:06:02Z

@nico-incubiq Thanks I added a commit

Tests/DependencyInjection/FixtureMonologExtensionTest.php

jokaorgua · 2020-12-30T14:04:10Z

@pvgnd any chance you could fix the PR to pass all tests?
I think your fix is very important because this will fix a lot of deprecated warnings for all users who uses fingers_crossed in symfony 5.2

pvgnd · 2020-12-30T14:55:33Z

Of course, I will try to take some time to fix tests by the end of the week. Le mer. 30 déc. 2020 à 15:04, jokaorgua <[email protected]> a écrit :

…

@pvgnd <https://github.com/pvgnd> any chance you could fix the PR to pass all tests? I think your fix is very important because this will fix a lot of deprecated warnings for all users who uses fingers_crossed in symfony 5.2 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#336 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABHSZYD22OXHGQCH7V7CDXTSXMXORANCNFSM4KWCPP5A> .

.travis.yml

DependencyInjection/MonologExtension.php

Tests/DependencyInjection/MonologExtensionTest.php

nico-incubiq · 2021-01-18T11:20:07Z

No update on this PR? @jderusse @stof do you think it can be merged?

nicolas-grekas · 2021-01-20T18:08:49Z

@pvgnd can you please rebase?
/cc @lyrixx could you please have a look?
We have deprecations now :)

pvgnd · 2021-01-20T19:01:10Z

@nicolas-grekas done, branch rebased

OskarStark · 2021-01-20T19:59:50Z

Tests/DependencyInjection/MonologExtensionTest.php

+        if (!class_exists(HttpCodeActivationStrategy::class)) {
+            $this->markTestSkipped('Symfony Monolog 4.1+ is needed.');
+        }
+        if (!\class_exists(SwitchUserTokenProcessor::class)) {
+            $this->markTestSkipped('Symfony Monolog 5.2+ is needed.');
+        }


Not sure why fabbot.io is not running, but either we use class_exists with or without a leading backslash 🧐 can you please check or reopen the PR to trigger fabbot again?

I am currently on a phone 📱

Thanks

Reopening does not trigger fabbot, apparently.

generally, it will work in both cases, but \class_exists() will be slightly faster, as parser will not check current namespace for such function and will check global namespace right away.

but in terms of code style consistency, IMO there shouldn't be such mix of usage. Either with backslashes, either without ;)

jderusse

I think the implementation could be simplified a lot (cf my comment)

jderusse · 2021-01-23T14:01:12Z

DependencyInjection/MonologExtension.php

-                $activationDef = new Definition('Symfony\Bridge\Monolog\Handler\FingersCrossed\NotFoundActivationStrategy', [
-                    new Reference('request_stack'),
-                    $handler['excluded_404s'],
-                    $handler['action_level']
-                ]);
+                if ($isLegacyBridge) {
+                    $activationDef = new Definition('Symfony\Bridge\Monolog\Handler\FingersCrossed\NotFoundActivationStrategy', [
+                        new Reference('request_stack'),
+                        $handler['excluded_404s'],
+                        $handler['action_level']
+                    ]);
+                } else {
+                    $activationDef = new Definition('Symfony\Bridge\Monolog\Handler\FingersCrossed\NotFoundActivationStrategy', [
+                        new Reference('request_stack'),
+                        $handler['excluded_404s'],
+                        new Definition('Monolog\Handler\FingersCrossed\ErrorLevelActivationStrategy', [
+                            $handler['action_level']
+                        ])
+                    ]);
+                }


IMHO all this can be simplified to

+ $activation = $handler['action_level']; + if (class_exists(SwitchUserTokenProcessor::class)) { + $action = new Definition(ErrorLevelActivationStrategy::class, [ + $activation + ]) + } // ... if (...) { $activationDef = new Definition('Symfony\Bridge\Monolog\Handler\FingersCrossed\NotFoundActivationStrategy', [ new Reference('request_stack'), $handler['excluded_404s'], - $handler['action_level'] + $activation ]); $activation = new Reference($handlerId.'.not_found_strategy'); } elseif (...) { $activationDef = new Definition('Symfony\Bridge\Monolog\Handler\FingersCrossed\HttpCodeActivationStrategy', [ new Reference('request_stack'), $handler['excluded_http_codes'], - $handler['action_level'] + $activation ]); $activation = new Reference($handlerId.'.http_code_strategy'); - } else { - $activation = $handler['action_level']; }

jderusse · 2021-01-23T14:06:05Z

Tests/DependencyInjection/FixtureMonologExtensionTest.php

    {
+        if (\class_exists(SwitchUserTokenProcessor::class)) {


Suggested change

if (\class_exists(SwitchUserTokenProcessor::class)) {

if (class_exists(SwitchUserTokenProcessor::class)) {

same everywhere

jderusse · 2021-01-24T19:05:12Z

Is fabbot enabled on this project?
I checked on other PR, fabbot is not running..

adrienfr · 2021-01-29T08:24:24Z

Is fabbot enabled on this project?
I checked on other PR, fabbot is not running..

Indeed, it seems like it's not enabled, regarding others PR for this project.

jokaorgua · 2021-02-08T07:40:19Z

@jderusse @adrienfr
I'm really sorry to disturb you but is there any movement in accepting this PR and making a new release with fixes?

jderusse · 2021-02-08T08:30:16Z

@jderusse @adrienfr
I'm really sorry to disturb you but is there any movement in accepting this PR and making a new release with fixes?

Looks like the comments I have made, are not addressed.

jokaorgua · 2021-02-10T07:48:59Z

@pvgnd maybe you could help a bit to move this further ?

spirit-q2 · 2021-02-16T17:28:05Z

Ping @stof 🙏

…vailable (jderusse) This PR was merged into the 3.x-dev branch. Discussion ---------- Use `ActivationStrategy` instead of `actionLevel` when available Take over #336 Fixes #369 Commits ------- 7eaabac Use `ActivationStrategy` instead of `actionLevel` when available

jderusse · 2021-02-18T10:51:00Z

closed in favor of #388

pvgnd force-pushed the composed-activation-strategy branch from 3a07f6b to 7d1983e Compare February 16, 2020 18:50

pvgnd changed the title ~~WIP: Handle composed monolog-bridge activation strategy~~ Handle composed monolog-bridge activation strategy Feb 16, 2020

pvgnd force-pushed the composed-activation-strategy branch from 4bab351 to 9187278 Compare February 29, 2020 15:51

adrienfr mentioned this pull request Nov 10, 2020

Handle deprecation of the actionLevel argument in finger crossed strategies #369

Closed

pvgnd force-pushed the composed-activation-strategy branch 2 times, most recently from 5d7d11c to 7660e8c Compare November 11, 2020 10:50

adrienfr mentioned this pull request Dec 1, 2020

Deprecation: "Passing an actionLevel (int|string) as constructor's 3rd argument" #378

Closed

stof reviewed Dec 1, 2020

View reviewed changes

DependencyInjection/MonologExtension.php Outdated Show resolved Hide resolved

pvgnd requested a review from stof December 6, 2020 21:36

Taluu reviewed Dec 8, 2020

View reviewed changes

DependencyInjection/MonologExtension.php Outdated Show resolved Hide resolved

DependencyInjection/MonologExtension.php Outdated Show resolved Hide resolved

DependencyInjection/MonologExtension.php Outdated Show resolved Hide resolved

nico-incubiq reviewed Dec 18, 2020

View reviewed changes

Tests/DependencyInjection/FixtureMonologExtensionTest.php Outdated Show resolved Hide resolved

jderusse reviewed Dec 31, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

DependencyInjection/MonologExtension.php Outdated Show resolved Hide resolved

DependencyInjection/MonologExtension.php Outdated Show resolved Hide resolved

Tests/DependencyInjection/MonologExtensionTest.php Show resolved Hide resolved

pvgnd force-pushed the composed-activation-strategy branch from d962597 to a928f4e Compare January 3, 2021 15:46

pvgnd requested a review from jderusse January 3, 2021 16:05

pvgnd force-pushed the composed-activation-strategy branch from a928f4e to bbf53e9 Compare January 5, 2021 10:37

Handle composed monolog-bridge activation strategy

86646d5

pvgnd force-pushed the composed-activation-strategy branch from bbf53e9 to 86646d5 Compare January 20, 2021 18:38

OskarStark reviewed Jan 20, 2021

View reviewed changes

jderusse mentioned this pull request Jan 23, 2021

Fix deprecated #385

Closed

jderusse reviewed Jan 23, 2021

View reviewed changes

derrabus closed this Jan 24, 2021

derrabus reopened this Jan 24, 2021

jderusse mentioned this pull request Feb 14, 2021

Use ActivationStrategy instead of actionLevel when available #388

Merged

jderusse closed this Feb 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle composed monolog-bridge activation strategy #336

Handle composed monolog-bridge activation strategy #336

pvgnd commented Feb 16, 2020 •

edited

Loading

nico-incubiq commented Dec 17, 2020

pvgnd commented Dec 18, 2020

jokaorgua commented Dec 30, 2020

pvgnd commented Dec 30, 2020 via email

nico-incubiq commented Jan 18, 2021

nicolas-grekas commented Jan 20, 2021

pvgnd commented Jan 20, 2021

OskarStark Jan 20, 2021

derrabus Jan 24, 2021

spirit-q2 Jan 25, 2021 •

edited

Loading

jderusse left a comment

jderusse Jan 23, 2021

jderusse Jan 23, 2021

jderusse commented Jan 24, 2021

adrienfr commented Jan 29, 2021

jokaorgua commented Feb 8, 2021 •

edited

Loading

jderusse commented Feb 8, 2021

jokaorgua commented Feb 10, 2021

spirit-q2 commented Feb 16, 2021

jderusse commented Feb 18, 2021

	if (\class_exists(SwitchUserTokenProcessor::class)) {
	if (class_exists(SwitchUserTokenProcessor::class)) {

Handle composed monolog-bridge activation strategy #336

Handle composed monolog-bridge activation strategy #336

Conversation

pvgnd commented Feb 16, 2020 • edited Loading

nico-incubiq commented Dec 17, 2020

pvgnd commented Dec 18, 2020

jokaorgua commented Dec 30, 2020

pvgnd commented Dec 30, 2020 via email

nico-incubiq commented Jan 18, 2021

nicolas-grekas commented Jan 20, 2021

pvgnd commented Jan 20, 2021

OskarStark Jan 20, 2021

Choose a reason for hiding this comment

derrabus Jan 24, 2021

Choose a reason for hiding this comment

spirit-q2 Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

jderusse left a comment

Choose a reason for hiding this comment

jderusse Jan 23, 2021

Choose a reason for hiding this comment

jderusse Jan 23, 2021

Choose a reason for hiding this comment

jderusse commented Jan 24, 2021

adrienfr commented Jan 29, 2021

jokaorgua commented Feb 8, 2021 • edited Loading

jderusse commented Feb 8, 2021

jokaorgua commented Feb 10, 2021

spirit-q2 commented Feb 16, 2021

jderusse commented Feb 18, 2021

pvgnd commented Feb 16, 2020 •

edited

Loading

spirit-q2 Jan 25, 2021 •

edited

Loading

jokaorgua commented Feb 8, 2021 •

edited

Loading