Formalize current deprecations #14915

markstory · 2020-08-19T01:01:38Z

Add formal warnings for deprecations that have been added so far in 4.x. I'll follow this up with a change to rector as well.

othercorey · 2020-08-19T01:59:36Z

src/ORM/Behavior.php

@@ -188,6 +188,8 @@ public function initialize(array $config): void
     */
    public function getTable(): Table
    {
+        deprecationWarning('Behavior::getTable() is deprecated. Use table() instead.');


Are we going to throw a warning the same point release it was deprecated? I am not sure what the policy is or if there is one.

Throwing them a bit too soon can be annoying for plugins and stuff.
As they always need to then pin to the highest version of minors, leaving a lot of people very annoyed by this.
The same issue I mentioned back when we started to do hard instead of soft deprecations (or rather opt out vs opt in).

What benefit would delaying warnings have? For anyone keeping up to date with the minors will eventually see the warnings anyway.

Plugins wouldnt be forced to directly upgrade (and thus wouldnt minimize constraint range).
So a wider constraint range makes it easier to support a wider range of users and their different plugin usage.
All they need would be to silence deprections here where needed.
But if plugins are force-constraint to last minor all the time, this puts a lot of pressure on all involved and makes it impossible to keep a lot of different plugins in the app - since not all plugins are well maintained.

Example

PluginA - not well maintained

PluginB - maintained, now forced to use latest core minor only because of this

If user wants to get last patch of PluginB, he is now forced to upgrade also core to latest minor, which wasn't needed if we didn't enforce the deprecations this way.
If now the other plugin is not updating here yet, you are in an impossible situation already.
And that would be most dramatic always directly after a new minor appeared.

I don't see how these deprecation warnings force constraint to last minor when one can just silence the deprecations by modifying the error level.

I wouldn't mind waiting till the last minor release to add deprecation warnings but we don't follow fixed release cycles and don't exactly know which minor will be the last one. We started adding them in 3.6 thinking it might be the last but we are up to 3.9 now. Also waiting till next minor to add the warning doesn't work in practice as evident by this PR, we always forget to add the warnings later.

Thats because you only have a very small amount of plugins I do :)
I can tell you, this is super annoying.

The tests are failing, so you remove the deprecated part in favor of the new code.
But now you need to constraint, as the code would not work anymore with the pervious minor
So this is the force on last minor for all plugins that have anything related to this kind of deprecation errors thrown.

And the result (despite a lot of work for plugin maintainers) is a minor lock from project side to latest, while not maintained plugins are often forcing to keep you back, forcing into a dead lock scenario.

This all wouldnt be an issue if we went to what I advocating very loudly for years:
Opt-in instead of Opt-out notifications :)

I agree. With PHP, I'd much prefer warnings in doc blocks.

@dereuromark Building another mechanism on top of PHP errors seems silly when we already have access to an opt-in/out system for error warnings. Should we be setting the default error level in plugins and applications to exclude deprecations?

If people think the runtime warnings are annoying and not helpful, we can skip this process entirely and do only documentation warnings. I don't think documentation only warnings will help users though. You can't use documentation tags to verify that your application has no remaining deprecations. Runtime warnings lets you at least be confident that the code you have tests for, or code you manually test no longer has deprecations.

src/View/Helper/NumberHelper.php

ADmad · 2020-08-19T11:43:47Z

src/ORM/Behavior.php

@@ -188,6 +188,8 @@ public function initialize(array $config): void
     */
    public function getTable(): Table
    {
+        deprecationWarning('Behavior::getTable() is deprecated. Use table() instead.');


What benefit would delaying warnings have? For anyone keeping up to date with the minors will eventually see the warnings anyway.

src/Database/Type/DateTimeType.php

src/Http/Cookie/Cookie.php

dereuromark · 2020-08-19T15:10:36Z

Above, I explained how too strict opt-out can lead to plugin constraint topic and thus fallout for projects.

Additionally, I would like to point out that we need to think about what we want to achieve here.
Do we want to help people upgrading? Or punish the ones that didn't (yet)?
The latter is sure in line with these deprecations and the result usually is that all projects just silence it completely, getting rid of any and all (even external or more low level) deprecations here.
So in the end (and that includes my apps as well), people really don't see anything anymore. And also can never activate it again, as at least a few plugins directly are responsible for some of them.

If we want to really help people while not annoy the others that are not ready for it yet (there is no business value in these things):

We shouldn't throw them for anything that is not critical. Security-related, sure. But if it is just an API rename, that can be done any time, using rector this is a pure opt-in decision and does not need you to silence anything until then.
An opt-in way would suffice, e.g. a configure driven flag that would enable this. Ideally, this is also scoped to the project in reporting. The projects usually have no control over plugin code, and having a lot of noise coming from those will just cloak the actual issues they would like to find and fix.
Let rector tool do the main legwork for API renames, as this is a) fully automated b) includes good reporting in dry-run for the scope you are interested in c) doesn't require the throwing of deprecations anymore by design.

Maybe other people see it differently, this is just my experience being involved in all sides, and thus being able to speak from experience from project and plugin side as well.

Co-authored-by: Edgaras Janušauskas <edgaras.janusauskas@gmail.com>

garas · 2020-08-19T16:02:01Z

I would like granular opt-out by file path, similary how currently psalm-baseline.xml ignores warnings, also developer would always see the list of plugins or app files to upgrade.

Developer could add ROOT relative paths to Error.ignoredDeprecationsPaths config array, ex. src/, src/SomeFile.php, vendor/org/plugin-name/.

$frame['file'] from here would be used.

cakephp/src/Core/functions.php

Lines 291 to 293 in 2c0273a

    
           if (isset($trace[$stackFrame])) { 
        
               $frame = $trace[$stackFrame]; 
        
               $frame += ['file' => '[internal]', 'line' => '??'];

Co-authored-by: ADmad <ADmad@users.noreply.github.com>

markstory · 2020-08-20T03:54:00Z

I would like granular opt-out by file path, similary how currently psalm-baseline.xml ignores warnings, also developer would always see the list of plugins or app files to upgrade.

@garas Why an opt out by path? Would an opt-in by path work? Having a set of paths you want to receive deprecation warnings from could help with the problems that @dereuromark has been having as well.

othercorey · 2020-08-20T04:33:04Z

One place where deprecation documentation is difficult is options. It's not something we have a standard way of deprecating.

garas · 2020-08-20T11:38:33Z

Why an opt out by path? Would an opt-in by path work? Having a set of paths you want to receive deprecation warnings from could help with the problems that @dereuromark has been having as well.

@markstory I think opt-out by path will work better as I want to have most code/plugins clean so I will run with warnings enabled everywhere first, then fill list one-by-one with paths if I don't control them or I can't fix now. List should be kept as short as possibly and perfectly empty.

I don't see how opt-in by path would help. If path is not in list, I will miss if previously-clean code becomes dirty code. That's not what I want.

List should document problems I know, but I can't fix now, and I should revise and clean up later by upgrading plugins or code.

ADmad · 2020-08-20T13:27:38Z

@garas Something like this?

diff --git a/src/Core/functions.php b/src/Core/functions.php
index 330fc39f3c..b48b195ffd 100644
--- a/src/Core/functions.php
+++ b/src/Core/functions.php
@@ -292,6 +292,14 @@ if (!function_exists('deprecationWarning')) {
             $frame = $trace[$stackFrame];
             $frame += ['file' => '[internal]', 'line' => '??'];
 
+            // deprecation_optout.php must return array of paths relative to app ROOT.
+            // For e.g. ['src/Controller/FooFontroller.php', 'vendor/cakephp/bake/src/Plugin.php']
+            $optOutList = @include CONFIG . 'deprecation_optout.php';
+            $path = substr($frame['file'], strlen(ROOT) + 1);
+            if ($optOutList && in_array($path, $optOutList, true)) {
+                return;
+            }
+
             $message = sprintf(
                 '%s - %s, line: %s' . "\n" .
                 ' You can disable deprecation warnings by setting `Error.errorLevel` to' .

garas · 2020-08-20T14:50:59Z

Something like that 👍, but list should be stored in config/app.php, DS normalized, and should be able exclude whole directory/plugin like vendor/cakephp/bake/ if plugin has many bad files.

markstory · 2020-08-20T16:54:32Z

To replay back what I'm understanding:

Enabling deprecations on/off would still be done via Error.level and the PHP constants.
If you have a noisy plugin or a 'bad' part of your code base you can silence or disable deprecations using the proposed configuration. The new configuration option could be Error.disableDeprecations and contain an array of glob compatible patterns. If a path matches one of the glob expression any deprecations that are triggered will not be rendered.

Is that what you folks understand too?

markstory · 2020-08-23T02:41:09Z

Going to merge this and follow up with another pull request to add the deprecation silencing configuration.

Refs cakephp/cakephp#14915

Being able to have deprecations on globally but silence problematic parts of your application or a noisy plugin that can't be upgraded just yet allows developers more control when upgrading. Instead of having to hope they don't add new deprecations they can fix a part of their application and then remove it from the deprecation ignore patterns. I've used glob here instead of regex as I don't think the power of regex is needed for this. Refs #14915

Being able to have deprecations on globally but silence problematic parts of your application or a noisy plugin that can't be upgraded just yet allows developers more control when upgrading. Instead of having to hope they don't add new deprecations they can fix a part of their application and then remove it from the deprecation ignore patterns. I've used glob here instead of regex as I don't think the power of regex is needed for this. Refs cakephp/cakephp#14915

Formalize current deprecations

7667c92

Add formal warnings for deprecations that have been added so far in 4.x. I'll follow this up with a change to rector as well.

markstory added the deprecations label Aug 19, 2020

markstory added this to the 4.2.0 milestone Aug 19, 2020

othercorey reviewed Aug 19, 2020

View reviewed changes

garas reviewed Aug 19, 2020

View reviewed changes

src/View/Helper/NumberHelper.php Outdated Show resolved Hide resolved

ADmad reviewed Aug 19, 2020

View reviewed changes

Update src/View/Helper/NumberHelper.php

bc8c90d

Co-authored-by: Edgaras Janušauskas <edgaras.janusauskas@gmail.com>

Apply suggestions from code review

ab88514

Co-authored-by: ADmad <ADmad@users.noreply.github.com>

Fix CS error

f6a7c58

ADmad force-pushed the add-deprecations branch from 0aef6aa to f6a7c58 Compare August 22, 2020 07:55

markstory merged commit 9cdfe12 into 4.next Aug 23, 2020

markstory deleted the add-deprecations branch August 23, 2020 02:41

markstory added a commit to cakephp/app that referenced this pull request Aug 23, 2020

Add deprecation disable configuration option.

e860b28

Refs cakephp/cakephp#14915

markstory mentioned this pull request Aug 23, 2020

Deprecation config cakephp/app#803

Merged

markstory mentioned this pull request Aug 23, 2020

Add support for ignoring deprecations by path #14925

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Formalize current deprecations #14915

Formalize current deprecations #14915

markstory commented Aug 19, 2020

othercorey Aug 19, 2020

dereuromark Aug 19, 2020

ADmad Aug 19, 2020

dereuromark Aug 19, 2020 •

edited

ADmad Aug 19, 2020

dereuromark Aug 19, 2020 •

edited

dereuromark Aug 19, 2020

rchavik Aug 19, 2020

markstory Aug 19, 2020

ADmad Aug 19, 2020

dereuromark commented Aug 19, 2020 •

edited

garas commented Aug 19, 2020

markstory commented Aug 20, 2020

othercorey commented Aug 20, 2020

garas commented Aug 20, 2020

ADmad commented Aug 20, 2020

garas commented Aug 20, 2020

markstory commented Aug 20, 2020

markstory commented Aug 23, 2020

Formalize current deprecations #14915

Formalize current deprecations #14915

Conversation

markstory commented Aug 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dereuromark Aug 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dereuromark Aug 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dereuromark commented Aug 19, 2020 • edited

garas commented Aug 19, 2020

markstory commented Aug 20, 2020

othercorey commented Aug 20, 2020

garas commented Aug 20, 2020

ADmad commented Aug 20, 2020

garas commented Aug 20, 2020

markstory commented Aug 20, 2020

markstory commented Aug 23, 2020

dereuromark Aug 19, 2020 •

edited

dereuromark Aug 19, 2020 •

edited

dereuromark commented Aug 19, 2020 •

edited