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

Fast opt-in check for deprecations of CakePHP #11889

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
10 participants
@dereuromark
Copy link
Member

commented Apr 1, 2018

I would like to discuss a very fast way of opt-in checking the deprecation warnings of CakePHP.

Current status quo

It will throw them until you globally turn of E_USER_DEPRECATED.
Which itself will also turn of all other (either own or from specific vendors) ones.

This itself means in reality:

  • Most users will just turn this off forever, especially with the plugin ecosystem not always being super quick with each minor released and having deprecations in them you cannot get rid off.
  • This is not helpful, as it only is a pain without any much gain. I am afraid this will reduce the currently nicely working plugin ecosystem and might scare off some of the newbies and people freshly checking out the code (you get immediate errors everywhere with a lot of plugins).
  • The time around 3.next landing, this is especially painful, as some plugins are not yet upgraded, others you need to know to use "3.next" branch etc. Very confusing, especially if you want to help checking if the next minor framework version is BC and will work fine with your app.

Proposal

Use this tool as opt-in and use it deliberately to help upgrading parts of the system when there is time and resources. Do not mix this with other deprecation warnings of other parts of the vendor or app namespace, and force those to also hide away.

This means:

  • Plugins can stay BC and supporting at least one more version (e.g. 3.5 and 3.6 instead of just 3.6), easing the pain for this ecosystem. Most could work with more than just the latest.
  • No one is forced to always upgrade immediately. There is neither business value or any other value other than being "4.x ready" - which you wouldnt need to spend immediate resources on.
  • You dont scare off people with something that shouldnt scare off anyone. This is and should be a optional thing to do. Most do not quite know how to turn off the global deprecations either.

We can enable the constant by default in cakephp/app bootstrap and document how to activate it for existing projects.

Alternative

We could also opt-out by default - but this would mean all plugins then would also have to add this constant in their testing suite if they want to stay compatible with a previous framework version.

Why const

I used a const as this is a good "application only" way of introducing a flag which also has probably the quickest speed (over Configure etc). We sure don't want much performance issues around this issue.

For test suites, the define statement can have a getenv() part to read from config here.


I am bringing this up specifically to discuss the pros and cons here of the current opt-out which will soon land in stable master.

@dereuromark dereuromark added this to the 3.6.0 milestone Apr 1, 2018

@dereuromark dereuromark added the RFC label Apr 1, 2018

@markstory

This comment has been minimized.

Copy link
Member

commented Apr 1, 2018

We would need this constant in our own test suite to make tests pass in cakephp/cakephp.

I like the idea of a constant more than the configure check. I have some concern that this constant needs to be set and the error level need to be correct for deprecations to be visible, but I don't have a solution for that right now.

. Do not mix this with other deprecation warnings of other parts of the vendor or app namespace, and force those to also hide away.

I originally thought that havin one knob for all sorts of deprecations would be helpful. With this change deprecations emitted by cake and plugins using our helper function have require an additional layer of knobs to toggle their deprecations. I don't have a better solution, but we are adding more complexity and potentially 'magic' constants that need to be defined.

Adding this constant will require more documentation than an error level alone would too.

@saeideng

This comment has been minimized.

Copy link
Member

commented Apr 1, 2018

how you want to disable deprecations warnings only for plugin or especial plugin ?
or what happen if one of plugins was not up-to-date but others was update ?
how we can enable deprecations warnings just for user App area , and not for one or all plugins ?

@saeideng

This comment has been minimized.

Copy link
Member

commented Apr 1, 2018

if this constant is fast to check
my suggest is to having 2 constant
CAKEPHP_DEPRECATIONS and CAKEPHP_PLUGINS_DEPRECATIONS
and other for checking plugin namespace ( needs to check performance)

@markstory

This comment has been minimized.

Copy link
Member

commented Apr 1, 2018

@saeideng Why would we need more constants/controls? In other tools I have worked with (ember.js, sqlachemy and rails) deprecations are always an all or nothing flag. If any of your plugins have deprecations still, turn the warnings off unless you want to address the issues.

The goal with deprecations warnings is to help users know what changes are coming. If they aren't ready/don't want to upgrade soon warnings should be turned off.

@markstory markstory modified the milestones: 3.6.0, 3.7.0 Apr 15, 2018

@dereuromark

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2018

Experiences like https://twitter.com/panzer803/status/985828819905871872 just prove my point.
Also, now every plugin cannot work anymore without adjustment for 3.6 - a hard bump.

Any bugfix/feature added into https://github.com/FriendsOfCake/search now will require 3.6 of CakePHP.
If you didnt upgrade yet, you are forced to (and get all the other issues along with it).
This is a no go for me.

Again: Plugin development (and in the end project development) shouldnt have the immediate need to bump always to latest minor framework version right away. This just creates obstacles for no good reason. I said it before, I like to maintain at least 1 version back (e.g. 3.6 + 3.5 for now), which is impossible given the new "constraints" due to the hard deprecation issues in place.

@davidyell

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2018

I found this issue as my colleagues ran a composer update today and all their projects "broke". This is not the experience they wanted in a minor release, and raised questions in the office as to why upgrading would break the application.

So immediately, this creates a whole sideline of why is it erroring? What needs fixing? Then you realise that it's deprecations, but you're not here to fix those, so you spend time finding out how to turn them off, disable them and finally can continue working.

I totally agree with @dereuromark this must be opt-in. When userland developers work to fix deprecations they can turn them on, but pushing something which causes applications to hard fail on a minor release will cause pain for everyone, and I'd think in most cases, just when they are not trying to fix deprecations.

I can't see this being anything but painful, and causing a large amount of support.

@dereuromark dereuromark referenced this pull request Apr 16, 2018

Merged

updates for cake 3.6 #38

@codecov-io

This comment has been minimized.

Copy link

commented Apr 16, 2018

Codecov Report

Merging #11889 into 3.next will increase coverage by 0.18%.
The diff coverage is 50%.

Impacted file tree graph

@@             Coverage Diff              @@
##             3.next   #11889      +/-   ##
============================================
+ Coverage     91.96%   92.15%   +0.18%     
- Complexity    13479    14277     +798     
============================================
  Files           467      467              
  Lines         34627    36802    +2175     
============================================
+ Hits          31845    33915    +2070     
- Misses         2782     2887     +105
Impacted Files Coverage Δ Complexity Δ
src/Core/functions.php 77.98% <50%> (-0.53%) 0 <0> (ø)
src/Database/Statement/BufferedStatement.php 96.36% <0%> (-3.64%) 21% <0%> (+6%)
src/Datasource/QueryTrait.php 90.77% <0%> (-0.75%) 73% <0%> (+25%)
src/View/Helper/FormHelper.php 95.07% <0%> (-0.08%) 484% <0%> (+107%)
src/I18n/Parser/PoFileParser.php 100% <0%> (ø) 33% <0%> (+11%) ⬆️
src/Routing/Route/Route.php 99.7% <0%> (ø) 149% <0%> (ø) ⬇️
src/Http/Response.php 93.54% <0%> (+0.08%) 533% <0%> (+267%) ⬆️
src/Database/Schema/PostgresSchema.php 98.83% <0%> (+0.4%) 162% <0%> (+51%) ⬆️
src/ORM/Rule/ExistsIn.php 98.36% <0%> (+0.44%) 27% <0%> (+5%) ⬆️
src/I18n/RelativeTimeFormatter.php 95.02% <0%> (+0.49%) 74% <0%> (ø) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6565302...bbce25e. Read the comment docs.

@dereuromark

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2018

It is also super annoying to have to do dereuromark/cakephp-ide-helper@8c69b64 in every plugin now
(which doesnt even seem to work? :/)

as well as telling people that 3.6 restriction on plugins now is not acceptable. They can and should continue to work also with older, at least 1 back, framework minors.

//EDIT
Ah, apparently, tests bootstrap config gets partially resetted later one somehow

I set it as Cake\Core\Configure::write('Error.errorLevel', E_ALL & ~E_USER_DEPRECATED);

    'Error' => [
            'errorLevel' => (int) 16383
    ]

and confirmed that in bootstrap.
But then in the test case itself it is again:

            'errorLevel' => (int) 8191

Turns out I had to dereuromark/cakephp-ide-helper@12f056c#diff-98c12a693941ae3c4f50872403416471R14

@WyriHaximus

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2018

Woke up this morning to a bunch of failed builds from Dependabot (it auto updates my dependencies and made a PR for 3.6). Most of those seem fixable, but when trying to update @dereuromark's tags plugin I can across a suggested hasAssociation method that exists in 3.6 but doesn't in 3.5. I'm fine with deprecation errors, but having deprecation errors with no way to support both 3.5 and 3.6 so we as plugin authors can make live of our users easier, feels like a stab in the back.

Managed to fix TwigView and released 4.3.4 but I'de rather see an opt-in rather then a opt-out for this.

@dereuromark

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2018

@WyriHaximus Your changes are all invalid, your composer.json suggests 3.5.2 ( https://github.com/WyriHaximus/TwigView/blob/master/composer.json#L26 ), thus you cant use any of them. Thus your failing prefer lowest tests.
And yes, it is either a stab in the back of plugin devs (making life super hard), or a stab in the back of plugin users (by plugin devs, as admad did with search and just hardcoding it now to 3.6, forget about all other users).

@ADmad

This comment has been minimized.

Copy link
Member

commented Apr 16, 2018

as admad did with search and just hardcoding it now to 3.6

Hardcode? I published a new minor release which has Cake ^3.6 as dependency, so Cake 3.6 users will get that version and Cake 3.5 users will get the previous minor release of the plugin.

@dereuromark

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2018

@ADmad That is exactly the problem. They won't get any of the patches/bugfixes anymore. No way to port them back in a reasonable way/time - and this wouldn't even be necessary if we continued to at least support 3.5 here. No "new" functionality of 3.6 has been used at all.

@ADmad

This comment has been minimized.

Copy link
Member

commented Apr 16, 2018

If you want to ensure support for both 3.5 and 3.6 in your plugins then you can for e.g. use method_exists() for the few methods that were added in 3.6. It's really not that much work.

@dereuromark

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2018

you can for e.g. use method_exists() for the few methods that were added in 3.6. It's really not that much work.

I really hope you don't mean this seriously.. That would be sad.
You want to add cyclic complexity and if/else logic all over the code because of what exactly?
An annoying deprecation notice that has no functional value and is only supposed to be used as opt-in tool to help upgrading to 4.x which is still far away? Wow.

...

And yet another issue that now pops up:
phpstan of course doesnt like the opt-out either: https://travis-ci.org/dereuromark/cakephp-ide-helper/jobs/367368775

@WyriHaximus

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2018

@dereuromark ^3.5.2 still allows for 3.6, and all tests pass https://travis-ci.org/WyriHaximus/TwigView/builds/367355643 😉 . (Besides some 7.x tests for some odd reason.)

@ADmad While your suggestion is technically possible it wouldn't be really feasible due to the added complexion resulting in a mess.

How about we added the new methods from 3.6 to 3.5 so we can target out plugins to 3.5.99 and still support 3.5 without having to deal with a bombardment of errors? (For us and our users.)

@markstory

This comment has been minimized.

Copy link
Member

commented Apr 17, 2018

As the most vocal advocate of using error levels to make deprecations an optional process, I'm willing to concede that approach does not fit our community and that using a constant to enable warnings may better fit the expectations people have.

My question for you folks is that if we require a constant to be set for deprecations to display, what is the appropriate error level for warnings. Should they continue to be emitted at E_USER_DEPRECATED or E_USER_WARNING?

I ask this because getting the error level and constant right requires 2 knobs to be set correctly instead of the 1 we have today.

@markstory markstory changed the base branch from 3.next to master Apr 17, 2018

@markstory

This comment has been minimized.

Copy link
Member

commented Apr 17, 2018

From re-reading the thread it seems like folks are upset with how deprecations are being displayed. How would people feel if deprecation errors were only emitted into logs and not output in HTML/CLI contexts?

@saeideng

This comment has been minimized.

Copy link
Member

commented Apr 17, 2018

we can move deprecated warnings into debug_kit :)
just by changing a line in config/bootstrap.php

cakephp/debug_kit#605

@markstory

This comment has been minimized.

Copy link
Member

commented Apr 17, 2018

@saeideng We could, it would need to require no changes to the application code though. Right now disabling deprecations is also 1 line, and that is causing issues. The debug kit solution also doesn't address the issues that plugins have according to @dereuromark.

@saeideng

This comment has been minimized.

Copy link
Member

commented Apr 17, 2018

Right now disabling deprecations is also 1 line, and that is causing issues.

debug_kit will help us to find deprecated methods/addresses

plugins will can disable error_reporting in their Travis

@makallio85

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2018

I like idea, where all deprecations go to deprecations.log file.

But still I think it should be some config or constant that should be used over deprecated error reporting flag.

Turning deprecated error reporting on could cause pain with some vendors.

@davidyell

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2018

I would think they should still be E_USER_DEPRECATED as that is appropriate, and what I use in my own application code when changing my internal apis.

@lorenzo

This comment has been minimized.

Copy link
Member

commented Apr 17, 2018

@markstory I like the idea of only logging instead of visibly displaying the errors in the html

@inoas

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2018

Is there a way to always show deprecation errors but opt-out by setting a Configure value?
Or a somewhat hacky global cost as that's much faster as dereuromark explained rightly so.

@saeideng

This comment has been minimized.

Copy link
Member

commented Apr 17, 2018

I'm 👍 with E_USER_DEPRECATED
but we can log them here

protected function _displayError($error, $debug)
{
if (!$debug) {
return;
}
Debugger::getInstance()->outputError($error);

with this way we can still use debug_kit for showing deprecated warning in special panel

@saeideng

This comment has been minimized.

Copy link
Member

commented Apr 17, 2018

and this allow user to customize deprecated errors output

@markstory

This comment has been minimized.

Copy link
Member

commented Apr 17, 2018

@makallio85 What problems do you foresee? Wouldn't those vendors use deprecation warnings with the same goals? PHP also uses deprecation warnings in the way we currently are.

Logging instead of displaying will require a change to the default error handlers. We would need an additional configuration option to enable deprecations to be displayed, as the new default would be to log only.

markstory added a commit that referenced this pull request Apr 18, 2018

Don't display deprecation warnings by default.
Change the default error handling so that deprecations are not displayed
by default. Instead deprecations are only logged and can be displayed
through an opt-in setting.

Refs #11956
Refs #11889
@dereuromark

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2018

If any of your plugins have deprecations still, turn the warnings off unless you want to address the issues.

@markstory This does not work for tooling that do not respect the phpunit.xml error code set there.
Thus the default must never be the "BAD opt-out one", but instead a full opt-in toggle.
See stuff like https://travis-ci.org/dereuromark/cakephp-ide-helper/jobs/367368775

@dereuromark

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2018

Looks like there is further interest in this useful opt-in thingie. As such I will then close it.

@dereuromark dereuromark deleted the 3.next-depr-opt-in branch Jul 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.