[Console] improve deprecation warning triggers #12796
Conversation
Since the default helper set of the Console `Application` relies on the `DialogHelper`, the `ProgressHelper` and the `TableHelper`, there must be a way to not always trigger `E_USER_DEPRECATION` errors when one of these helper is used.
| @@ -78,6 +78,14 @@ public function get($name) | |||
| throw new \InvalidArgumentException(sprintf('The helper "%s" is not defined.', $name)); | |||
| } | |||
|
|
|||
| if ('dialog' === $name && $this->helpers[$name] instanceof DialogHelper) { | |||
There was a problem hiding this comment.
We could also remove the 'dialog' === $name check if you agree that checking for the object type is sufficient. I just decided to stick with the keys used by the default helper set (the same applies to the two checks below).
There was a problem hiding this comment.
this looks very weird, couldn't we trigger this when the instance is created, not in this class?
There was a problem hiding this comment.
No, the real issue is that all three helper instances are created when the default helper set is built in the application (see https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Console/Application.php#L949-960). Triggering there results in warnings for every user using the Application whether or not they are using the deprecated helpers (this is why #12771 and #12791 have been opened).
There was a problem hiding this comment.
i would love using HelperSet as Factory, consider
return new HelperSet(array(
new FormatterHelper(),
function() { trigger_error('...'); new DialogHelper(); },
new ProgressHelper(),
new TableHelper(),
new DebugFormatterHelper(),
new ProcessHelper(),
new QuestionHelper(),
));
There was a problem hiding this comment.
We could do that. But then we make this "feature" available to everyone even if it isn't documented. Let's see what the core team thinks about this.
There was a problem hiding this comment.
The current way seems pragmatic to me (keep in mind that this is temporary code) 👍
There was a problem hiding this comment.
I agree with @fabpot here. The suggestion of @timglabisch requires to add a new feature to be able to trigger the deprecation notices, and I'm not sure we want such feature in the HelperSet (and adding a deprecated feature to be able to trigger deprecation notices is even worse)
|
👍 |
|
Thank you @xabbuh. |
This PR was merged into the 2.7 branch. Discussion ---------- [Console] improve deprecation warning triggers | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #12771, #12791 | License | MIT | Doc PR | Since the default helper set of the Console `Application` relies on the `DialogHelper`, the `ProgressHelper` and the `TableHelper`, there must be a way to not always trigger `E_USER_DEPRECATION` errors when one of these helper is used. Commits ------- 88e3314 [Console] improve deprecation warning triggers
Since the default helper set of the Console
Applicationrelies onthe
DialogHelper, theProgressHelperand theTableHelper, theremust be a way to not always trigger
E_USER_DEPRECATIONerrors whenone of these helper is used.